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

Add After dispatch hook and Before transition, rename existing hooks #23

Merged
merged 17 commits into from
Jan 8, 2025

Conversation

dragonnn
Copy link
Contributor

Although on_dispatch and on_transition hook are intend to be used for loging in some cases it can be useful to abuse them to have a global even handler that stores some filtered events in self. For that case it sometimes makes sense to be able to store something in self after a state handler is called to for example use it for storing previous values that can be used when a new value comes in to compare too.

@mdeloof
Copy link
Owner

mdeloof commented Jan 1, 2025

I'm open to adding an after_dispatch hook, but we should then probably rename on_dispatch to before_dispatch, and on_transition to after_transition. For completeness sake we should probably also add before_transition.

(My apologies for not responding to this sooner. I'll try to be more responsive going forward :) )

@dragonnn
Copy link
Contributor Author

dragonnn commented Jan 2, 2025

Thanks for looking into it! I do agree renaming all of those and adding before before_transition. Do you want all of that in this PR?

@mdeloof
Copy link
Owner

mdeloof commented Jan 2, 2025

Yes, if you want to, you can go ahead and make the changes on this PR. But if you're not able to do it at the moment, just let me know and I'll see what I can do.

@dragonnn dragonnn changed the title After dispatch hook Add After dispatch hook and Before transition, rename existing hooks Jan 5, 2025
@dragonnn
Copy link
Contributor Author

dragonnn commented Jan 5, 2025

Sorry for the delay, should be done. I don't think I missed anything. I renamed the consts for hooks to uppercase in IntoStateMachine too as suggested by clippy.

@christianheussy
Copy link

Isn't this a breaking change? I would think the manifest version should be revised to 0.4.0. Alternatively, maybe the macro could still support the previous names and emit a warning that they are deprecated?

@dragonnn
Copy link
Contributor Author

dragonnn commented Jan 5, 2025

Definitely a breaking change, if you would like to have the version bump in this PR too I am will add that right away, didn't do it because I am not sure how you would like to manage that.

Copy link
Owner

@mdeloof mdeloof left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you forgot to call M::BEFORE_TRANSITION() in the transition() function? :) Other than that everything looks good, just a couple of comments with the wrong name.

Regarding the version bump, you can change it to 0.4.0 for both the statig and statig_macro crate.

examples/macro/async_blinky/src/main.rs Outdated Show resolved Hide resolved
examples/macro/async_blinky/src/main.rs Outdated Show resolved Hide resolved
examples/macro/async_blinky/src/main.rs Outdated Show resolved Hide resolved
statig/src/lib.rs Show resolved Hide resolved
statig/src/lib.rs Show resolved Hide resolved
statig/src/lib.rs Show resolved Hide resolved
@dragonnn
Copy link
Contributor Author

dragonnn commented Jan 8, 2025

Thanks! I hope I didn't missing anything, should be fine now

@dragonnn dragonnn requested a review from mdeloof January 8, 2025 11:38
Copy link
Owner

@mdeloof mdeloof left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Almost there I think, just a small error in the docs left. You will also have to bump the version of the statig_macro dependency in /statig/Cargo.toml.

[dependencies]
-statig_macro = { path = "../macro", version = "0.3.0", optional = true }
+statig_macro = { path = "../macro", version = "0.4.0", optional = true }

README.md Outdated Show resolved Hide resolved
@dragonnn
Copy link
Contributor Author

dragonnn commented Jan 8, 2025

Oh, my bad! Sorry, I should catch that. Should be fine now :)

@dragonnn dragonnn requested a review from mdeloof January 8, 2025 18:26
@mdeloof mdeloof merged commit bbda6fe into mdeloof:main Jan 8, 2025
2 checks passed
@mdeloof
Copy link
Owner

mdeloof commented Jan 8, 2025

No problem ;) Thanks again for your work!

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.

3 participants