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

Docs: MultiCommand.resultcallback vs MultiCommand.result_callback #1160

Closed
NotTheEconomist opened this issue Nov 3, 2018 · 2 comments · Fixed by #1848
Closed

Docs: MultiCommand.resultcallback vs MultiCommand.result_callback #1160

NotTheEconomist opened this issue Nov 3, 2018 · 2 comments · Fixed by #1848
Assignees
Milestone

Comments

@NotTheEconomist
Copy link

NotTheEconomist commented Nov 3, 2018

The commands doc page shows usage of MultiCommand.resultcallback under the header "Multi Command Pipelines" as being a callable itself, that is passed all the associated callbacks with the chained group.

Where do the returned functions go? The chained multicommand can register a callback with MultiCommand.resultcallback() that goes over all these functions and then invoke them.

However, at the bottom of that same doc page, it seems to imply that MultiCommand.result_callback is the same.

Return values of groups can be processed through a MultiCommand.result_callback. This is invoked with the list of all return values in chain mode, or the single return value in case of non chained commands.

The API seems to show that result_callback is an attribute that stores the callback itself, while resultcallback is a function that sets what the callback should be. The naming here is confusing, and it's not clear which one is which. In addition: referring to both of them in the documentation (without clarifying which is which and why) throws an additional spanner into the works.

I recommend a rename for a permanent solution (result_callback -> _result_callback) and more explicit documentation in the short-term.

@jcrotts
Copy link
Contributor

jcrotts commented May 23, 2019

I don't think adding a _ really improves clarity.
The parentheses imply resultcallback is a function, and the dot lookup implies result_callback is an attribute.

How would you suggest the documentation be improved?

@jcrotts jcrotts added the docs label May 23, 2019
@davidism davidism added f:chain feature: chained commands and removed save-for-sprint labels Jun 16, 2020
@davidism davidism added this to the 9.0.0 milestone Aug 13, 2020
@davidism davidism modified the milestones: 9.0.0, 8.0.0 Apr 13, 2021
@davidism davidism removed docs f:chain feature: chained commands labels Apr 13, 2021
@davidism
Copy link
Member

davidism commented Apr 13, 2021

I'm renaming the decorator to @result_callback() and the attribute to _result_callback, since it's only used to store the callback, it doesn't appear to be intended to be set directly as a public API. The docs will not mention the attribute and will only mention the renamed decorator.

@davidism davidism self-assigned this Apr 13, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 28, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants