pytransitions / transitions

A lightweight, object-oriented finite state machine implementation in Python with many extensions
MIT License
5.49k stars 524 forks source link

Enum members cannot contain underscores without changing the separator #487

Closed thedrow closed 3 years ago

thedrow commented 3 years ago

If you have the following enum:

class MyEnum(Enum):
  bla_foo = auto()

When you try to initialize a state machine with it you get:

../jumpstarter/actors.py:53: in __init__
    super().__init__(
/home/thedrow/.cache/pypoetry/virtualenvs/jumpstarter-q-gDbjwh-py3.9/lib/python3.9/site-packages/transitions/extensions/nesting.py:335: in __init__
    _super(HierarchicalMachine, self).__init__(*args, **kwargs)
/home/thedrow/.cache/pypoetry/virtualenvs/jumpstarter-q-gDbjwh-py3.9/lib/python3.9/site-packages/transitions/core.py:565: in __init__
    self.add_states(states)
/home/thedrow/.cache/pypoetry/virtualenvs/jumpstarter-q-gDbjwh-py3.9/lib/python3.9/site-packages/transitions/extensions/nesting.py:462: in add_states
    self._init_state(new_state)
/home/thedrow/.cache/pypoetry/virtualenvs/jumpstarter-q-gDbjwh-py3.9/lib/python3.9/site-packages/transitions/extensions/nesting.py:880: in _init_state
    with self(state.name):
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

self = <jumpstarter.actors.Actor object at 0x7f1dbeee1520>, to_scope = 'bla_a'

    def __call__(self, to_scope=None):
        if isinstance(to_scope, string_types):
            state_name = to_scope.split(self.state_cls.separator)[0]
>           state = self.states[state_name]
E           KeyError: 'bla'

/home/thedrow/.cache/pypoetry/virtualenvs/jumpstarter-q-gDbjwh-py3.9/lib/python3.9/site-packages/transitions/extensions/nesting.py:340: KeyError
aleneum commented 3 years ago

Yes, that's why you can change the separator of hierarchical states. The reason why _ is the default value is due to the fact that convenience functions can be used with underscores while using a dot for instance is syntactically not possible. As the documentation states

Some things that have to be considered when working with nested states: State names are concatenated with NestedState.separator. Currently the separator is set to underscore ('_') and therefore behaves similar to the basic machine. This means a substate bar from state foo will be known by foo_bar. A substate baz of bar will be referred to as foo_bar_baz and so on. When entering a substate, enter will be called for all parent states. The same is true for exiting substates. Third, nested states can overwrite transition behaviour of their parents. If a transition is not known to the current state it will be delegated to its parent.

This means that in the standard configuration, state names in HSMs MUST NOT contain underscores. For transitions it's impossible to tell whether machine.add_state('state_name') should add a state named state_name or add a substate name to the state state. In some cases this is not sufficient however. For instance if state names consists of more than one word and you want/need to use underscore to separate them instead of CamelCase. To deal with this, you can change the character used for separation quite easily. You can even use fancy unicode characters if you use Python 3. Setting the separator to something else than underscore changes some of the behaviour (auto_transition and setting callbacks) though:

Do you have a suggestion about how to deal with (nested) state names more elegantly while keeping the default functionality close to standard Machine?

thedrow commented 3 years ago

You probably need to migrate from using strings to using the enum objects themselves.

thedrow commented 3 years ago

I forgot to mention: My complaint is that the error is non-obvious and requires you to read the documentation.

If there's another fix for this, I'll be glad but at the very least, we should raise a proper error.

aleneum commented 3 years ago

At least for strings there is hardly any way to tell whether passing "FOO_BAR" was intentional or not. Looking at the tests machine.add_state("FOO_BAR") should add a state "FOO" and a substate "FOO_BAR" in case "FOO" hasnt been there. For Enum this wont work though and splitting Enum names does not make much sense. Thus, ONLY for enums its possible to raise an error. 62c72d5 added raising such a warning:

from transitions.extensions import HierarchicalMachine
from transitions import MachineError
from enum import Enum, auto

class MyEnum(Enum):
    BLA_FOO = auto()

try:
    m = HierarchicalMachine(states=MyEnum, initial=MyEnum.BLA_FOO)
except ValueError as e:
    # full message:
    # ValueError: "State 'BLA_FOO' contains '_' which is used as state name separator.
    # Consider changing the NestedState.separator to avoid this issue."
    assert str(e).index("Consider changing the NestedState.separator to avoid this issue.") > -1

m = HierarchicalMachine(states=['FOO_BAR'], initial='FOO_BAR')
assert m.is_FOO(allow_substates=True)
assert m.is_FOO_BAR()
assert "FOO" in m.states
assert "FOO_BAR" not in m.states
aleneum commented 3 years ago

FYI: As noted in b398229, instead of a MachineError, a ValueError is raised when the check fails.

thedrow commented 3 years ago

I guess it's better than before. Thanks.