Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Nested enums now translate to nested states #405

Merged

Conversation

thedrow
Copy link
Contributor

@thedrow thedrow commented Mar 25, 2020

Fixes #404.

This needs documentation but I don't know which version this feature will released on.
I'm assuming 0.9.0. Am I correct?

@coveralls
Copy link

coveralls commented Mar 25, 2020

Coverage Status

Coverage decreased (-0.04%) to 98.481% when pulling 2d9e508 on thedrow:nested-enums-to-nested-state into 070b450 on pytransitions:master.

@aleneum
Copy link
Member

aleneum commented Mar 26, 2020

That looks good to me. Some minor tweeks need to be done though (for instance here). Did you check whether the resulting Graphviz graphs look okay? Is it possible to determine the actual state and can you use auto transitions?:

from enum import Enum, auto
class Foo(Enum):
  A = auto()  # 1 
  B = auto()  # 2

class Bar(Enum):
  FOO = Foo
  C = auto()  # 1

m = HierarchicalMachine(states=Bar, initial=Bar.C)
m.to_FOO_A()  # does this work?
assert m.is_C() is False  # a check by state.value would return True though 

@thedrow
Copy link
Contributor Author

thedrow commented Mar 29, 2020

Do we need to remove this check?

if isinstance(model_states, Enum):
res[model_states.name] = {} # since enum cannot be nested we do not need an OrderedDict here

@thedrow
Copy link
Contributor Author

thedrow commented Mar 29, 2020

Setting the initial to Bar.C uncovered a bug in this PR that causes the test to fail because of the Graphvis extension.
I fixed it now. I'll generate a graph and see if it looks correct.

@thedrow
Copy link
Contributor Author

thedrow commented Mar 29, 2020

Looks right to me :)
g

@aleneum aleneum changed the base branch from master to next-release March 30, 2020 00:00
@aleneum aleneum merged commit 2d9e508 into pytransitions:next-release Mar 30, 2020
@aleneum
Copy link
Member

aleneum commented Mar 30, 2020

Merged! Thanks again for your valuable contributions!

Setting the initial to Bar.C uncovered a bug in this PR that causes the test to fail because of the Graphvis extension.

this is comparable to #400 and should be fixed with 0a66a36. An additional test in set_node_style should not be necessary anymore. I removed it.

Sidenote: Enums with identical values cannot be distinguished. That should be at least mentioned in the documentation.

@thedrow
Copy link
Contributor Author

thedrow commented Mar 30, 2020

Do we need to remove this check?

if isinstance(model_states, Enum):
res[model_states.name] = {} # since enum cannot be nested we do not need an OrderedDict here

Wait, what about this? Nevermind, I see your commits.
But check my comments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for nested states from nested Enums
3 participants