-
Notifications
You must be signed in to change notification settings - Fork 418
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
Create new feature parameterAllowedBeforeEndOfOptions #2115
base: main
Are you sure you want to change the base?
Conversation
…cting positional parameters until after EndOfOptions
Are you waiting on something from me? |
Apologies. Family circumstances prevent me from working on my open source projects at the moment. |
I am trying to decide if I should fork the project or wait. Do you have an ETA for your return to working on this project? |
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.
Sorry for the long delay. My mother passed away and I was not in the mood to work on picocli for a long time.
I have added some individual feedback to the commit changes:
- please add
@since 4.8.0
to the new public API javadoc - please add some tests, details are below
- please update the user manual documentation. I propose we add a new section before 11.10. Toggle Boolean Flags
/** | ||
* Enhancement from issue 2103 enables or disables positional parameters before the EndOfOptions delimiter (such as "--"). | ||
*/ | ||
public class Issue2103 { |
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.
Please also add tests to verify that
- the CommandLine.setParameterAllowedBeforeEndOfOptions method returns
true
by default - calling setParameterAllowedBeforeEndOfOptions also impacts the existing subcommands
- when subcommands are added after setParameterAllowedBeforeEndOfOptions is called, these added subcommands have the default value.
Please also add tests for the public ParserSpec
parameterAllowedBeforeEndOfOptions
getter and setter methods.
There may also be existing tests that need to be fixed since the output of ParserSpec::toString
looks different now (which is good, by the way).
see https://github.com/remkop/picocli/blob/main/src/test/java/picocli/SubcommandTests.java#L776
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.
These two failing tests are probably caused by the change to ParserSpec::toString
:
picocli.TracerTest > testTracingDebugWithSubCommands FAILED
org.junit.ComparisonFailure at TracerTest.java:375
picocli.TracerTest > testDebugOutputForDoubleDashSeparatesPositionalParameters FAILED
org.junit.ComparisonFailure at TracerTest.java:118
@@ -13932,6 +13964,10 @@ private void processPositionalParameter(Collection<ArgSpec> required, Set<ArgSpe | |||
if (tracer.isDebug()) { | |||
tracer.debug("Parser is configured to treat all unmatched options as positional parameter", arg);} | |||
} | |||
if (!endOfOptions && !commandSpec.parser().parameterAllowedBeforeEndOfOptions()) { | |||
handleUnmatchedArgument(args); |
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.
What error is shown to the end user when they violate this restriction?
Since the parser knows what went wrong, it should provide a hint to the user as to how they can fix the issue.
We should also have a test that verifies the error message.
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.
Currently, it is Unmatched argument at index 5: 'a'
where the index is the position of the "bad" input and 'a' is the value of the bad input. I assume that you would prefer a new message, perhaps like Unmatched argument at index 5: 'a'. Positional parameters must follow the "{EndOfOptions Character, like --}"
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.
Yes please!
How about:
Unmatched argument at index 5: 'a'. Positional parameters must follow the EndOfOptions delimiter '--'.
(The delimiter string is configurable, so the code that generates this error message should use get EndOfOptionsDelimiter
instead of hardcoding the default --
)
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.
This change is a bit tricky due to the existing code flow. Please take a close look at this part of the change before approving.
/** Returns whether positional parameters on the command line are allowed to occur before the special End of Options delimiter. | ||
* The default is {@code true}. | ||
* @return {@code true} positional parameters may occur anywhere on the command line, {@code false} if they must follow End of Options. | ||
*/ |
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.
Can you add @since 4.8.0
to the javadoc for all public methods that were added in this pull request?
(Picocli follows semantic versioning, so additional API means the next version number should be 4.8.0, not 4.7.6.)
Just a side note, I think these files might need to be cleaned up. It looks like you accidentally pushed CRLF (or GIT failed to remove the CR during the push). |
It is possible that what you are seeing is related to this recent change: #2174 |
… the new allowParametersBeforeEndOfOptions feature
when editing the documentation, so I need to modify both index.adoc and index.html? |
No, there’s no need to change the HTML, those are generated from the AsciiDoc. |
…ption, with corresponding unit tests.
Requested changes complete. Please re-review. |
Thank you! Note that I’ll probably do one more 4.7.x release before merging this PR into the |
Create new feature parameterAllowedBeforeEndOfOptions to allow restricting positional parameters until after EndOfOptions. Enhancement for and fixes #2103