List available transactions given the state of the state machine #22

sammydowds · 2020-10-08T20:29:07Z

1 New Class:

TransitionMeta which is added to the transition methods .__fsm attribute

2 New StateMachine methods:

all_transitions, returns list of all Transitions
available_transitions, returns list of all available Transitions based on state of the StateMachine

At first, I thought this would be way simpler. Let me know if this is not the solution you were looking for!

It looks like pytest still passes, however this is only my second PR - so I would love some feedback!

… reference to transition objects in the wrapper function

…nsitions from TransitionMeta

codecov-io · 2020-10-08T20:29:48Z

Codecov Report

Merging #22 into master will decrease coverage by 1.95%.
The diff coverage is 77.77%.

@@            Coverage Diff             @@
##           master      #22      +/-   ##
==========================================
- Coverage   96.10%   94.15%   -1.96%     
==========================================
  Files           7        7              
  Lines         154      171      +17     
==========================================
+ Hits          148      161      +13     
- Misses          6       10       +4

Impacted Files	Coverage Δ
finite_state_machine/draw_state_diagram.py	`81.81% <ø> (ø)`
finite_state_machine/state_machine.py	`95.06% <77.77%> (-4.94%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bb96cf3...732e456. Read the comment docs.

alysivji

Think this should work, but it feels like you are bringing a lot of complexity from django-fsm into this project. While this library is inspired by django-fsm, we don't need to copy all of their design decisions... unless the complexity makes sense for the project

The way I would think about this is by looking at tests. This PR adds 50 lines of code, 30 of which are untested. How much work would it be to write tests for these changes? I think it's going to be a sizeable amount.

I would take a step back and think... what information do you need to get the list of available transactions? As each @transition function is read in, we can dynamically create a source-to-target lookup table. That table only changes when additional functions are wrapped with @transition decorators are added.

finite_state_machine/state_machine.py

finite_state_machine/exceptions.py

finite_state_machine/state_machine.py

…reation of dictionary with source->target

sammydowds

Removed meta class for transitions
Added back in the original logic
Added logic in the transition_decorator function to pull the StateMachine instance
Added logic to build out a python dict to an "__fsm" attribute of the StateMachine instance with structure {source: [targets]}

I did not realize commits automatically appeared in the PR - sorry for the mess.

sammydowds · 2020-10-23T14:16:04Z

finite_state_machine/exceptions.py

-
-class TransitionNotAllowed(Exception):
-    def __init__(self, *args, **kwargs):
-        self.object = kwargs.pop('object', None)
-        self.method = kwargs.pop('method', None)
-        super(TransitionNotAllowed, self).__init__(*args, **kwargs)


Removing this exception all together.

sammydowds · 2020-10-23T14:17:44Z

finite_state_machine/state_machine.py

+        mems = inspect.getmembers(func)
+        state_machine_instance = [
+            mem[1]["StateMachine"] for mem in mems if mem[0] == "__globals__"
+        ][0]


Saving instance of StateMachine to later add "__fsm" attribute. It turned out func was type "function" not "method".

this could be replced by state_machine_instance = func.__self__

When I tried this it would not work because func is of type function not a method. So I had to dive into the globals referenced for the function type here: https://docs.python.org/3/library/inspect.html. Perhaps I am missing something.

Although, let me play around with it again.

Ahhh.... doing a quick google search, doesn't seem like it's possible without inspect. 🤦

I think a possible workaround might be to use a class as a function decorator. Don't worry about it. This works, let's just go with it. If this library has some magic, that's okay. It makes the code which uses this library cleaner which is really what we want.

sammydowds · 2020-10-23T14:22:07Z

finite_state_machine/state_machine.py

+        if hasattr(state_machine_instance, "__fsm"):
+            if isinstance(source, list):
+                for src in source:
+                    if src in state_machine_instance.__fsm:
+                        state_machine_instance.__fsm[src].append(target)
+                    else:
+                        state_machine_instance.__fsm[src] = [target]
+            else:
+                if source in state_machine_instance.__fsm:
+                    state_machine_instance.__fsm[src].append(target)
+                else:
+                    state_machine_instance.__fsm[src] = [target]
        else:
-            func.__fsm.add_transition(source, target, on_error, conditions)
+            if isinstance(source, list):
+                state_machine_instance.__fsm = {}
+                for src in source:
+                    state_machine_instance.__fsm[src] = [target]
+            else:
+                state_machine_instance.__fsm.source = [target]


This probably needs to be optimized. Checks if the StateMachine has an attribute "__fsm", and appends a target to an existing source or creates an entry in a dict of structure {source: [target]}.

Two questions:

Any advice on how to optimize this?

I like collections.defaultdict as the data type. Should I build that out?

collections.defaultdict would definitely reduce one level of nesting. I would try that out and see what you are left with

Will be easier to find the next optimization

Since our types at the moment are booleans, ints, and strings. We can probably start with the default type of a set. Not sure how that would work for enums.

I still need to investigate that.

sammydowds · 2020-10-23T14:23:17Z

finite_state_machine/state_machine.py

-class TransitionMeta(object):
-    def __init__(self, name):
-        self.name = name
-        self.transitions = {}
-
-    def get_transition(self, source):
-        transition = self.transitions.get(source, None)
-        if transition is None:
-            transition = self.transitions.get("*", None)
-        if transition is None:
-            transition = self.transitions.get("+", None)
-        return transition
-
-    def add_transition(self, source, target, on_error=None, conditions=[]):
-        if source in self.transitions:
-            raise AssertionError("Duplicate transition for {0} state".format(source))
-        self.transitions[source] = Transition(
-            name=self.name, source=source, target=target, on_error=on_error, conditions=conditions
-        )
-
-    def next_state(self, current_state):
-        transition = self.get_transition(current_state)
-        if transition is None:
-            raise TransitionNotAllowed("No transition from {0}".format(current_state))
-        return transition.target
-
-


Removing an overly complex meta class.

sammydowds · 2020-10-23T14:24:13Z

finite_state_machine/draw_state_diagram.py

+    class_fns = inspect.getmembers(cls, predicate=inspect.isfunction)
+    state_transitions: List[Transition] = [
+        func.__fsm for name, func in class_fns if hasattr(func, "__fsm")
+    ]


Adding back in the original logic to the file.

alysivji

This looks good, think you might need to add the available_actions method on the base StateMachine class.

Can you also write a couple of tests to make sure this returns what you expect?

sammydowds added 6 commits October 8, 2020 10:29

Adding meta class for transitions

b7464b4

adding methods to transition meta class

84654d5

Adding creation of TransitionMeta in transition function, and updated…

e44968d

… reference to transition objects in the wrapper function

Importing new exception, plus correct formatting errors

496284c

Fixing formatting

7efea97

Updating logic in the generating state markdown to create list of tra…

b5ea22b

…nsitions from TransitionMeta

sammydowds mentioned this pull request Oct 8, 2020

Available actions #19

Open

alysivji requested changes Oct 8, 2020

View reviewed changes

finite_state_machine/state_machine.py Outdated Show resolved Hide resolved

finite_state_machine/state_machine.py Outdated Show resolved Hide resolved

finite_state_machine/exceptions.py Outdated Show resolved Hide resolved

finite_state_machine/state_machine.py Outdated Show resolved Hide resolved

Added in inspect module to pull instance of StateMachine, and added c…

c6a63dd

…reation of dictionary with source->target

sammydowds force-pushed the available_actions branch from b522af6 to c6a63dd Compare October 14, 2020 13:45

sammydowds added 2 commits October 14, 2020 09:24

Removing init attr dict, adding comment on lines 61-80.

16f897f

Removing accidental duplication

732e456

sammydowds requested a review from alysivji October 23, 2020 14:38

sammydowds commented Oct 23, 2020

View reviewed changes

alysivji requested changes Oct 23, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

List available transactions given the state of the state machine #22

List available transactions given the state of the state machine #22

sammydowds commented Oct 8, 2020

codecov-io commented Oct 8, 2020 •

edited

Loading

alysivji left a comment

sammydowds left a comment

sammydowds Oct 23, 2020

sammydowds Oct 23, 2020

alysivji Oct 23, 2020

sammydowds Oct 23, 2020 •

edited

Loading

alysivji Oct 23, 2020

sammydowds Oct 23, 2020

alysivji Oct 23, 2020

alysivji Oct 23, 2020

sammydowds Oct 23, 2020

sammydowds Oct 23, 2020

alysivji left a comment

List available transactions given the state of the state machine #22

Are you sure you want to change the base?

List available transactions given the state of the state machine #22

Conversation

sammydowds commented Oct 8, 2020

codecov-io commented Oct 8, 2020 • edited Loading

Codecov Report

alysivji left a comment

Choose a reason for hiding this comment

sammydowds left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sammydowds Oct 23, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alysivji left a comment

Choose a reason for hiding this comment

codecov-io commented Oct 8, 2020 •

edited

Loading

sammydowds Oct 23, 2020 •

edited

Loading