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

Make core extendable by making classes and initializations configurable #938

Merged
merged 5 commits into from Aug 9, 2020

Conversation

txomon
Copy link
Contributor

@txomon txomon commented Feb 23, 2018

I started to create a parallel implementation of Command/Group/etc. to support asyncio (#85) and some other features I need, one of the problems I had is that even if I can instruct cls=MyCommand in some calls, internally the modules use hardcoded classes in many cases.

The idea of the PR is not to support directly asyncio, but to open to the possibility of doing so. Comments are welcome.

@txomon
Copy link
Contributor Author

txomon commented Jul 14, 2018

Is there any way to push this forward?

@davidism
Copy link
Member

Possibly fixes #561

@sirosen
Copy link
Contributor

sirosen commented Jan 22, 2019

I self assigned this a while ago, but never finished writing up feedback. We should address this need, but I'm not sure I'm sold on this particular implementation.

Making a class attribute for this feels clunky, IMO.
I wanted to explore other ways of doing this; potentially seeing if the information can be passed around on the Context or something.

I don't have a clear alternative -- nothing I wrote and tested, anyway -- and maybe this is the best way of doing it. I'd like to try for a cleaner approach though, to see if it's doable.

@davidism
Copy link
Member

Subclassing to override attributes is a common pattern in the Pallets libraries, so the general idea is fine. Some classes can already be passed to decorators, and the context idea is interesting too. So we should take our time to ensure this is all consistent and well documented.

@schollii
Copy link

Is there a list of such classes? Other than the obvious _textwrap.TextWrapper of #1218. Perhaps a list of use cases would be useful in guiding this:

Copy link
Contributor

@sirosen sirosen left a comment

Choose a reason for hiding this comment

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

I'm disappointed that I haven't been able to get time to try other versions/variants of this which I'm interested in exploring. C'est la vie, but sorry all the same.

What I actually care about and what can be done to push this forward:

  1. Should this Comand.command_class be an instance attribute (Command.command_class) rather than a class attribute?
  2. What does usage look like? Can we add some test-cases for this functionality which demonstrate custom Context, custom Command, and custom MultiCommand or Group classes? That would show what the interface is and how to use it.
  3. I'd want narrative docs long-term. But! I think it's a bit much to demand them for an initial version. I'd like to know enough to write such narrative docs myself.
  4. Can this usage be supported by the click.command and click.group decorators? e.g. click.command(cls=MyCustomClass) ?

The basic issue that I'm hung up on is that it seems all of the usage is predicated on creating a class heirarchy which mimics the click classes -- you need CustomCmd(Command), CustomMultiCmd(MultiCommand), and CustomGroup(Group) each of which sets command_class = CustomCmd, right? Or am I mistaken about that?

And you get in MRO-related trouble quickly if you try to setup CustomMultiCmd(CustomCmd, MultiCommand) because you may want to override behavior in CustomCmd which you would like CustomMultiCommand to inherit from MultiCommand.

Could we just make command_class an instance attribute, rather than a class attribute, so that I can have

@click.group('mycmd', command_class=CustomCommandClass)
def cli():
    pass

@cli.command('foocmd', cls=CustomCommandClass)
def foo():
    pass

# by omitting `command_class` from `bar` and `cls` from `baz`, can we sanely allow
# `mycmd foo` to have one behavior, and `mycmd bar baz` to have another?
@cli.group('bargroup', cls=CustomGroupClass)
def bar():
    pass

@bar.command('baz')
def baz():
    pass

?
Where/how/why does that break down?

If the intent is that users modify global state by setting Command.command_class = CustomCmd, I would argue very strongly that that's bad interface/API design. So I don't want that to be the answer, and if that's the current plan, then we should figure out better usage and then what changes need to be made to support it.

A lot of this boils down to "I'm struggling to see what usage looks like."
So some example of usage would go a long way, I think. Ideally in the form of a test case -- and if it can't be put into a test case, please explain why, because that may be a sign of a problem.

click/core.py Outdated
@@ -526,10 +526,11 @@ def invoke(*args, **kwargs):
# It's also possible to invoke another command which might or
# might not have a callback. In that case we also fill
# in defaults and make a new context for this command.
if isinstance(callback, Command):
if isinstance(callback, self.command.command_class or Command):
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that if a user overrides the command class, they should do so with a custom subclass of Command.
That would mean this check can stay isinstance(callback, Command).
Does that sound right?

If so, I'd prefer to use Command directly still for this because I think it's a little bit more readable.

Applies here and to the check below.

@davidism davidism force-pushed the feature/make-extendable branch 2 times, most recently from 8308ac6 to 3abf95c Compare August 8, 2020 18:12
@davidism davidism self-assigned this Aug 8, 2020
@davidism davidism added this to the 8.0.0 milestone Aug 8, 2020
@davidism
Copy link
Member

davidism commented Aug 8, 2020

To answer @sirosen, in general, the idea here is not to add any new behavior, but to make it easier for custom classes to persist through the stack. Right now, @click.command(cls=CustomCommand) can't actually customize a lot of behavior consistently because specific classes are hard-coded rather than using super() or a composable attribute.

As far as I can tell, command_class isn't doing anything here that couldn't be replaced with super(). For now, I'll remove that. Additionally, there are many more places where super() can be used. I'll add those.

@davidism davidism force-pushed the feature/make-extendable branch 2 times, most recently from 338de06 to 8f19a8b Compare August 8, 2020 20:58
@davidism
Copy link
Member

davidism commented Aug 8, 2020

Added a Context.formatter_class attribute that will be used by Context.make_formatter. Except for the discussion below, I don't think there's any other big things to add to this right now.

I'm considering what the Group.command and Group.group decorators should do. group could set cls=type(self) by default, so sub-groups would use the same type. However, I'm not sure how this would affect existing code. For example, Flask has a top-level FlaskGroup, but I don't think it would make sense for it to create sub-groups of that type. Bringing back Group.command_class for use by Group.command might make sense, to have a group with all commands behaving consistently. In that case, we could also add Group.group_class, but that might be a little weird. A custom group class could still customize these by overriding the decorator methods to set the cls argument themselves, it's just a little more manual.

@davidism
Copy link
Member

davidism commented Aug 9, 2020

Decided to add Group.command_class and Group.group_class. group_class can be set to the special value type to indicate that it should use its own type. That way a custom group can continue creating custom groups, without needing to set up a circular reference (group_class = type in the class, instead of CustomGroup.group_class = CustomGroup after the class).

@davidism
Copy link
Member

davidism commented Aug 9, 2020

Added tests and a longer changelog entry.

formatter_class on its own is pretty useless because HelpFormatter doesn't have a good API to override right now.

@davidism davidism merged commit d75ea33 into pallets:master Aug 9, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 13, 2020
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 this pull request may close these issues.

None yet

4 participants