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

🐛 Ensure that options_metavar is passed through correctly #816

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

gar1t
Copy link

@gar1t gar1t commented Apr 30, 2024

Typer was ignoring this setting when provided to commands. Refer to added test for details.

Typer was ignoring this setting when provided to commands. Refer to
added test for details.
gar1t added a commit to gageml/gage that referenced this pull request Apr 30, 2024
@svlandeg svlandeg added bug Something isn't working p2 labels May 2, 2024
Copy link
Collaborator

@svlandeg svlandeg left a comment

Choose a reason for hiding this comment

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

Thanks!

I have yet to check the test case, but a few quick first comments:

typer/main.py Outdated Show resolved Hide resolved
typer/main.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@svlandeg svlandeg left a comment

Choose a reason for hiding this comment

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

I double checked the unit test and found that the second option (c2) is already working correctly on master, while the first one (c1) fails. Nevertheless, it's good to have both options checked here.

One thing I wonder about though. If you change "OPTIONS" to "OPTS" or something different entirely like "STUFF", the box with options in the help menu would still be named "Options".

$ python script.py --help

 Usage: script.py [STUFF]

┌─ 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.                                                                                                                                                                                                                                       │
└─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┘

Is this something that should be addressed as well?

typer/main.py Outdated Show resolved Hide resolved
typer/main.py Outdated Show resolved Hide resolved
@gar1t
Copy link
Author

gar1t commented May 17, 2024

One thing I wonder about though. If you change "OPTIONS" to "OPTS" or something different entirely like "STUFF", the box with options in the help menu would still be named "Options".

$ python script.py --help

 Usage: script.py [STUFF]

┌─ 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.                                                                                                                                                                                                                                       │
└─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┘

Is this something that should be addressed as well?

I think that's a good idea!

But the goal of this PR is to fix a bug that currently prevents options_metavar from propagating to sub-commands. I believe the current PR accomplishes that and should be merged and closed.

I would classify your point here as an enhancement to the API to support configuration of the panel label/title. This could be captured as a new feature request, while merging this PR.

@svlandeg svlandeg changed the title Fix use of options_metavar 🐛 Ensure that options_metavar is passed through correctly May 21, 2024
@svlandeg
Copy link
Collaborator

But the goal of this PR is to fix a bug that currently prevents options_metavar from propagating to sub-commands. I believe the current PR accomplishes that and should be merged and closed.

I would classify your point here as an enhancement to the API to support configuration of the panel label/title. This could be captured as a new feature request, while merging this PR.

Agreed that we can address this as future work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working p2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants