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

Updated get_group and get_group_from_info to return a Click Group object #2

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

csr
Copy link
Member

@csr csr commented Jun 10, 2020

This PR seeks to solve tiangolo#96 [BUG] typer.main.get_group should return a click.Group by having typer.main.get_group and typer.main.get_group_from_info return a click.Group instead of a click.Command. There may be some issues with this solution - I'll add some comments to the changes using the review tools here.

I tested the code with this script using Typer:

import typer

app = typer.Typer()


@app.callback()
def callback():
    """ My partially-upgraded-to-Typer app """

# insert some fancy new Typer commands here


click_group = typer.main.get_group(app)


@click_group.command()
def some_old_click_function():
      """ I need to rewrite this somday """

All tests pass locally with 100% coverage. Running mypy on this script with mypy main.py returns

Success: no issues found in 1 source file

Once again, this PR is just looking for initial feedback (criticism on code, styling, logic is especially appreciated).

Does Typer have a standard template for PR? Also, we may want to add codecov-io to this repository if it's possible to check for code coverage. I'm also curious as to why the initial tests didn't catch the bug - maybe we could write an additional test with something similar to the script above?

@@ -228,11 +228,11 @@ def get_command(typer_instance: Typer) -> click.Command:
or len(typer_instance.registered_commands) > 1
):
# Create a Group
click_command = get_group(typer_instance)
Copy link
Member Author

Choose a reason for hiding this comment

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

I've renamed from click_command to click_group mainly because otherwise it would return Incompatible types in assignment (expression has type "Command", variable has type "Group"). Seems to be a scope issue?

Copy link
Member

Choose a reason for hiding this comment

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

It's a type error! My understanding is that Group is more specific than Command, and this function returns a Command. So you'd probably want to cast the result of get_group.

Copy link
Member Author

@csr csr Jun 11, 2020

Choose a reason for hiding this comment

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

@juped Thank you.

Do you suggest making so that main.get_group returns a click.Group (I believe this is a requirement for mypy to run successfully)? By looking at the example code in the issue page, the user was calling get_group from their script, so I'm assuming for mypy to run successfully we need main.get_group to return a click.Group?

If yes, then I'm trying to figure out how to cast the result of main.get_group_from_info (which is what main.get_group is calling) from click.Command to click.Group. Is this how you do it in Python:

def get_group(typer_instance: Typer) -> click.Group:
    group = click.Group(get_group_from_info(TyperInfo(typer_instance)))
    return group

Thank you!

Copy link
Member

Choose a reason for hiding this comment

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

This is at the type hinting level, where I believe it's cast().

Copy link
Member Author

@csr csr Jun 11, 2020

Choose a reason for hiding this comment

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

I tried using return cast(click.Group, group) in main.get_group (this requires an import of cast from the typing module), like this:

def get_group(typer_instance: Typer) -> click.Command:
    group = get_group_from_info(TyperInfo(typer_instance))
    return cast(click.Group, group)

However when I run mypy on the sample script in the issue I get

main.py:16: error: "Command" has no attribute "command"
Found 1 error in 1 file (checked 1 source file)

which isn't that helpful as line 16 is just a series of imports. Mmm...

@@ -356,7 +356,7 @@ def get_group_from_info(group_info: TyperInfo) -> click.Command:
convertors,
context_param_name,
) = get_params_convertors_ctx_param_name_from_function(solved_info.callback)
cls = solved_info.cls or click.Group
Copy link
Member Author

Choose a reason for hiding this comment

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

So from the Click docs cls is the the argument class to instantiate (which, from my understanding, that could be a Command or a Group). I believe this change will basically create groups of single commands instead of creating a group or a command depending on what you need. If you need just a command, then it may not make sense to create a group (extra information in memory), but I'm not too sure.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure of this one (don't know enough about the Click internals).

@csr csr requested a review from juped June 11, 2020 10:07
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.

None yet

2 participants