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

[#2104] Usage shows alias used rather than command name #2105

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

Conversation

roxspring
Copy link

First attempt at a fix for #2104

It may not be easy to build bookkeeping to keep track of this. Such bookkeeping also needs to handle scenarios like nested sub-subcommands and repeatable subcommands.

Looking at repeatable subcommands gave me the inspiration for this PR - for repeatable subcommands there seems to be some copying of CommandLine and so this PR takes a similar approach: When a subcommand alias is used, the CommandLine is copied with name and alias swapped.

I imagine the need for additional CommandLine instances here might be seen as a hack. And I suspect there's potentially some mileage in adding a parsedName field rather swapping alias/name. But I figure this was worth getting feedback on.

Secondly, the display part. Currently help is static: it's not designed to change depending on user input (only the error message is, but that isn't part of the usage help message).

There's no parser state available to the logic that constructs the usage help message. I imagine we would have to use a static variable (or something similar) to store the user-specified command name/alias, to make that alias available to the logic that constructs the usage help message. If you can think of a better solution, ideas are welcome!

Yeah, I was surprised the CommandLine doesn't retain access to the ParseResult. I've tried to avoid resorting to static stuff for now, but can see that could be necessary.

Finally, the help is designed to be highly customizable. If applications have tests that assert the exact help message, then these tests may break, that's unavoidable.

But other than that, any changes we introduce here should not impact other applications who have customized their help message, those customizations should continue to work as before.

Presumably the PR could cause some side effects here with the CommandLine name having changed. I guess the new behaviour could be hidden behind some new optional flag. Although I'm not sure where best that option would fit off the top of my head.

@remkop remkop marked this pull request as ready for review September 4, 2023 18:26
Copy link
Owner

@remkop remkop left a comment

Choose a reason for hiding this comment

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

This approach (copying the CommandLine instance) is going to require a lot of additional unit tests to ensure that no regressions are introduced.

As the TODO in the current code indicates, I have looked at doing something similar for another ticket (you may be able to use git blame to see what ticket, I'm on my phone now so not easy to check...)
Last time I wasn't able to get everything to work perfectly but since the use case was limited (programmatic API doesn't always work correctly, but applications using the annotation API aren't impacted), it was acceptable.

If we're extending this mechanism for all use cases we must be very careful about regressions, which means unit tests that verify that the object after the copy still has all the attributes.

@remkop remkop added type: enhancement ✨ theme: usagehelp An issue or change related to the usage help message labels Sep 12, 2023
@remkop remkop linked an issue Sep 12, 2023 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
theme: usagehelp An issue or change related to the usage help message type: enhancement ✨
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Displaying command alias in help/usage
2 participants