Create new feature parameterAllowedBeforeEndOfOptions #2115

ddsharpe · 2023-09-18T22:18:24Z

Create new feature parameterAllowedBeforeEndOfOptions to allow restricting positional parameters until after EndOfOptions. Enhancement for and fixes #2103

…cting positional parameters until after EndOfOptions

ddsharpe · 2023-09-26T16:34:17Z

Are you waiting on something from me?

remkop · 2023-09-27T08:42:13Z

Apologies. Family circumstances prevent me from working on my open source projects at the moment.

ddsharpe · 2023-12-12T20:25:39Z

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?

remkop

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

remkop · 2023-12-14T10:54:30Z

src/test/java/picocli/Issue2103.java

+/**
+ * Enhancement from issue 2103 enables or disables positional parameters before the EndOfOptions delimiter (such as "--").
+ */
+public class Issue2103 {


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

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

remkop · 2023-12-14T10:57:56Z

src/main/java/picocli/CommandLine.java

@@ -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);


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.

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 --}"

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 --)

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.

remkop · 2023-12-14T11:00:03Z

src/main/java/picocli/CommandLine.java

+    /** 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.
+     */


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.)

ddsharpe · 2023-12-19T23:34:52Z

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).
docs/A-Whirlwind-Tour-of-Picocli.html
docs/announcing-picocli-1.0.html
docs/autocomplete.html
docs/build-great-native-cli-apps-in-java-with-graalvm-and-picocli.html
docs/feedback.html
docs/groovy-2.5-clibuilder-renewal-part1.html
docs/groovy-2.5-clibuilder-renewal-part2.html
docs/groovy-2.5-clibuilder-renewal.html
docs/index.html
docs/man/index.html
docs/migrating-from-commons-cli.html
docs/picocli-2.0-do-more-with-less.html
docs/picocli-2.0-groovy-scripts-on-steroids.html
docs/picocli-on-graalvm.html
docs/picocli-programmatic-api.html
docs/quick-guide.html
docs/zh/picocli-2.0-do-more-with-less.html
docs/zh/picocli-2.0-groovy-scripts-on-steroids.html

remkop · 2023-12-20T02:42:08Z

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). docs/A-Whirlwind-Tour-of-Picocli.html docs/announcing-picocli-1.0.html docs/autocomplete.html docs/build-great-native-cli-apps-in-java-with-graalvm-and-picocli.html docs/feedback.html docs/groovy-2.5-clibuilder-renewal-part1.html docs/groovy-2.5-clibuilder-renewal-part2.html docs/groovy-2.5-clibuilder-renewal.html docs/index.html docs/man/index.html docs/migrating-from-commons-cli.html docs/picocli-2.0-do-more-with-less.html docs/picocli-2.0-groovy-scripts-on-steroids.html docs/picocli-on-graalvm.html docs/picocli-programmatic-api.html docs/quick-guide.html docs/zh/picocli-2.0-do-more-with-less.html docs/zh/picocli-2.0-groovy-scripts-on-steroids.html

It is possible that what you are seeing is related to this recent change: #2174
(which ironically was meant to fix CRLF issues...)

…foreEndOfOptions

… the new allowParametersBeforeEndOfOptions feature

ddsharpe · 2023-12-21T23:09:12Z

when editing the documentation, so I need to modify both index.adoc and index.html?

remkop · 2023-12-21T23:22:52Z

No, there’s no need to change the HTML, those are generated from the AsciiDoc.
Just the AsciiDoc is fine.

…ption, with corresponding unit tests.

…ter-after-end

ddsharpe · 2024-01-04T00:21:52Z

Requested changes complete. Please re-review.

remkop · 2024-01-04T12:36:02Z

Thank you!
Still reviewing, but looks good so far.

Note that I’ll probably do one more 4.7.x release before merging this PR into the main branch.

Create new feature parameterAllowedBeforeEndOfOptions to allow restri…

311ab39

…cting positional parameters until after EndOfOptions

ddsharpe closed this Sep 18, 2023

ddsharpe reopened this Sep 19, 2023

remkop self-assigned this Sep 19, 2023

remkop added this to the 4.8 milestone Dec 14, 2023

remkop requested changes Dec 14, 2023

View reviewed changes

Merge branch 'main' into parameter-after-end

5b469e4

remkop added type: enhancement ✨ type: API 🔌 theme: parser An issue or change related to the parser labels Dec 14, 2023

Merge branch 'remkop:main' into parameter-after-end

5af6e2b

added javadoc since notations to all new API methods for this feature.

c90bbfb

ddsharpe added 2 commits December 21, 2023 16:49

additional unit tests for subcommand behavior with parameterAllowedBe…

6df1aa7

…foreEndOfOptions

fix TracerTest unit tests where output was changed by the addition of…

97dd03b

… the new allowParametersBeforeEndOfOptions feature

ddsharpe and others added 4 commits January 3, 2024 15:59

Added documentation for new parser configuration option

eca3fc4

Merge branch 'main' into parameter-after-end

1781c14

Altered error message for UnmatchedArgumentException for new parser o…

64b1470

…ption, with corresponding unit tests.

Merge remote-tracking branch 'origin/parameter-after-end' into parame…

3eb7bab

…ter-after-end

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create new feature parameterAllowedBeforeEndOfOptions #2115

Create new feature parameterAllowedBeforeEndOfOptions #2115

ddsharpe commented Sep 18, 2023 •

edited

ddsharpe commented Sep 26, 2023

remkop commented Sep 27, 2023

ddsharpe commented Dec 12, 2023

remkop left a comment

remkop Dec 14, 2023

remkop Dec 16, 2023

remkop Dec 14, 2023

ddsharpe Dec 21, 2023

remkop Dec 21, 2023

ddsharpe Jan 4, 2024

remkop Dec 14, 2023

ddsharpe commented Dec 19, 2023

remkop commented Dec 20, 2023

ddsharpe commented Dec 21, 2023

remkop commented Dec 21, 2023

ddsharpe commented Jan 4, 2024

remkop commented Jan 4, 2024

Create new feature parameterAllowedBeforeEndOfOptions #2115

Are you sure you want to change the base?

Create new feature parameterAllowedBeforeEndOfOptions #2115

Conversation

ddsharpe commented Sep 18, 2023 • edited

ddsharpe commented Sep 26, 2023

remkop commented Sep 27, 2023

ddsharpe commented Dec 12, 2023

remkop left a comment

Choose a reason for hiding this comment

remkop Dec 14, 2023

Choose a reason for hiding this comment

remkop Dec 16, 2023

Choose a reason for hiding this comment

remkop Dec 14, 2023

Choose a reason for hiding this comment

ddsharpe Dec 21, 2023

Choose a reason for hiding this comment

remkop Dec 21, 2023

Choose a reason for hiding this comment

ddsharpe Jan 4, 2024

Choose a reason for hiding this comment

remkop Dec 14, 2023

Choose a reason for hiding this comment

ddsharpe commented Dec 19, 2023

remkop commented Dec 20, 2023

ddsharpe commented Dec 21, 2023

remkop commented Dec 21, 2023

ddsharpe commented Jan 4, 2024

remkop commented Jan 4, 2024

ddsharpe commented Sep 18, 2023 •

edited