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

Fix #600 - better handling of --help and --version options #608

Open
wants to merge 15 commits into
base: develop
Choose a base branch
from

Conversation

rmunn
Copy link
Contributor

@rmunn rmunn commented Mar 27, 2020

This PR builds on #607 to fix #600. Now that string values are handled by the tokenizer exactly how getopt handles them, we can also handle the special --, --help and --version options exactly how getopt handles them. In particular, we can respond to --help no matter where it appears in the argument list (the previous behavior would only respond to --help if it was the first parameter, and had some hacks that tried to ensure that it would be responded to elsewhere, but those hacks were incomplete and buggy, creating issues like #596 among others).

Note that because I wrote this PR on top of #607, merging this PR will automatically merge #607 as well. I did it this way because proper tokenization of --help (and NOT handling it if it was a string value to some other option) is essential to how this PR is supposed to work. Without #607, many of the tests I wrote for the AutoHelp and AutoVersion options would fail.

tydunkel and others added 5 commits March 13, 2020 16:20
This will let us parse -vvv and turn that into "Verbose=3".
Most tests pass; a couple of tests were either not needed, or were
testing semantics that change with the new implementation (e.g., spaces
in option values are allowed now).
Pattern matching is a particularly good fit here, as each case is
data-driven and the flow is easy to read top-to-bottom.
The short and long option code can move into separate functions, which
makes the big switch statement much easier to read.

Also remove one TODO comment that's not relevant to the current feature.
@rmunn
Copy link
Contributor Author

rmunn commented Mar 27, 2020

And because #607 is built on top of #594, merging this PR will merge not only #607 but #594 as well.

@rmunn
Copy link
Contributor Author

rmunn commented Apr 27, 2020

Now that I've incorporated #610 into #607, I'll be rebasing this PR on top of the new version of #607. Please do not merge until I've done that.

@rmunn rmunn force-pushed the feature/autohelp-rewrite branch from 8e3e1e8 to 33fc02f Compare May 4, 2020 09:13
@rmunn
Copy link
Contributor Author

rmunn commented May 4, 2020

I've rebased this PR on top of the new version of #607. It's ready to merge now.

rmunn added 2 commits June 1, 2020 17:15
Double dash (--) should end option sequences, but should have no effect
on value sequences. These unit tests reflect that design.
Now things like -vvv for triple-verbose can be done in user code.
@rmunn
Copy link
Contributor Author

rmunn commented Jun 2, 2020

Rebasing again since #607 has had some changes, in particular it now has a working implementation (and a unit test) for the FlagCounter attribute added to Options.

rmunn added 7 commits June 2, 2020 14:09
Next we'll implement the feature.
These default to false and determine whether the automatically-added
"--help" and "--version" parameters will have "-h" and "-V" shortnames.
The shortnames, like the longnames, are NOT configurable.
The --help option should be recognized anywhere it appears in the
command line, according to the GNU getopt standard, and if it appears,
it should override any other processing of arguments -- which includes
returning no other parsing errors. Ditto for --version.
The tests were already written, but the AutoHelpShortName and
AutoVersionShortName parts were commented out because otherwise the
tests wouldn't compile. Now the tests compile, and pass.
Ensure that UnknownOptionError is still generated for --help option when
AutoHelp is off.
Ensure that UnknownOptionError is still generated for --version
option when AutoVersion is off.
@rmunn
Copy link
Contributor Author

rmunn commented Aug 19, 2020

Replaced by #686, which separates out the --help and --version handling into its own single PR instead of having a mixed PR like this one. Closing this PR in favor of #686.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants