You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
After testing, #1586 had some major flaws in practice
In contrary to Rust idioms, the method chaining was done using &Self, which makes things like passing a method chain into a parameter expecting an own more cumbersome than necessary.
It was a blunt instrument, being at best minor pollution, and at worst, a hindrance. Especially once the first point is addressed, forcing all void returns to be owning self (and by extension take owning self) was not practical.
This replaces the --enable-method-chaining switch with a --chainable-methods parameter, that works similar to the current async filter. You can choose to opt-in to it for all methods, for all methods within a particular resource, or just for a particular method (or any combination of those).
Additional, methods with method chaining enabled now take owning self instead of the default borrowing &self, and are -> Self rather than -> &Self. The ABI remains unaffected.
For crates/core/src/chainable_method.rs it looks like that's more-or-less a duplicate of crates/core/src/async_.rs I think? Could those be deduplicated? Overall though I do think it's nice to have more fine-grained control over the configuration here.
For the implementation, personally I think returning &Self would be more suitable here because that still allows chaining and is still an idiomatic way of doing chaining in Rust. It's true that it doesn't allow returning something, so therading it all the way through would require a temporary, but personally I think that's reasonable (and is already the case for other builder-like patterns such as Command in the standard library).
Additionally for the implementation, would it make sense to give the FunctionBindgen structure access to the underlying Function? That way the return_self: bool parameter should largely go away since the function itself could be stored and then the predicate could be called on-demand.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow up to #1586
After testing, #1586 had some major flaws in practice
&Self, which makes things like passing a method chain into a parameter expecting an own more cumbersome than necessary.This replaces the
--enable-method-chainingswitch with a--chainable-methodsparameter, that works similar to the current async filter. You can choose to opt-in to it for all methods, for all methods within a particular resource, or just for a particular method (or any combination of those).Additional, methods with method chaining enabled now take owning
selfinstead of the default borrowing&self, and are-> Selfrather than-> &Self. The ABI remains unaffected.