-
Notifications
You must be signed in to change notification settings - Fork 18
feature: add current and next state matches to on_enter and on_exit #19
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
Conversation
Hm, this makes the API a bit more confusing. Would a default type work here (ex |
I'm not super familiar with rust, but I did try this and found that Rust does not support default type params for functions: rust-lang/rust#36887. New methods could be introduced (like |
Good idea. I'll give a proper review soon. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change on_enter
and on_exit
back and add on_enter_from
and on_exit_to
(as you suggested). Same for command_on_enter
et al.
Someday, I'll add a feature that makes all the command_on_*
functions unnecessary, so all this duplication is temporary.
Thanks!
src/machine.rs
Outdated
pub fn on_enter<S: EntityState>( | ||
/// Adds an on-enter event to the state machine. Whenever the state machine transitions to the | ||
/// given next state from the given current state, it will run the event. | ||
pub fn on_enter<NextState: EntityState, CurrentState: EntityState>( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NextState
should be called Next
and CurrentState
should be called Prev
to match other methods.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
src/machine.rs
Outdated
pub fn on_enter<S: EntityState>( | ||
/// Adds an on-enter event to the state machine. Whenever the state machine transitions to the | ||
/// given next state from the given current state, it will run the event. | ||
pub fn on_enter<NextState: EntityState, CurrentState: EntityState>( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Put CurrentState
before NextState
. Both ways have ways they can be confusing, but putting CurrentState
first feels to me more consistent with the rest of the API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aside from this, it looks good
Whoops forgot to add the comment |
Thank you |
Implements #18.