Skip to content

Make continuation FnOnce#15

Merged
WaffleLapkin merged 4 commits intomasterfrom
stuff
Jul 18, 2022
Merged

Make continuation FnOnce#15
WaffleLapkin merged 4 commits intomasterfrom
stuff

Conversation

@WaffleLapkin
Copy link
Copy Markdown
Contributor

This generally makes code a little bit nicer

@WaffleLapkin WaffleLapkin requested a review from hirrolot July 12, 2022 16:15
@WaffleLapkin WaffleLapkin enabled auto-merge July 12, 2022 16:15
Copy link
Copy Markdown
Member

@hirrolot hirrolot left a comment

Choose a reason for hiding this comment

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

This is a breaking change, right?

@WaffleLapkin
Copy link
Copy Markdown
Contributor Author

Yes, this is a breaking change.

@hirrolot
Copy link
Copy Markdown
Member

The only reason to make the change is to structure the code in the library with less hassle? In Rust, we normally expose the most generic function type to library users, which is Fn in our case. Yes, FnOnce may look nicer in the library bowels, but I don't think we should introduce this change for just this reason. In addition, this would break someone's code.

@WaffleLapkin
Copy link
Copy Markdown
Contributor Author

While it's true that this change makes calling the continuation more restrictive, at the same time it makes writing the continuation a lot less restrictive. We lose ability to call continuation multiple times (that we didn't use anyway) but gain ability to move stuff into the closure, to set flags from inside it, etc. There really isn't any reason for the continuation to be Fn.

[...] the most generic function type to library users, which is Fn in our case

Not really, as said before: it's more permissive to call, but not to provide.

In addition, this would break someone's code.

While this could of course be the case, this is highly unlikely. For this change to break something a user would need to be writing their own handlers (instead of using tools provided by this library) and calling the continuation multiple times (Fn is trivially FnOnce so providing continuation can't be broken).

@hirrolot hirrolot self-requested a review July 18, 2022 10:22
Copy link
Copy Markdown
Member

@hirrolot hirrolot left a comment

Choose a reason for hiding this comment

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

Yes, sorry, I forgot for a minute that the hierarchy of functions is FnOnce : FnMut : Fn, not the opposite.

The only missing change is updating CHANGELOG.md.

@WaffleLapkin WaffleLapkin requested a review from hirrolot July 18, 2022 10:53
@WaffleLapkin WaffleLapkin merged commit e4d5b8a into master Jul 18, 2022
@WaffleLapkin WaffleLapkin deleted the stuff branch July 18, 2022 10:58
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.

2 participants