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

Relax callback bounds #229

Merged
merged 4 commits into from May 14, 2024
Merged

Conversation

adwhit
Copy link
Contributor

@adwhit adwhit commented May 9, 2024

I noticed bounds on several of the traits are overly restrictive - they receive callbacks of type Fn when type FnOnce (or FnMut) would work. In general it is preferable to use the less restrictive type in APIs.

According to the docs, "Since both Fn and FnMut are subtraits of FnOnce, any instance of Fn or FnMut can be used where a FnOnce is expected." - therefore relaxing the trait bounds is non-breaking.

Copy link
Member

@charypar charypar left a comment

Choose a reason for hiding this comment

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

Thanks for doing this! ❤️ Looks good to me, but would like at least one more ✅.

Can't quite see whether we might want to restrict them again later or why we would. All those Fns are generally meant to be the event variant constructors which are used as a form of callback, so most of them are indeed called once.

The ones used in subscriptions get cloned because they potentially move between threads anyway (would be nice to avoid that, but I think I once tried and couldn't work it out)

Copy link
Member

@StuartHarris StuartHarris left a comment

Choose a reason for hiding this comment

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

Yeah, personally, I think this is the right thing to do! Thank you, Alex! Couple of small changes to allow the examples to compile.

crux_time/tests/time_test.rs Outdated Show resolved Hide resolved
examples/counter/shared/src/capabilities/sse.rs Outdated Show resolved Hide resolved
Copy link
Member

@StuartHarris StuartHarris left a comment

Choose a reason for hiding this comment

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

Thanks!

@StuartHarris StuartHarris merged commit d3e36ba into redbadger:master May 14, 2024
9 checks passed
@adwhit adwhit deleted the relax-callback-bounds branch May 14, 2024 12:28
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.

None yet

3 participants