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
[CLI-323] Added Supplier<T> defaults for getParsedOptionValue #229
[CLI-323] Added Supplier<T> defaults for getParsedOptionValue #229
Conversation
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.
Hi @Claudenw
Thank you for your PR.
You'll need to add tests to cover the new feature.
I think I have this fixed now. |
|
||
try { | ||
return res == null ? defaultValue : (T) option.getConverter().apply(res); | ||
return res == null ? defaultValue.get() : (T) option.getConverter().apply(res); |
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.
Hi @Claudenw
You're missing a test for when defaultValue
is null.
|
||
assertEquals(123, ((Number) cmd.getParsedOptionValue(optI)).intValue()); | ||
assertEquals("foo", cmd.getParsedOptionValue(optF, "foo")); | ||
assertEquals("foo", cmd.getParsedOptionValue(optF, ()-> "foo")); |
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 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.
If you know how to fix the checkstyle to look for this error I would appreciate it.
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.
If you know how to fix the checkstyle to look for this error I would appreciate it.
Hi @Claudenw
Done. Please rebase on git master.
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.
@Claudenw
TY for your updates. Please see my comment.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #229 +/- ##
============================================
+ Coverage 91.90% 92.11% +0.21%
- Complexity 575 586 +11
============================================
Files 22 22
Lines 1247 1255 +8
Branches 210 212 +2
============================================
+ Hits 1146 1156 +10
+ Misses 63 61 -2
Partials 38 38 ☔ View full report in Codecov by Sentry. |
4f53cba
to
c36c7f7
Compare
Fix for CLI-323 https://issues.apache.org/jira/browse/CLI-323