You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@commons.apache.org by "Claudenw (via GitHub)" <gi...@apache.org> on 2024/02/08 11:21:00 UTC
[PR] Added Supplier defaults for getParsedOptionValue [commons-cli]
Claudenw opened a new pull request, #229:
URL: https://github.com/apache/commons-cli/pull/229
Fix for CLI-323
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CLI-323: Added Supplier defaults for getParsedOptionValue [commons-cli]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on code in PR #229:
URL: https://github.com/apache/commons-cli/pull/229#discussion_r1491185632
##########
src/test/java/org/apache/commons/cli/CommandLineTest.java:
##########
@@ -166,7 +179,27 @@ public void testGetParsedOptionValueWithOption() throws Exception {
}
@Test
- public void testNullhOption() throws Exception {
+ public void testGetParsedOptionValueUsingDefault() throws Exception {
+ final Options options = new Options();
+ final Option optI = Option.builder("i").hasArg().type(Number.class).build();
+ final Option optF = Option.builder("f").hasArg().build();
+ options.addOption(optI);
+ options.addOption(optF);
+
+ final CommandLineParser parser = new DefaultParser();
+ final CommandLine cmd = parser.parse(options, new String[] {"-i", "123"});
+
+ assertEquals(123, ((Number) cmd.getParsedOptionValue(optI)).intValue());
+ assertEquals("foo", cmd.getParsedOptionValue(optF, "foo"));
+ assertEquals("foo", cmd.getParsedOptionValue(optF, () -> "foo"));
+ assertEquals("foo", cmd.getParsedOptionValue("f", "foo"));
+ assertEquals("foo", cmd.getParsedOptionValue("f", () -> "foo"));
+ assertEquals("foo", cmd.getParsedOptionValue('f', "foo"));
+ assertEquals("foo", cmd.getParsedOptionValue('f', () -> "foo"));
Review Comment:
The above checks are good but are all "happy paths".
I think you need assertions calling `getParsedOptionValue()` with a null Supplier and a Supplier that returns null since the runtime now supports this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CLI-323] Added Supplier defaults for getParsedOptionValue [commons-cli]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory merged PR #229:
URL: https://github.com/apache/commons-cli/pull/229
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CLI-323: Added Supplier defaults for getParsedOptionValue [commons-cli]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on code in PR #229:
URL: https://github.com/apache/commons-cli/pull/229#discussion_r1489662879
##########
src/test/java/org/apache/commons/cli/CommandLineTest.java:
##########
@@ -166,7 +166,27 @@ public void testGetParsedOptionValueWithOption() throws Exception {
}
@Test
- public void testNullhOption() throws Exception {
+ public void testGetParsedOptionValueUsingDefault() throws Exception {
+ final Options options = new Options();
+ final Option optI = Option.builder("i").hasArg().type(Number.class).build();
+ final Option optF = Option.builder("f").hasArg().build();
+ options.addOption(optI);
+ options.addOption(optF);
+
+ final CommandLineParser parser = new DefaultParser();
+ final CommandLine cmd = parser.parse(options, new String[] {"-i", "123"});
+
+ assertEquals(123, ((Number) cmd.getParsedOptionValue(optI)).intValue());
+ assertEquals("foo", cmd.getParsedOptionValue(optF, "foo"));
+ assertEquals("foo", cmd.getParsedOptionValue(optF, ()-> "foo"));
Review Comment:
`() -> ...`
##########
src/main/java/org/apache/commons/cli/CommandLine.java:
##########
@@ -437,15 +452,27 @@ public <T> T getParsedOptionValue(final Option option) throws ParseException {
* @see PatternOptionBuilder
* @since 1.7.0
*/
- @SuppressWarnings("unchecked")
public <T> T getParsedOptionValue(final Option option, final T defaultValue) throws ParseException {
- if (option == null) {
- return null;
- }
- final String res = getOptionValue(option);
+ return getParsedOptionValue(option, () -> defaultValue);
+ }
+
+ /**
+ * Gets a version of this {@code Option} converted to a particular type.
+ *
+ * @param option the name of the option.
+ * @param defaultValue the default value to return if opt is not set.
+ * @param <T> The return type for the method.
+ * @return the value parsed into a particular object.
+ * @throws ParseException if there are problems turning the option value into the desired type
+ * @see PatternOptionBuilder
+ * @since 1.7.0
+ */
+ @SuppressWarnings("unchecked")
+ public <T> T getParsedOptionValue(final Option option, final Supplier<T> defaultValue) throws ParseException {
+ final String res = option == null ? null : getOptionValue(option);
try {
- return res == null ? defaultValue : (T) option.getConverter().apply(res);
+ return res == null ? defaultValue.get() : (T) option.getConverter().apply(res);
Review Comment:
Hi @Claudenw
You're missing a test for when `defaultValue` is null.
##########
src/main/java/org/apache/commons/cli/CommandLine.java:
##########
@@ -423,7 +438,7 @@ public <T> T getParsedOptionValue(final char opt, final T defaultValue) throws P
* @since 1.5.0
*/
public <T> T getParsedOptionValue(final Option option) throws ParseException {
- return getParsedOptionValue(option, null);
+ return getParsedOptionValue(option, ()-> null);
Review Comment:
Missing a space after `()`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CLI-323: Added Supplier defaults for getParsedOptionValue [commons-cli]
Posted by "Claudenw (via GitHub)" <gi...@apache.org>.
Claudenw commented on code in PR #229:
URL: https://github.com/apache/commons-cli/pull/229#discussion_r1490721677
##########
src/test/java/org/apache/commons/cli/CommandLineTest.java:
##########
@@ -166,7 +166,27 @@ public void testGetParsedOptionValueWithOption() throws Exception {
}
@Test
- public void testNullhOption() throws Exception {
+ public void testGetParsedOptionValueUsingDefault() throws Exception {
+ final Options options = new Options();
+ final Option optI = Option.builder("i").hasArg().type(Number.class).build();
+ final Option optF = Option.builder("f").hasArg().build();
+ options.addOption(optI);
+ options.addOption(optF);
+
+ final CommandLineParser parser = new DefaultParser();
+ final CommandLine cmd = parser.parse(options, new String[] {"-i", "123"});
+
+ assertEquals(123, ((Number) cmd.getParsedOptionValue(optI)).intValue());
+ assertEquals("foo", cmd.getParsedOptionValue(optF, "foo"));
+ assertEquals("foo", cmd.getParsedOptionValue(optF, ()-> "foo"));
Review Comment:
If you know how to fix the checkstyle to look for this error I would appreciate it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CLI-323: Added Supplier defaults for getParsedOptionValue [commons-cli]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on code in PR #229:
URL: https://github.com/apache/commons-cli/pull/229#discussion_r1491025523
##########
src/test/java/org/apache/commons/cli/CommandLineTest.java:
##########
@@ -166,7 +166,27 @@ public void testGetParsedOptionValueWithOption() throws Exception {
}
@Test
- public void testNullhOption() throws Exception {
+ public void testGetParsedOptionValueUsingDefault() throws Exception {
+ final Options options = new Options();
+ final Option optI = Option.builder("i").hasArg().type(Number.class).build();
+ final Option optF = Option.builder("f").hasArg().build();
+ options.addOption(optI);
+ options.addOption(optF);
+
+ final CommandLineParser parser = new DefaultParser();
+ final CommandLine cmd = parser.parse(options, new String[] {"-i", "123"});
+
+ assertEquals(123, ((Number) cmd.getParsedOptionValue(optI)).intValue());
+ assertEquals("foo", cmd.getParsedOptionValue(optF, "foo"));
+ assertEquals("foo", cmd.getParsedOptionValue(optF, ()-> "foo"));
Review Comment:
> 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CLI-323: Added Supplier defaults for getParsedOptionValue [commons-cli]
Posted by "Claudenw (via GitHub)" <gi...@apache.org>.
Claudenw commented on PR #229:
URL: https://github.com/apache/commons-cli/pull/229#issuecomment-1943316591
I think I have this fixed now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CLI-323: Added Supplier defaults for getParsedOptionValue [commons-cli]
Posted by "Claudenw (via GitHub)" <gi...@apache.org>.
Claudenw commented on code in PR #229:
URL: https://github.com/apache/commons-cli/pull/229#discussion_r1492516030
##########
src/test/java/org/apache/commons/cli/CommandLineTest.java:
##########
@@ -166,7 +179,27 @@ public void testGetParsedOptionValueWithOption() throws Exception {
}
@Test
- public void testNullhOption() throws Exception {
+ public void testGetParsedOptionValueUsingDefault() throws Exception {
+ final Options options = new Options();
+ final Option optI = Option.builder("i").hasArg().type(Number.class).build();
+ final Option optF = Option.builder("f").hasArg().build();
+ options.addOption(optI);
+ options.addOption(optF);
+
+ final CommandLineParser parser = new DefaultParser();
+ final CommandLine cmd = parser.parse(options, new String[] {"-i", "123"});
+
+ assertEquals(123, ((Number) cmd.getParsedOptionValue(optI)).intValue());
+ assertEquals("foo", cmd.getParsedOptionValue(optF, "foo"));
+ assertEquals("foo", cmd.getParsedOptionValue(optF, () -> "foo"));
+ assertEquals("foo", cmd.getParsedOptionValue("f", "foo"));
+ assertEquals("foo", cmd.getParsedOptionValue("f", () -> "foo"));
+ assertEquals("foo", cmd.getParsedOptionValue('f', "foo"));
+ assertEquals("foo", cmd.getParsedOptionValue('f', () -> "foo"));
Review Comment:
Added tests.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CLI-323: Added Supplier defaults for getParsedOptionValue [commons-cli]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #229:
URL: https://github.com/apache/commons-cli/pull/229#issuecomment-1948444575
## [Codecov](https://app.codecov.io/gh/apache/commons-cli/pull/229?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
All modified and coverable lines are covered by tests :white_check_mark:
> Comparison is base [(`9f6b23b`)](https://app.codecov.io/gh/apache/commons-cli/commit/9f6b23b04d88fef2eaa67607753f3fc82375c52e?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) 91.90% compared to head [(`4f53cba`)](https://app.codecov.io/gh/apache/commons-cli/pull/229?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) 92.11%.
> Report is 12 commits behind head on master.
<details><summary>Additional details and impacted files</summary>
```diff
@@ 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
```
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/commons-cli/pull/229?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org