-
Notifications
You must be signed in to change notification settings - Fork 6
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
Throw CommandLineParserInternalException for immutable collections #102
base: master
Are you sure you want to change the base?
Throw CommandLineParserInternalException for immutable collections #102
Conversation
Codecov Report
@@ Coverage Diff @@
## master #102 +/- ##
===========================================
+ Coverage 75.68% 75.78% +0.1%
- Complexity 576 583 +7
===========================================
Files 22 22
Lines 2159 2189 +30
Branches 447 448 +1
===========================================
+ Hits 1634 1659 +25
- Misses 350 351 +1
- Partials 175 179 +4
Continue to review full report at Codecov.
|
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.
A couple of change requests for this one. We might want to factor out some code from setArgument since its getting to be a bit long. If you feel like doing that, should probably do it in a separate commit on top of the code review comment changes. Thanks.
//user specified this arg=null which is interpreted as empty list | ||
c.clear(); | ||
} else { | ||
c.add(value); |
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.
There is at least one other place in this method that calls clear on a collection (line 627). We should scan the code for any places that need to be changed.
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.
Done. I added a method to catch the exception while applying a consumer (either clear/add, but in the future may be others).
final ImmutableCollectionArguments o = new ImmutableCollectionArguments(); | ||
new CommandLineArgumentParser(o).parseArguments(System.err, new String[]{"--LIST", "A"}); | ||
} | ||
|
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.
We need to have tests for all combinations of APPEND_TO_COLLECTIONS, "null", and real values.
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.
Done. Added data provider and different combinations.
@cmnbroad - back to you (this should pass the tests now). |
43b3c49
to
4f2e0c7
Compare
Friendly ping here, @cmnbroad! |
Closes #100