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

Add support for option groups on cloup.Group #98

Closed
kdeldycke opened this issue Oct 13, 2021 · 8 comments
Closed

Add support for option groups on cloup.Group #98

kdeldycke opened this issue Oct 13, 2021 · 8 comments
Labels
enhancement New feature or request

Comments

@kdeldycke
Copy link
Contributor

This is explicitly mentioned in the documentation as not supported on purpose. But the documentation also ask for feedbacks on potential use-cases. Here is mine.

My mpm CLI has a total of 15 common options shared by 10 sub-commands. To reduce code duplication, I naturally used a cloup.group decorator, and it works great.

Now, To make things easier to apprehend to users, I'd like to group some of these common options by family. I'd like for instance to regroup --manager, --exclude, --all-managers and --xkcd options under the "Package manager selection options" banner. Unfortunately I can't because cloup.group does't support option groups.

Is this a valid case for considering adding this feature? Note that I do not need to apply any constraint (yet?), so implementation can exclude constraints.

@kdeldycke kdeldycke added the enhancement New feature or request label Oct 13, 2021
@janluke
Copy link
Owner

janluke commented Oct 13, 2021

It's very unusual for me to add options to the root group, because:

  • I tend to avoid conflicting or meaningless configurations (e.g. passing an option that won't be actually used) so I add a "Group option" only if I'm sure that all present and future subcommands will share such option and this is very rarely the case; good examples are --verbosity and --config
  • I like to type all parameters after command names.

What I usually end up to do is defining a class Params where I put shared parameters as class attributes, sometimes using functools.partial if I want to reuse the same parameter with slightly different args in different commands. Not saying it's a better approach, actually yours is the cleaner one probably (but see point 2 in the above list :-)).

Anyway, I generally don't like too much libraries that tell me what I should or not do, so feel free to open a PR. Nonetheless, note that it's very easy to work around the limitation (which is why I took the liberty of not allowing option groups by default):

from cloup import ConstraintMixin, OptionGroupMixin, Group

class MyGroup(OptionGroupMixin, Group):
    pass

@cloup.group(cls=MyGroup)
def cmd:
    ...

If we include OptionGroupMixin, I don't see any reason not to include ConstraintMixin as well. Probably we can add both to BaseCommand directly.

@kdeldycke
Copy link
Contributor Author

kdeldycke commented Oct 14, 2021

The workaround you proposed indeed does the trick. Thanks for providing a solution! 👍

For the record, this experiments led to the discovery of a bug with command sections that has been filled at #99.

@janluke
Copy link
Owner

janluke commented Oct 14, 2021

Yes, please. This is definitely a bug. I'll fix it later in the day.

@kdeldycke
Copy link
Contributor Author

Yes, please. This is definitely a bug. I'll fix it later in the day.

Just moved the bug into its own issue at #99.

@kdeldycke
Copy link
Contributor Author

Now back to the main topic, a.k.a. option groups on Group.

When I started using Click several years ago, I was taken aback by its way of sharing options in a group. As you pointed out in your second argument, the CLI [options] <subcommand> [more options] pattern is quite rigid and frustrating for users. How may time did I tried to use the CLI <subcommand> [all_my_options] form...

I didn't found an elegant way to allow for arbitrary positions of commands and options, so I followed Click's recommandation called it a day.

But maybe we can revisit this limitation and work around it.

@janluke
Copy link
Owner

janluke commented Oct 15, 2021

I can't see an easy way. If you want to type options after subcommands, the easiest workaround is what I explained above: reusing options when it's needed. And thinking again about it, I actually prefer this solution to Group parameters:

  • I don't need to grab parameters from a dictionary, ctx.parent.params, they are in the function signature
  • all options I can pass when using a subcommand are visible in the help of a subcommand.
from functools import partial

class Params:
    shared = option('--shared', ...)
    common = partial(option, '--common', default=1, ...) 

@command()
@Params.shared
@Params.common(required=True)
def foo(shared, common):
    ...

@command()
@Params.shared
@Params.common()
def bar(shared, common):
    ...

Of course this sharing can be extended to Cloup's option groups.

@janluke
Copy link
Owner

janluke commented May 8, 2022

@janluke janluke closed this as completed May 8, 2022
@janluke janluke removed this from the v0.14.0 milestone May 8, 2022
@kdeldycke
Copy link
Contributor Author

Thanks @janluke for adding this feature as part of #112 !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants