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

Import as #10

Merged
merged 12 commits into from
Feb 17, 2022
Merged

Import as #10

merged 12 commits into from
Feb 17, 2022

Conversation

ewels
Copy link
Owner

@ewels ewels commented Feb 13, 2022

Start playing with the import rich_click as click concept suggested by @willmcgugan in #8

@ewels ewels marked this pull request as draft February 13, 2022 23:02
@ewels ewels added the WIP Work in progress label Feb 13, 2022
@ewels
Copy link
Owner Author

ewels commented Feb 13, 2022

Hmm, did a speed back-of-the-car attempt just now @willmcgugan (I'm away travelling this weekend 😅) but no dice. Any ideas?

@willmcgugan
Copy link
Collaborator

That looks like it should work. Were you getting any errors?

@ewels
Copy link
Owner Author

ewels commented Feb 15, 2022

No errors, but just default click behaviour - no customisation.

I need to import click but I guess the custom rich-click imports aren't overwriting those classes for some reason? I guess I could sort of do monkey-patching in this file to make it work 🤔

@ewels
Copy link
Owner Author

ewels commented Feb 15, 2022

eg. Running the minimal example, just get the default click output:

$ python examples/03_simple_importas.py
Usage: 03_simple_importas.py [OPTIONS] COMMAND [ARGS]...

  My amazing tool does all the things.

  This is a minimal example based on documentation from the 'click' package.

  You can try using --help at the top level and also for specific group
  subcommands.

Options:
  -d, --debug / -n, --no-debug  Enable debug mode
  --help                        Show this message and exit.

Commands:
  download  Pretend to download some files from somewhere
  sync      Synchronise all your files between two places

I've tried a few things by trial and error, but I can't get the rich-click objects to be used with this method. I think I need a Python wizard with more skills than I have.. 👀

@ewels
Copy link
Owner Author

ewels commented Feb 15, 2022

Ok I can get it to work if do this in the __init__.py:

from click import *
from .rich_click import rich_format_help

Group.format_help = rich_format_help
Command.format_help = rich_format_help

Then the import rich_click as click works nicely. Definitely an improvement for the end-user (less typing, less to get wrong), but it's still monkey patching - just in a different place. Which is kind of what we were trying to avoid with this...

I'll leave this open a little longer in case anyone has any ideas why the sequential import doesn't overwrite the class names in the way I expect, but if not I'll merge with this change.

@ewels ewels mentioned this pull request Feb 15, 2022
@grst
Copy link

grst commented Feb 16, 2022

I don't think the current approach can work, as click never get's to know that there's a new Group and Command class in rich_click.

You could try something along the lines of:

__init__.py:

from click import * 
from .rich_click import rich_format_help
from .rich_click import Group
from .rich_click import Command

def group(*args, cls=Group, **kwargs):
    from click import group as group_
    return group_(*args, cls=cls, **kwargs)

@ewels
Copy link
Owner Author

ewels commented Feb 16, 2022

Right, so because we're not overwriting the group() function it's still using the default click Group class. Gotcha 👌🏻

I don't really know if this is any better than the monkey patching approach 🤔 - seems sort of the same? We are again overwriting things in the click namespace, either way.

@grst
Copy link

grst commented Feb 16, 2022

We are again overwriting things in the click namespace, either way.

I wouldn't say so. My example only modifies the rich_click namespace.
So if a user did

import click as cl
import rich_click as rcl

they could still use

@cl.command()
def cli():
   pass

@rcl.command()
def cli2():
   pass

and one would be rich-click formatted and the other not. Whereas with monkey-patching, both would be rich-click formatted.

@ewels
Copy link
Owner Author

ewels commented Feb 16, 2022

Whereas with monkey-patching, both would be rich-click formatted.

Is that true even when we're doing the monkey patching within the rich-click package? As in #10 (comment) ?

@ewels
Copy link
Owner Author

ewels commented Feb 16, 2022

Damn, just tried it and you're right. Even when I do Command.format_help = rich_format_help within the rich_click/__init__.py it affects the class with the vanilla import click as cl import 😞

@ewels
Copy link
Owner Author

ewels commented Feb 16, 2022

Ok, so I had a stab at implementing your suggestion in edb2156 @grst

It seems to mostly work - at least, the top-level stuff. So if you have one command or if you do the top-level group it formats it. However, if you try to get the help for a command within a group it uses the default click handling still.

Also need to figure out how to do this approach for printing error messages. That doesn't use decorator functions, so I'm less clear on how to get that to work.

@grst
Copy link

grst commented Feb 16, 2022

However, if you try to get the help for a command within a group it uses the default click handling still.

There's a command_class attribute, it seems:
https:/pallets/click/blob/49164faca678dd476f1b11a2584fd0e1c6be70b2/src/click/core.py#L1757-L1762

Maybe it's possible to set it as keyword argument:

return rich_group(*args, cls=cls, command_class=cls, **kwargs)

Otherwise it should be possible to override it in the Group class.

@ewels
Copy link
Owner Author

ewels commented Feb 16, 2022

Maybe it's possible to set it as keyword argument:

Nope: TypeError: __init__() got an unexpected keyword argument 'command_class'

Otherwise it should be possible to override it in the Group class.

Yup, this seems to work! I guess it makes things less customisable with custom classes etc, but then if you want to do that stuff you probably won't be using this package.

@ewels
Copy link
Owner Author

ewels commented Feb 16, 2022

ok awesome, just need to figure out how to handle the help now (ClickException.show). Need to run now..

@grst
Copy link

grst commented Feb 16, 2022

I guess it makes things less customisable with custom classes etc

I don't think it would prevent you from overriding the class in cli.comman(cls=...) anyway. It just sets another default.

@ewels
Copy link
Owner Author

ewels commented Feb 16, 2022

I'm not sure that we're going to be able to get at the error message printing code any way other than monkey patching the exception classes.. :/

Code is nestled in the main() function:

except ClickException as e:
    if not standalone_mode:
        raise
    e.show()
    sys.exit(e.exit_code)

I don't really fancy trying to replicate the massive main() function so I'm not sure that there's really any other way.. Unless anyone else has any smart ideas? 👀

@grst
Copy link

grst commented Feb 17, 2022

Looks tough! I can't think of any better way either.

@ewels
Copy link
Owner Author

ewels commented Feb 17, 2022

Figured it out 💥 4384217

Click hops about all over the place but managed to track back to the main() function call and override standalone_mode so that it allows the exceptions to raise. Then we can catch them with our custom code and handle the output nicely 🚀

@ewels ewels marked this pull request as ready for review February 17, 2022 15:26
@ewels ewels removed the WIP Work in progress label Feb 17, 2022
@ewels ewels merged commit d0eefad into main Feb 17, 2022
@ewels ewels deleted the import-as branch February 17, 2022 15:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants