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

deprecate MultiCommand, merge into Group #2590

Closed
davidism opened this issue Aug 18, 2023 · 4 comments · Fixed by #2591
Closed

deprecate MultiCommand, merge into Group #2590

davidism opened this issue Aug 18, 2023 · 4 comments · Fixed by #2591
Assignees
Milestone

Comments

@davidism
Copy link
Member

davidism commented Aug 18, 2023

As part of rewriting the parser in #2205, I was looking at the complexities of Click's definitions and processing. MultiCommand and CommandCollection could be combined with Group to simplify to a single way of working with multiple commands. This is a little more difficult than #2589 removing BaseCommand.

Currently, they're distinct because Group provides further group and command decorators. MultiCommand is the base and doesn't specify how commands are registered, and CommandCollection "flattens" multiple MultiCommands instead of nesting like Group.group.

The docs occasionally refer to "mulitcommand" instead of "group" in situations where the user would probably understand "group" better. There are very few examples in the docs of either class, and all of them can be reasonably implemented with Group instead.

#347 (and many linked issues) show a confusion over how CommandCollection is supposed to work. It doesn't use each source's parameters, and doesn't invoke the source's callback before a command. In other words, currently the collection only collects the commands in each source, but users expect it to merge the behavior of all sources. I'm not entirely convinced that we should continue to support either of these things. But we could continue to support the current behavior by adding Group.add_source or extend_commands with the same behavior, where the command is looked up first on the group, then on each source. In the future we could also add a merge_group method.

@davidism davidism added this to the 8.2.0 milestone Aug 18, 2023
@davidism davidism self-assigned this Aug 18, 2023
@davidism
Copy link
Member Author

To expand on why I'm not convinced that we should support flattening or merging groups, in general it's because it adds ambiguity and indirection when defining the CLI. Whether you flatten or merge, there's probably a way to get the source's callback called before its command. But there's no convenient way to merge different group's parameters, especially in the face of overlapping or conflicting parameters. It's something that any given project can make a call on, but that Click would have a hard time implementing in a generic and compatible way for all cases.

@davidism
Copy link
Member Author

@janluke @kdeldycke any feedback on this is appreciated

@davidism davidism changed the title deprecate MultiCommand and CommandCollection deprecate MultiCommand and CommandCollection, merge into Group Aug 18, 2023
@davidism
Copy link
Member Author

A search for CommandCollection on SourceGraph shows that it's in use. Some of those uses seem to be unnecessary. Many are for loading plugins that add top-level commands, where a custom Group.get_command would probably serve them better. I'll leave it for now and make it a subclass of Group.

@davidism davidism changed the title deprecate MultiCommand and CommandCollection, merge into Group deprecate MultiCommand, merge into Group Aug 19, 2023
@kdeldycke
Copy link
Contributor

I agree CommandCollection is confusing. I never had any use for that, and when working on Click Extra, I voluntarily ignored it, thinking it was too exotic. My rationale was to focus only on good support for Command and Group as they are the first-class citizen of the Click ecosystem. Then see if my users were asking for CommandCollection. Nobody came for CommandCollection.

So if you can get rid of CommandCollection, I vote for.

Just noticed you also just got rid of MultiCommand, and yes, that's also a good idea!

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 4, 2023
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.

2 participants