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

Nothing like set_state("string") for transitions #134

Closed
IwanLD opened this issue Aug 25, 2016 · 9 comments
Closed

Nothing like set_state("string") for transitions #134

IwanLD opened this issue Aug 25, 2016 · 9 comments

Comments

@IwanLD
Copy link

IwanLD commented Aug 25, 2016

  1. Both transitions and states can be added via strings
  2. states can be reached via strings

Still I did not find a method in core.py to reach a transition via string. In my opinion a necessary feature to use transitions for APIs where triggers get passed via string.

Currently I'm doing

def _set_transition(self, name): eval('self.' +name+'()')
self._set_transition("string")

to all my classes. But It would be great to have

self.machine.set_transition("string")

instead.

@tyarkoni
Copy link
Member

Can you elaborate? What would the expected behavior be? It's true that accessing Transition instances is a bit cumbersome at the moment, but part of the issue is that transitions aren't uniquely identified by the trigger name--there can be multiple transitions triggered by the same model call, depending on the current state. Is the idea that calling model.get_transition('my_trigger_name') would return the first Transition instance valid for the model's current state and the named trigger method? Or are you thinking of something else? We can probably accommodate this, I just want to make sure I understand what you're looking for.

@tyarkoni
Copy link
Member

tyarkoni commented Aug 25, 2016

On re-reading, maybe I'm overthinking this. Do you just mean you want to fire a trigger based on a string name? I.e., instead of model.my_trigger(), you want model.fire_event('my_trigger')? If so, I think the Pythonic approach would be:

getattr(model, 'my_trigger')()

I don't think you need to eval() anything.

Note also that the Machine class has a set_state() method, so you can also call machine.set_state('desired_state') to achieve the same thing. I suppose we could add (or move) this to the model, but it seems to work fine as-is.

@IwanLD
Copy link
Author

IwanLD commented Aug 25, 2016

Yes, I just want to fire a trigger based on a string name.

Thank you, getattr(model, 'my_trigger')() is sure more pythonic!

Jest ran some tests and set_state really does work on both states and transitions. Was not clear to me from the otherwise great documentation. To me that's rather confusing, since states and transitions are two rather different entities. In my opinion to have set_state() work only on states and add a set_transitions() that works on transitions only would be much cleaner, since else you can come into namespace conflicts - and obviously it is not very intuitive this way.

Sure not critical though, since there is an easy existing workaround.

@tyarkoni
Copy link
Member

tyarkoni commented Aug 26, 2016

Well, one limitation (though it's actually a feature and not a bug) of set_state() is that it won't cause any of the callbacks to fire. It'll just immediately move the model to the specified state. So you don't want to use set_state() if you need the callbacks to do stuff for you. I would probably go with the getattr approach above.

On further reflection though, I do think it probably makes sense to have an easy way to trigger events from the model by passing a string. While the approach I suggested above works fine, the user shouldn't really have to reach inside the model that way. @aleneum and @wtgee, any objections to my adding a trigger() method to the Model?

@wtgee
Copy link
Contributor

wtgee commented Aug 27, 2016

I actually have a private method called _lookup_trigger that I use to accomplish this same thing, so I certainly wouldn't be opposed. All of my states are loaded from an external file so my method does the lookup against a _state_table, which is just a yaml document that has been loaded and stored.

My method does also offer up a default transition if the requested transition can't be found. Not sure if we would want to build that functionality into it as it would be easy enough to write if someone needed it, but worth considering.

@aleneum
Copy link
Member

aleneum commented Aug 28, 2016

On further reflection though, I do think it probably makes sense to have an easy way to trigger events from the model by passing a string.

Easy to implement and maintain. Since this seems to add convenience I am all for it 👍

My method does also offer up a default transition if the requested transition can't be found.

This could be the second candidate for a model extension module: MixInFiniteStates and MixInDefaultTransition. What do you think?

@wtgee
Copy link
Contributor

wtgee commented Aug 28, 2016

👍 for the simple method. Let's leave the default until later. I don't think it would need a full extension module and I would be a little sceptical to start making too many extensions without a clear need although am not actually opposed.

@tyarkoni
Copy link
Member

I was planning to get to this once the multiple-models branch was merged, but thanks for taking care of it! :)

@aleneum
Copy link
Member

aleneum commented Aug 29, 2016

It was a welcome way to distract myself from something I (don't) want to do today ;).
And closing tickets is fun (sorry for taking that away).
BUT... stumbled upon a pygraphviz issue which occurs sometimes -_-
Update: Best way to work around error message generation issues is to prevent errors 🙈

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

No branches or pull requests

4 participants