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

✨ Show help items in order of definition #944

Merged
merged 5 commits into from
Aug 26, 2024

Conversation

svlandeg
Copy link
Member

@svlandeg svlandeg commented Aug 21, 2024

Fixes #933

Click's Group class specifically orders the items alphabetically:

    def list_commands(self, ctx: Context) -> list[str]:
        """Returns a list of subcommand names in the order they should appear."""
        return sorted(self.commands)

For TyperGroup, we override this behaviour and remove the sorted function. This works, because during creation of the group, the commands are kept in a list as they get declared, so self.commands has the original order.

@svlandeg svlandeg marked this pull request as draft August 21, 2024 16:59
@svlandeg svlandeg added the feature New feature, enhancement or request label Aug 21, 2024
@svlandeg svlandeg self-assigned this Aug 21, 2024
Copy link

"""Returns a list of subcommand names.
Note that in Click's Group class, these are sorted.
In Typer, we wish to maintain the original order of creation (cf Issue #933)"""
return [n for n, c in self.commands.items()]
Copy link
Member Author

@svlandeg svlandeg Aug 21, 2024

Choose a reason for hiding this comment

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

Note that we can't just

return self.commands

here as self.commands is actually a MutableMapping[str, Command] and so this wouldn't pass type checks.

@svlandeg
Copy link
Member Author

test_doc() is failing because of two reasons:

  • With this PR, "hello", "hi" and "bye" should stay in that order -> that part is correct and we should change the test accordingly
  • With this PR, "top" is being documented before "sub". I'm unsure about this particular case, because the definition in multi_app.py is like this:
sub_app = typer.Typer()

@sub_app.command()
def hello(...):
    ...

app = typer.Typer(help="Demo App", epilog="The end")
app.add_typer(sub_app, name="sub")


@app.command()
def top():
...

As such, I'm unsure why "top" is now being shown in the help first.

I'll investigate further and will keep this in draft while doing so.

Copy link

@svlandeg
Copy link
Member Author

svlandeg commented Aug 23, 2024

Ok, so we need to consider how subgroups are treated here versus commands.

Assume the following simplified code:

import typer


app = typer.Typer(help="Demo App")
app.add_typer(typer.Typer(), name="sub")


@app.command()
def top():
    """
    Top command
    """
    typer.echo("top")

Then run

typer example.py run --help

and you'll get

┌─ Commands ──────────────────────────────────────────────────────────────────┐
│ top   Top command                                                           │
│ sub                                                                         │
└─────────────────────────────────────────────────────────────────────────────┘

So even though sub is added to the app first, we still see top on top. On master, sub would be first just because it's alphabetically first.

Now, why is this? Because of the code in get_group_from_info() in main.py, which first processes all registered commands, and adds them to the list of commands, and only in a next loop processes the registered groups and adds those to commands as well:

    commands: Dict[str, click.Command] = {}
    for command_info in group_info.typer_instance.registered_commands:
        command = ...
        if command.name:
            commands[command.name] = command
    for sub_group_info in group_info.typer_instance.registered_groups:
        sub_group = ...
        if sub_group.name:
            commands[sub_group.name] = sub_group

So @tiangolo: are we happy with registered commands taking precedence of registered groups, with this PR? If so, we can have this PR as is and the changes to tests/assets/cli/multiapp-docs-title.md (specifically the top/sub switch) reflect that.

If we want to consider the order in which groups vs commands are declared in the input source (a.k.a. keep "sub" as first in the example above), then we're likely looking at a bigger refactor because we'll have to rethink how group_info stores its data.

@svlandeg svlandeg marked this pull request as ready for review August 23, 2024 09:23
@svlandeg svlandeg assigned tiangolo and unassigned svlandeg Aug 23, 2024

## Sorting of the commands

Note that by design, **Typer** shows the commands in the order they've been declared.
Copy link
Member Author

Choose a reason for hiding this comment

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

Note: if we keep the current behavour of command/group sorting, then we probably want to comment on that in the docs as well, because otherwise it may be confusing. I can add this after we decide how to move forward.

Copy link
Member

@tiangolo tiangolo left a comment

Choose a reason for hiding this comment

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

This looks great! 🚀 🎉

@svlandeg feel free to merge it. 😎

@svlandeg svlandeg merged commit 1f684d4 into fastapi:master Aug 26, 2024
23 checks passed
@svlandeg svlandeg deleted the feat/issue_933 branch August 26, 2024 09:21
@liquidcarbon
Copy link

sorry, does this work? I'm seeing sorted commands.

import typer

print(typer.__version__)

app = typer.Typer(no_args_is_help=True)


@app.command()
def say(what: str):
    """Say it"""
    typer.echo(f"say {what}")


@app.command()
def hi():
    """Say hi to pup"""
    typer.echo("🐶 says: woof!")


if __name__ == "__main__":
    app()

Output:

pixi run python typer_sort.py
0.12.5

 Usage: typer_sort.py [OPTIONS] COMMAND [ARGS]...

╭─ Options ────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╮
│ --install-completion          Install completion for the current shell.                                                                                                          │
│ --show-completion             Show completion for the current shell, to copy it or customize the installation.                                                                   │
│ --help                        Show this message and exit.                                                                                                                        │
╰──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╯
╭─ Commands ───────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╮
│ hi    Say hi to pup                                                                                                                                                              │
│ say   Say it                                                                                                                                                                     │
╰──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╯

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

Successfully merging this pull request may close these issues.

Show help items in order of definition, not alphabetically
3 participants