pytransitions / transitions

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

Raise exception when adding transition to non-existing state #394

Closed ltog closed 4 years ago

ltog commented 4 years ago

Using transitions=0.7.2 I worked on code that looked like this:

#!/usr/bin/env python3

from transitions import Machine

class Blub(object):
    states = ['no', 'yes']

    def __init__(self):

        self.machine = Machine(model=self, states=Blub.states, initial='no')

        self.machine.add_transition(trigger='nod',
                                    source='no',
                                    dest='yes')

        # add internal transition
        self.machine.add_transition(trigger='nod',
                                    source='yes',
                                    dest='None') # <-- should not have quotes, exception should happen here

if __name__=='__main__':
    b = Blub()
    print(b.state)
    b.nod()
    print(b.state)
    b.nod()
    print(b.state) # but exception happens here

The second transition should have been an internal transition (defined by setting dest=None) but by mistake I wrote None in quotes. This resulted in transitions looking for a state called None.

Generally speaking, I would have expected that trying to add a transition to a non-existing state would raise an exception immediately. However, an exception is only raised when one indeed tries to do the transition.

I propose to add a check in add_transition looking for invalid source and destination states. This check would raise an exception if an invalid state was encountered. This would allow to detect invalid configurations early and decrease the risk of errors happening in seemingly running software.

PS: After typing all of my text I saw that https://github.com/pytransitions/transitions/issues/155 already mentioned my concern. A tradeoff seems to be necessary between code complexity, the ability of detecting errors early and being able to dynamically adding states after already having defined transitions using these states.

aleneum commented 4 years ago

Hello @ltog,

as you already mentioned this behaviour is by design as transitions is used in many cases where machine configurations dynamically change, This has a drawback as you pointed out. If you don't need that behaviour I'd suggest to derive a "StrictMachine" from your needed machine class. The easiest alteration would be to override the actual creation of a transition in Machine._create_transition since at this point special arguments such as wildcards, enums or state instances have already been resolved.

from transitions import Machine

class StrictMachine(Machine):

    def _create_transition(self, source, dest, *args, **kwargs):
        for sta in [source, dest]:
            if sta not in self.states:
                raise ValueError(f"'{sta}' is not a valid state")
        return self.transition_cls(source, dest, *args, **kwargs)

m = Machine(states=['A', 'B'], transitions=[['go', 'A', 'C']], initial='A')
m.add_state('C')
m.go()
assert m.is_C()

# raises ValueError: 'C' is not a valid state
m = StrictMachine(states=['A', 'B'], transitions=[['proceed', 'A', 'B'], ['go', 'A', 'C']], initial='A')
aleneum commented 4 years ago

Since there is no feedback I assume this issue being resolved. If this is not the case, feel free to comment and I will reopen the issue if necessary.

ltog commented 4 years ago

@aleneum : Thank you for the code snippet. That looks quite concise. I had hoped for a switch that could configure the mentioned behaviour so this would work out of the box, but if that won't be happening, your StrictMachine is a good alternative.

Thank you for maintaining transitions and stay healthy!

aleneum commented 4 years ago

Much appreciated. You too, @ltog.