fix issue #995

[wycers]

fix #995

[codecov-io]

Codecov Report

Merging #1024 into master will decrease coverage by 0.17%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##             master    #1024      +/-   ##
============================================
- Coverage     94.54%   94.36%   -0.18%     
- Complexity      417      443      +26     
============================================
  Files             2        2              
  Lines          6176     6424     +248     
  Branches       1656     1719      +63     
============================================
+ Hits           5839     6062     +223     
- Misses           92       93       +1     
- Partials        245      269      +24     

Impacted Files	Coverage Δ	Complexity Δ
src/main/java/picocli/CommandLine.java	94.20% <100.00%> (-0.20%)	304.00 <0.00> (+21.00)	⬇️
src/main/java/picocli/AutoComplete.java	97.04% <0.00%> (+0.06%)	139.00% <0.00%> (+5.00%)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8b13948...9f98530. Read the comment docs.

[remkop]

@wycers @Linyer-qwq Thank you for the pull request!

I added some more tests (currently @Ignored) that currently fail, with @Parameters-annotated Set, Map and EnumSet fields.

Your fix surprised me: I thought something more difficult would be necessary. 👍 😅

I did more analysis and added a comment on #995. The fix may be even simpler than your proposal, perhaps this is enough:

// applyValuesToCollectionField (and similar change in applyValuesToMapField)
if (collection == null || ( /*!collection.isEmpty() &&*/  !initialized.contains(argSpec))) {

Can you explore this?

[wycers]

Thanks for adding more tests, we will follow up.

[remkop]

@wycers @Linyer-qwq any update?

I am planning to finish up some items over the weekend (hopefully including a fix for #995) and release picocli 4.3.

Please let me know if you are still working on this.

[wycers]

We're still working on this, but it's a little difficult for us.

No worries if you want to fix it. We're happy to see that happens!

[Linyer-qwq]

Maybe you could give us a little more time....XD

[remkop]

@wycers @Linyer-qwq Thank you for the quick reply!

Maybe you could give us a little more time....XD

Okay. Can you give me some indication of how much time?

I suggest the following approach:

• I did some analysis and wrote a summary in this comment in Reset to initial value for multi-value options/positional params #995. Because of that analysis, I believe the below may be a good way to fix it:

// applyValuesToCollectionField (and similar change in applyValuesToMapField)
if (collection == null || ( /*!collection.isEmpty() &&*/  !initialized.contains(argSpec))) {

• This fix will likely cause some new test failures, specifically, tests that verify that empty initial instances of collections or maps are reused. For example:

@Parameters List<File> list = new ArrayList(); // this instance used to be reused but after our change it is not reused

• With the new logic we will no longer reuse and modify that initial instance. When the first value is matched we will create a new ArrayList (or other collection or Map), and put the values into that new instance.
• So, the tests that verify reuse will fail, and that is okay, because we do not want to reuse this initial instance. For those tests, we need to change the expectation. (Expect no reuse.)
• However, if no value is matched, then we expect that our initial value is unchanged.
• There is one test with an ArrayBlockingQueue that fails because there is public no-arg constructor. That test can be @Ignore-ed.

[Linyer-qwq]

Sorry to reply so lately. I am following that "change the expectation. (Expect no reuse.)" So most of the failed testcase are passed now. And now I am working on fixing testcase : "testMapFieldHappyCase". I thought i will work it out tonight.

[remkop]

No problem at all.
Take your time; I’ll probably do the release on Sunday.

[wycers]

We give it a new try... (#1035)

[remkop]

I merged #1035, so I will close this pull request.
(This PR is still mentioned in the release notes for picocli 4.3 though.)