-
Notifications
You must be signed in to change notification settings - Fork 217
Add new options for parsing: ignore and skip #379
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
Conversation
Simpler variation of apache#378
garydgregory
left a comment
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.
Hi @cstamas
Thank you for the PR 😄
- You'll need to add at least one unit test to validate the behavior of the new method. Otherwise, a future change might blow up the expected use case.
- The now protected method needs a Javadoc
@sincetag.
|
Thanks for checking it out. Pushed tests and some more changes. |
when there was one boolean doing this or that only
|
I'll review/merge and so on in a few hours... |
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.
Hi @cstamas
I'm somewhat confused - unsurprising 😉 - as to what some combinations mean.
I think it would help users to document what happens in the new matrix:
| stopAtNonOption | throwAtNonOption | Means |
|---|---|---|
| true | true | ? |
| true | false | ? |
| false | true | ? |
| false | false | ? |
The current docs don't say what overrides what if anything.
TY!
As not all combos make sense.
I got too, as IMHO not all combos of booleans made sense, so I switched to enum. |
| * @throws ParseException if there are any problems encountered while parsing the command line tokens. | ||
| * @since 1.10.0 | ||
| */ | ||
| public CommandLine parse(final Options options, final String[] arguments, final Properties properties, final NonOptionAction nonOptionAction) |
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.
If we add an API, this could be an opportunity to simplify call sites to use variable arguments:
public CommandLine parse(final Options options, final Properties properties, final NonOptionAction nonOptionAction, final String... arguments)
WDYT?
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.
Sounds reasonable.
garydgregory
left a comment
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.
Hi @cstamas
Thank you for your update :-)
I don't see tests for the enum value STOP but we see new tests that use the other enum values.
TY
The new |
|
TY @cstamas, merged! |
|
When do you need a release? Is there anything else you'd like to contribute before that? TY! |
Goal is to introduce new "ops" for CLI parser: a mode when unknown options are completely ignored or just skipped (added to args). At the same time encapsulation is loosened as well, by making relevant methods
protectedto allow possible override/extension ofDefaultParserclass.Simpler variation of #378
Goal: to "postpone" full argument parsing. To be used in Maven. We introduced new "cli commands" aside of existing
mvn, likemvnenc,mvnshandmvnup.The "base" options (all CLI command) are these:
https://github.com/apache/maven/blob/77ebd14d1580e0a67c5a929afdd3ac62a3bfea1f/api/maven-api-cli/src/main/java/org/apache/maven/api/cli/Options.java
And each command extends this base with specific options, for example:
https://github.com/apache/maven/blob/77ebd14d1580e0a67c5a929afdd3ac62a3bfea1f/api/maven-api-cli/src/main/java/org/apache/maven/api/cli/mvn/MavenOptions.java
https://github.com/apache/maven/blob/77ebd14d1580e0a67c5a929afdd3ac62a3bfea1f/api/maven-api-cli/src/main/java/org/apache/maven/api/cli/mvnenc/EncryptOptions.java
https://github.com/apache/maven/blob/77ebd14d1580e0a67c5a929afdd3ac62a3bfea1f/api/maven-api-cli/src/main/java/org/apache/maven/api/cli/mvnup/UpgradeOptions.java
Now the goal would be to move these commands (currently "burned into Maven core") into dynamically resolved extensions. Original idea is to "boot Maven DI base", figure out which command extension is wanted, and then once given "tool" loaded, have it parse the "remainder" specific arguments (that are unknown and start, the options depend on the tool being loaded).
tl;dr: we want to be able to parse "common" base set of options, that are same across multiple commands, then load command dynamically (Maven would resolve and load it into DI like any other extension), and then let the loaded command parse the "remainder" (own specific subset) or arguments.
tl;dr more short: to be able to "chain" arg parsing from "generic" to more "specific" (and unknown which specific at start).