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 Typer #26

Merged
merged 18 commits into from
Mar 28, 2022
Merged

Add support for Typer #26

merged 18 commits into from
Mar 28, 2022

Conversation

browniebroke
Copy link
Contributor

Fix #14

Many thanks to @wasi-master for the original draft

@paw-lu
Copy link

paw-lu commented Mar 3, 2022

Thanks for this!

As this is, this change will break Typer's env var documentation.

Currently, if a Typer Option has envvar set.

typer.Option(
    False,
    "--bar",
    help="Lorep ipsum.",
    envvar="BAR",
)

Then it will automatically document it.

% foo --help

Options:
--bar            Lorep ipsum.  [envvar: BAR]

Right now though, this change removes the automatic envvar documentation:

% foo --help
╭─ Options ────────────────────────────────────────────────────────╮
│     --bar                                     Lorep ipsum.       │
│                                                                  │

@paw-lu
Copy link

paw-lu commented Mar 3, 2022

click-contrib/click-help-colors is similar to this library, but preserves the envvar documentation when used with Typer (it's what I'm currently using on a Typer project). So it might be worth looking into the differences between what that and rich-click are doing.

@ewels
Copy link
Owner

ewels commented Mar 3, 2022

I think that the envvar thing is in click, so probably not related to the Typer work at all. Sounds like something I've missed (click does a tonne of stuff!) https://click.palletsprojects.com/en/8.0.x/options/#values-from-environment-variables

Made #36 to track this..

@paw-lu
Copy link

paw-lu commented Mar 3, 2022

I think that the envvar thing is in click

Yeah, to be clear you're right—it does come from Click. Think the difference is that it's documented under --help by default in Typer, while for Click it is hidden by default (I think!).

Made #36 to track this..

Thanks!

Copy link
Owner

@ewels ewels left a comment

Choose a reason for hiding this comment

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

Hi all,

I've just had another read over the code and I think it looks great - love how simple it is 👌🏻

I was partly waiting to see if there would be imminent movement on the core Typer library to include support. Sounds like it might happen, but I figure that there's no problem in having both and we can always strip this out again at a later date if it becomes redundant.

Just a couple of minor readme suggestions for additional clarity about installation requirements, then the crash that I'm getting when I run the example script.

Phil

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
browniebroke and others added 2 commits March 7, 2022 11:51
@browniebroke
Copy link
Contributor Author

then the crash that I'm getting when I run the example script.

If I remember well, I was getting the same crash with some of the Click examples. I assumed it was my environment that was messed up.

Copy link
Owner

@ewels ewels left a comment

Choose a reason for hiding this comment

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

I replicated the error on another computer, so not sure that it's just my environment (though the setups are similar so there could be something odd).

After a bit of trial and error testing out examples from the Typer docs (which worked), I came up with the three suggested changes here which make the example script work for me.

Phil

examples/08_typer.py Outdated Show resolved Hide resolved
examples/08_typer.py Outdated Show resolved Hide resolved
examples/08_typer.py Outdated Show resolved Hide resolved
@ewels
Copy link
Owner

ewels commented Mar 7, 2022

Whilst going through the Typer docs (which I'm unfamiliar with, I've not used Typer for a project before), I noticed that the basic examples don't get any rich-click functionality. For example, the simplest example:

import rich_click.typer as typer

def main():
    typer.echo("Hello World")

if __name__ == "__main__":
    typer.run(main)

Gives no rich output:

$ python test_typer.py  --help
Usage: test_typer.py [OPTIONS]

Options:
  --install-completion [bash|zsh|fish|powershell|pwsh]
                                  Install completion for the specified shell.
  --show-completion [bash|zsh|fish|powershell|pwsh]
                                  Show completion for the specified shell, to
                                  copy it or customize the installation.
  --help                          Show this message and exit.

The rich-click functionality only seems to click in when using the decorator functionality:

app = typer.Typer()
@app.command()
def cli():  #...

I don't know enough to speculate why this is, or how we could support the other usage method. However, we should probably mention this in the readme at least to prevent confusion.

Phil

@browniebroke
Copy link
Contributor Author

Thanks a lot for the feedback and the comments. I don't have time to look into it now but I think we can make the basic examples work.

I'll try to find some time in the coming week 👍

@ewels
Copy link
Owner

ewels commented Mar 8, 2022

Sounds good, thanks! Happy to merge this PR now and add the additional support for the basic examples in a later PR if you prefer, just let me know.. (also re: my suggested example code edits above)

Phil

@browniebroke
Copy link
Contributor Author

browniebroke commented Mar 10, 2022

I've updated the example, it should now work. On top of your suggestions, I've also:

  • renamed the file to not have 2 examples/08_...
  • changed the decorator of cli from @app.command() to @app.callback(): without that, the cli was detected as a subcommand rather than the help of the CLI itself. It also enables the top level --debug option to work, similar to what is done in the 01_simple.py example

@browniebroke
Copy link
Contributor Author

I'm thinking to move the typer example in a dedicated directory, which would enable adding more than 1, thoughts?

Comment on lines 29 to 33
def run(function: Callable[..., Any]) -> Any:
"""Redefine typer.run() to use our custom Typer class."""
app = Typer()
app.command()(function)
app()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's copied from typer itself:

https://github.com/tiangolo/typer/blob/6d8e954664d433fb8beb63cd1329f21554ea7b36/typer/main.py#L861-L864

I don't see a better way to inject our Typer class, open to suggestions.

@ewels
Copy link
Owner

ewels commented Mar 10, 2022

I'm thinking to move the typer example in a dedicated directory, which would enable adding more than 1, thoughts?

Yes, I was thinking the same - have directories for click and typer. Maybe can drop the numeric prefixes too, as they're getting annoying. It made sense when there were about 3 as it helped walk through them logically, but as their number grows it's a hassle.

They're sort of doubling as crap tests at the moment, so hopefully once we get proper unit testing in place they might not proliferate quite so quickly! 😅

@ewels
Copy link
Owner

ewels commented Mar 16, 2022

Sorry for all of the commit spam with the new CI precommit tests. They've been passing locally for me which made it a bit tricky to work with. Anyway, they were getting annoying for the examples so I've just set flake8 and mypy to ignore that folder. Now it's passing so I think we're good..

@browniebroke
Copy link
Contributor Author

No worries, your updates look good to me 👍thanks for tyding it up!

Copy link
Owner

@ewels ewels left a comment

Choose a reason for hiding this comment

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

Thanks again for this! Let's go for it and brace for the issues that start coming in once it's supported 😅

@ewels ewels merged commit 56ac360 into ewels:main Mar 28, 2022
@browniebroke browniebroke deleted the feat/typer-support branch March 28, 2022 21:36
@browniebroke
Copy link
Contributor Author

Interesting timing... FYI A new version of click was released today and it broke Typer: fastapi/typer#375 (in case you get one of these issus)

@ewels
Copy link
Owner

ewels commented Mar 28, 2022

hah - good to know, thanks!

@tiangolo
Copy link

Hey, hey! I just released Typer 0.4.1 handling the incompatibility with Click 8.1.0. 🚀 🤓

@ewels
Copy link
Owner

ewels commented May 16, 2022

Hi all - I just added support for click envvar (which also gives support for Typer envvar). Thanks for bringing this to my attention @paw-lu! See #36 for details (but it should "just work").

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.

Add support for Typer
4 participants