-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Warn about obscured help option #1931
Warn about obscured help option #1931
Conversation
(cherry picked from commit 87db4ba)
…ed-help-option-warnings
…ed-help-option-warnings
A quick reaction. I may not accept this PR, or at least not to "Warn about obscured help option". The zero-config obscuring was added in #1247 and raised in #645. The ability to explicitly suppress the built-in help option was added later in #1325 and perhaps the obscuring is no longer as appropriate, so I'll think about it a bit. Separately, storing the help option configuration in an actual Option is something I considered again while working on #1910. I have not had a deep look yet myself. (I resisted attempting it in the same PR.) |
Eliminates the need for the check in other places. (cherry picked from commit a12f5be)
…ed-help-option-warnings
Motivation: tj#1955
…ed-help-option-warnings
5e6b33b
to
517fc6c
Compare
This reverts commit e8bea4a.
…ed-help-option-warnings
517fc6c
to
aa6e9b3
Compare
I think the commit specific to the title for this PR is d03566b The current behaviour evolved out of a sort of auto-configuration for the implicit help option, which slowly became more robust as doing other work. I suspect the behaviour is more appropriate for simpler programs where the author uses That leads to an interesting thought that could treat explicit and implicit help options differently. But not solving a problem I am invested in for now, and behaviour is as intended. (I have lost track of how many of your PRs have the help configuration refactored into a help option. That is something I am interested in as a concept, but not urgently, and may refer back to this in the future.) |
An extension of #1929 adding warnings about obscured help option as suggested in #1926 (comment).
Additionally proves the usefulness of storing the
Option
instance returned fromcreateOption()
in the_helpOption
property (a change introduced by 9857b1e in #1929), since in the solution, not only the individual flags but also the originally providedflags
value needs to be accessed, and in the future it could be the.name()
, the.attributeName()
or something else pertaining to the help option. It makes no sense to add a property to theCommand
instance each time such a property is needed, it is way better to simply store theOption
instance.Has been merged with the already approved tiny fixes PR #1930 because the parent #1929 has been merged with it.
Problem
States in which the help options is obscured are supported by the library (see
Help.visibleOptions()
and the corresponding unit tests), but definitely not desirable.Solution
Add warnings when such a state is reached similar to those in #1915 and #1917.
ChangeLog
Added
Changed
.createOption()
in.helpOption()
to support custom option flag extractionPeer PRs
…solving similar problems
_registerOption()
, too – watch out for merge conflicts!)Warnings need to be consistent with…
chainArgParserCalls()
for configuration. Additionally await thenable implicit and default option values and thenable default argument values #1915.command()
#1938.parse()
#1940Requires changes when merged with…
.suppressWarnings()
for warnings in #1915 #1931 #1938 #1940 #1955