-
-
Notifications
You must be signed in to change notification settings - Fork 805
🐛 Make second column of Rich help output reflect the type consistently, even when using metavar
#1410
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
base: master
Are you sure you want to change the base?
Conversation
|
|
||
| result = runner.invoke(app, ["--help"]) | ||
| # assert "Usage: main [OPTIONS] ARG1 ARG3 [ARG4] [meta7] ARG8 arg9" in result.stdout | ||
| assert "Usage: main [OPTIONS] ARG1 ARG3 [ARG4] meta7 ARG8 arg9" in result.stdout |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📝 Docs previewLast commit 987eaaf at: https://49bcbb69.typertiangolo.pages.dev |
| if isinstance(param, click.Option): | ||
| metavar_type = metavar_str |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that this part is crucial to ensure the current Enum tests won't fail. This is the part that will replace Choice with something like [simple|conv|lstm]
| elif metavar_name: # pragma: no cover | ||
| secondary_opt_short_strs.append(metavar_name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not too sure yet about these secondary_opts, probably requires another test (instead of having pragma cover).
Ok this is a bit of a tricky PR, so I'll dump train of thought here in detail. [disclaimer: 0% vibe-coded, comment nor code]
Fixes #1156 .
Current master behaviour
First let's look at test_tutorial/test_arguments/test_help/test_tutorial006.py, corresponding to the section in the docs about metavar, python source here.
Without
richmarkdown onmaster, this looks like:With
richonmaster:What we see here, is that the original
"name"still shows up in the very first column of the output formatting, with themetavarname only in the second column, which is currently onmasterconstrued as the "metavar" column.When we look at another non-metavar example, e.g. tutorial005 we see something similarly weird on
master:i.e. the var's proper formatting
[NAME]only shows up in the second metavar column.Let's look at a much more extensive example, taken from @Noxitu in #438 and in adapted form added to this PR as
test_rich_help_metavarintest_rich_utils.py. It gives as output:What we see here is that types and metavar information is freely mixed in that "metavar" column when Rich is used to print the help.
Compare this to the non-Rich output on
master:This PR
So, what this PR proposes to do is:
metavarname if set, as the non-Rich formatting doesThis required some edits to the tests:
✨user✨instead of✨username✨so the tests can more easily double check that the original name of the parameter, being"name", is not present anymore in the outputtest_tutorial006_rich.pyandtest_tutorial006_an_rich.pythat double check the same behaviour with or without Rich formatting - both tests fail onmaster.test_rich_help_metavarintest_rich_utils.py.Results with this PR:
This now shows
[NAME]properly, andTEXTas type in the second column.No more "name" visible in the output.
And finally:
Which feels a lot more consistent and nice.
Details of the actual fix
Which brings us to the actual fix in
rich_utils.py.The old L375 had code specifically designed for a "metavar column", but as we see in the examples, this column was filled with different types of data. I've found out that in fact,
Argument's store an alternative name in theirmetavardata, whileOptionobjects may store alternative represenations of their types. This function is not directly used in Typer, but becomes apparent forEnumcases where we want to display something like[simple|conv|lstm]instead ofChoice. For this reason, the new code inrich_utils.pyputs themetavar_strdata in a different column, depending on the object instance check. As before, we don't explicitely displayBOOLEANvalues.Breaking behaviour
While I consider this to be a bug fix, it's also breaking behaviour and may impact users. One potential way to minimize the impact is to keep the capitalization of argument names in Rich help formatting the same as before (i.e. lowercased) even though this is inconsistent with non-Rich formatting (where they are upper-cased, which is also what the docs state).