[CLI-312] Inconsistent behaviour in key/value pairs (Java property style)#233
Conversation
| */ | ||
| private void checkRequiredArgs() throws ParseException { | ||
| if (currentOption != null && currentOption.requiresArg()) { | ||
| if (isJavaProperty(currentOption.getKey()) && currentOption.getValues().length == 1) { |
There was a problem hiding this comment.
Hi @Claudenw
I think you are missing a test case to account for Option#getValues() returning null.
There was a problem hiding this comment.
Added test, and modified code to handle the case. Good catch.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #233 +/- ##
============================================
+ Coverage 91.90% 91.94% +0.04%
- Complexity 575 582 +7
============================================
Files 22 22
Lines 1247 1254 +7
Branches 210 212 +2
============================================
+ Hits 1146 1153 +7
Misses 63 63
Partials 38 38 ☔ View full report in Codecov by Sentry. |
garydgregory
left a comment
There was a problem hiding this comment.
@Claudenw
TY for your update.
Please see my comments.
| * @return the number of values this option has. | ||
| * @since 1.7.0 | ||
| */ | ||
| int numberOfValues() { |
There was a problem hiding this comment.
Unnecessary IMO, especially since it is not public and only used once. See above.
| */ | ||
| private void checkRequiredArgs() throws ParseException { | ||
| if (currentOption != null && currentOption.requiresArg()) { | ||
| if (isJavaProperty(currentOption.getKey()) && currentOption.numberOfValues() == 1) { |
There was a problem hiding this comment.
Instead of adding a method that's only used once, you can say instead currentOption.getValue() != null or currentOption.getValuesList().size() == 1
There was a problem hiding this comment.
I missed getValuesList() when I was implementing. I saw that getValues() would return null and that threw me off. Good catch.
garydgregory
left a comment
There was a problem hiding this comment.
@Claudenw
TY for your update.
One more comment...
| } | ||
|
|
||
| @Test | ||
| public void testNoOptionValues() throws ParseException { |
There was a problem hiding this comment.
ParseException is not thrown by this method, please remove it from the method signature.
Fix for CLI-312 https://issues.apache.org/jira/browse/CLI-312
Solves issue where java property type option has a single value separated from the -D flag e.g "-D y" as was illustrated in pull request #189
closes #189