[#1380] Fix for requiredOptionMarker displayed on ArgGroup #1505

ahmede41 · 2021-12-08T07:14:22Z

fix for Issue#1380

…r Methods

.gitignore

remkop · 2021-12-09T03:06:48Z

(I had a longer overall comment but I can't see it now so let me post it again.)

Thank you for this PR!

Looking back at the commit for issue #871 that made options in exclusive groups required, it is clear that this solution resulted in a loss of information: it became impossible to determine whether an option was originally marked as required = true or not. Which is the direct cause of #1380.

I like the approach of this PR to add a field to store the original value of required.

I also like the fact that the PR adds several tests, nice! (Also thanks for the test for #1420, that will be useful when working on that ticket.)
I did have some feedback on the tests to make them smaller and more concise, can you take a look?

Also I noticed that this PR modifies several html files in the docs/ folder, can those changes be excluded from this PR?

I like the approach of this PR, I think it is the correct way to go.
One small thing is the naming of the newly added field.
Can you take a look at my feedback on that?

ahmede41 · 2021-12-10T05:53:38Z

(I had a longer overall comment but I can't see it now so let me post it again.)

Thank you for this PR!

Looking back at the commit for issue #871 that made options in exclusive groups required, it is clear that this solution resulted in a loss of information: it became impossible to determine whether an option was originally marked as required = true or not. Which is the direct cause of #1380.

I like the approach of this PR to add a field to store the original value of required.

I also like the fact that the PR adds several tests, nice! (Also thanks for the test for #1420, that will be useful when working on that ticket.) I did have some feedback on the tests to make them smaller and more concise, can you take a look?

Also I noticed that this PR modifies several html files in the docs/ folder, can those changes be excluded from this PR?

I like the approach of this PR, I think it is the correct way to go. One small thing is the naming of the newly added field. Can you take a look at my feedback on that?

@remkop somehow I don't see any comments/feedback on code, Can you check?

remkop

@remkop somehow I don't see any comments/feedback on code, Can you check?

I think I lost my comment and then did not press the "Submit review" button so my feedback has been pending all the time... sorry about that.

Hopefully the individual comments will become visible when I click "Submit review" on this comment. Summary of the main items:

I did have some feedback on the tests to make them smaller and more concise, can you take a look?
I noticed that this PR modifies several html files in the docs/ folder, can those changes be excluded from this PR?
I like the approach of this PR, I think it is the correct way to go. One small thing is the naming of the newly added field. Can you take a look at my feedback on that?

remkop · 2021-12-09T00:07:49Z

qaplug_result.html

+&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;<font style="font-family:verdana; font-weight:bold; color:#005555; size = 3">Project:</font><br/>&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;<b>picocli</b><br/>&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;<font style="font-family:verdana; font-weight:bold; color:#005555; size = 3">Scope:</font><br/>&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;<b>File '…/picocli-annotation-processing-tests/src/test/java/picocli/Issue1380Test.java [picocli.picocli-annotation-processing-tests.test]'</b><br/>&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;<font style="font-family:verdana; font-weight:bold; color:#005555; size = 3">Profile:</font><br/>&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;<b>Default</b><br/>&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;<font style="font-family:verdana; font-weight:bold; color:#005555; size = 3">Results:</font><br/>&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;<b>Enabled coding rules: 459</b><br/><ul><li>FindBugs: 346</li><li>Checkstyle: 30</li><li>PMD: 83</li></ul>&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;<b>Problems found: 0</b><br/><ul><li>FindBugs: 0</li><li>Checkstyle: 0</li><li>PMD: 0</li></ul><br/>&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;<font style="font-family:verdana; font-weight:bold; color:#005555; size = 3">Time statistics:</font><ul><li>Started: Sun Nov 21 13:09:35 PST 2021</li><ul><li>Initialization: 00:00:00.769</li><li>PMD: 00:00:01.066</li><li>FindBugs: 00:00:01.553</li><li>Checkstyle: 00:00:00.415</li><li>Gathering results: 00:00:00.018</li></ul><li>Finished: Sun Nov 21 13:09:39 PST 2021</li></ul>
+&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;<font style="font-family:verdana; font-weight:bold; color:#005555; size = 3">Detailed Results:</font><br/>&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;<b>Empty results</b><br/><ul></ul><br/>
+</body>
+</html>


Why is this qaplug_result.html file necessary?
If this is a report that is generated during the build, should this be added to .gitignore, or is this something that should be published?

This was part of my course work, I removed the file.

remkop · 2021-12-09T00:14:58Z

picocli-annotation-processing-tests/src/test/java/picocli/Issue1380Test.java

+        return this.exclusive;
+    }
+
+    private static class ExclusiveOptions {


It seems that the TestingClassExclusiveFalse.ExclusiveOptions and TestingClassExclusiveTrue.ExclusiveOptions classes are identical (unless I overlooked something). Maybe we can move ExclusiveOptions out of those nested classes so we only need one class? (We can rename it to Issue1380ExclusiveOptions to give it a more unique name.)

remkop · 2021-12-09T00:16:53Z

picocli-annotation-processing-tests/src/test/java/picocli/Issue1380Test.java

+        /**
+         * Added getters to satisfy PMF and findBug requirements
+         */
+        public boolean getSilent() {


Also, I am not that concerned about PMF and findBug warnings, especially for test code.
I am okay with making fields package protected and removing the getters if the getters are not used anywhere.
Less code makes it easier to read and understand. :-)

that was needed for my course work, removed

remkop · 2021-12-09T00:18:08Z

picocli-annotation-processing-tests/src/test/java/picocli/Issue1380Test.java

+        /**
+         * Added getters to satisfy PMF and findBug requirements
+         */
+        public boolean getVerbose() {


Similar to the other @Option-annotated fields (let's make them package protected and remove the unused getters).

src/main/java/picocli/CommandLine.java

remkop · 2021-12-09T02:36:41Z

src/main/java/picocli/CommandLine.java

@@ -8636,6 +8638,13 @@ void applyInitialValue(Tracer tracer) {
                }
            }

+            /** Returns whether this is a required option or positional parameter without a default value.


Let's document that this is the original value of the option's required value, regardless of whether the option is in an exclusive group or not.

How about this instead:

"Returns the original value of the option's required attribute, regardless of whether the option is used in an exclusive group or not."

src/main/java/picocli/CommandLine.java

remkop · 2021-12-09T02:41:43Z

src/main/java/picocli/CommandLine.java

@@ -8636,6 +8638,13 @@ void applyInitialValue(Tracer tracer) {
                }
            }

+            /** Returns whether this is a required option or positional parameter without a default value.
+             * If this argument is part of a {@linkplain ArgGroup group}, this method returns whether this argument is required <em>within the group</em> (so it is not necessarily a required argument for the command).
+             * @see Option#optionIsNotRequired() */


Let's remove this: there is no @Option.optionIsNotRequired or @Option.originallyRequired method on the @Option annotation. Maybe link to @see OptionSpec#required() ?

Also, since we are adding public API, let's add a @since tag.
This will be included in the 4.7.0 release, so let's add @since 4.7.0.

src/main/java/picocli/CommandLine.java

ahmede41

Updated as requested

ahmede41 · 2021-12-17T18:59:48Z

Docs folder changes were all whitespaces something from my bad IDE, that is why I had on .gitignore initially

remkop

Thank you for the changes.
I think we are almost there.

One thing: this pull request still contains many modifications to HTML files in the docs/ folder.
I don't want to modify those files.
Can you revert the changes to the HTML files in the docs/ folder?

src/main/java/picocli/CommandLine.java

remkop · 2021-12-18T03:13:44Z

src/main/java/picocli/CommandLine.java

@@ -8636,6 +8638,13 @@ void applyInitialValue(Tracer tracer) {
                }
            }

+            /** Returns whether this is a required option or positional parameter without a default value.


How about this instead:

"Returns the original value of the option's required attribute, regardless of whether the option is used in an exclusive group or not."

ahmede41 · 2021-12-18T10:25:37Z

Thank you for the changes. I think we are almost there.

One thing: this pull request still contains many modifications to HTML files in the docs/ folder. I don't want to modify those files. Can you revert the changes to the HTML files in the docs/ folder?

I reverted the docs/ files

remkop

Great, thank you for making the changes.
This looks ready to merge now.

I just need to decide on whether I will do a 4.6.3 release first or not.
I am a bit preoccupied with the Log4j issue at the moment, so this may take some time.

Thank you again for the contribution!

remkop · 2021-12-20T03:09:50Z

I am seeing the following test failures. Can you take a look?

> Task :picocli-annotation-processing-tests:test

picocli.Issue1380Test > testingWithExclusiveTrue FAILED
    org.junit.ComparisonFailure at Issue1380Test.java:68

picocli.Issue1380Test > testingWithExclusiveFalse FAILED
    org.junit.ComparisonFailure at Issue1380Test.java:86

ahmede41 · 2021-12-20T18:04:04Z

I am seeing the following test failures. Can you take a look?

> Task :picocli-annotation-processing-tests:test

picocli.Issue1380Test > testingWithExclusiveTrue FAILED
    org.junit.ComparisonFailure at Issue1380Test.java:68

picocli.Issue1380Test > testingWithExclusiveFalse FAILED
    org.junit.ComparisonFailure at Issue1380Test.java:86

I just rechecked the test and did a clean rebuild (./gradlew clean build) and no failure seen here

Issue1380Test: 2 total, 2 passed
Collapse | Expand
240 mspicocli.Issue1380Test
234 mspassedtestingWithExclusiveTrue
6 mspassedtestingWithExclusiveFalse

BUILD SUCCESSFUL in 3m 13s
98 actionable tasks: 98 executed

Looking at the checks I saw these on windows (I don't have a windows machine) (only change is the move of the inner class, Do you want me to put it back retest again?)

…oup options bugfix

remkop · 2022-02-10T11:29:36Z

Merged. Thank you for the contribution and for your patience!

ahmede41 and others added 13 commits November 18, 2021 16:10

Added a test case for Issue1380

d08858e

Updated .gitignore by docs/

2141f12

Updated my test case group to include required option

50c366e

Added more testing

b204786

Fix for Issue 1380

034b316

Added Java doc to my class on Junit tests

19a7006

Updated test case object to satisfy PMD and FindBug issues with Gette…

13de998

…r Methods

Updated Issue # fix and added QA BugPlug and PMD reports

fc13021

Added a test case for Issue1420

4179537

Added a test case for Issue1420

e1533bf

Merge branch 'master' of github.com:ahmede41/picocli

a9ddc63

Testing

0ff6d84

[remkop#1380] Fix for requiredOptionMarker displayed on ArgGroup

917dbe6

remkop reviewed Dec 8, 2021

View reviewed changes

.gitignore Outdated Show resolved Hide resolved

ahmede41 and others added 2 commits December 8, 2021 01:30

Update .gitignore

d8ec459

removing doc from gitinore and retest

33e6351

Final CS427 Submitting

914a95d

remkop requested changes Dec 10, 2021

View reviewed changes

ahmede41 force-pushed the master branch from fac6cdc to 8ec3886 Compare December 17, 2021 06:06

ahmede41 commented Dec 17, 2021

View reviewed changes

ahmede41 requested a review from remkop December 17, 2021 19:00

remkop requested changes Dec 18, 2021

View reviewed changes

ahmede41 force-pushed the master branch from 8ec3886 to 06a3f59 Compare December 18, 2021 10:22

ahmede41 requested a review from remkop December 18, 2021 10:25

[remkop#1380] Updated fix for requiredOptionMarker displayed on ArgGroup

d03ed5f

ahmede41 force-pushed the master branch from 06a3f59 to d03ed5f Compare December 18, 2021 10:47

remkop previously approved these changes Dec 18, 2021

View reviewed changes

remkop added this to the 4.6.3 milestone Dec 18, 2021

remkop added type: API 🔌 theme: usagehelp An issue or change related to the usage help message labels Dec 18, 2021

remkop modified the milestones: 4.6.3, 4.7 Jan 14, 2022

Merge branch 'main' into master

0bd6a34

remkop linked an issue Feb 10, 2022 that may be closed by this pull request

requiredOptionMarker displayed on ArgGroup when it should not #1380

Closed

remkop added 2 commits February 10, 2022 17:57

Merge branch 'main' into master

b27c7be

[remkop#1380] use OS-independent line breaks in test

020a770

remkop dismissed stale reviews from themself via 020a770 February 10, 2022 09:17

remkop approved these changes Feb 10, 2022

View reviewed changes

remkop merged commit 85cd899 into remkop:main Feb 10, 2022

remkop added a commit that referenced this pull request Feb 10, 2022

[#1380][#1505] update release notes for requiredOptionMarker on ArgGr…

1eaafd9

…oup options bugfix

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#1380] Fix for requiredOptionMarker displayed on ArgGroup #1505

[#1380] Fix for requiredOptionMarker displayed on ArgGroup #1505

ahmede41 commented Dec 8, 2021

remkop commented Dec 9, 2021 •

edited

Loading

ahmede41 commented Dec 10, 2021

remkop left a comment •

edited

Loading

remkop Dec 9, 2021

ahmede41 Dec 17, 2021

remkop Dec 9, 2021

remkop Dec 9, 2021

ahmede41 Dec 17, 2021

remkop Dec 9, 2021

ahmede41 Dec 17, 2021

remkop Dec 9, 2021

ahmede41 Dec 17, 2021

remkop Dec 18, 2021

ahmede41 Dec 18, 2021

remkop Dec 9, 2021

ahmede41 Dec 17, 2021

ahmede41 left a comment

ahmede41 commented Dec 17, 2021

remkop left a comment

remkop Dec 18, 2021

ahmede41 commented Dec 18, 2021

remkop left a comment

remkop commented Dec 20, 2021

ahmede41 commented Dec 20, 2021 •

edited

Loading

remkop commented Feb 10, 2022

[#1380] Fix for requiredOptionMarker displayed on ArgGroup #1505

[#1380] Fix for requiredOptionMarker displayed on ArgGroup #1505

Conversation

ahmede41 commented Dec 8, 2021

remkop commented Dec 9, 2021 • edited Loading

ahmede41 commented Dec 10, 2021

remkop left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahmede41 left a comment

Choose a reason for hiding this comment

ahmede41 commented Dec 17, 2021

remkop left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahmede41 commented Dec 18, 2021

remkop left a comment

Choose a reason for hiding this comment

remkop commented Dec 20, 2021

ahmede41 commented Dec 20, 2021 • edited Loading

remkop commented Feb 10, 2022

remkop commented Dec 9, 2021 •

edited

Loading

remkop left a comment •

edited

Loading

ahmede41 commented Dec 20, 2021 •

edited

Loading