Fix for CLI-340 #334

Claudenw · 2024-11-12T17:41:36Z

Fix for CLI-340 (add getParsedOptionValues())
fix a minor formatting issue in Help javadoc.

garydgregory

See exception management comment (rebase on git master svp)
For all Javadoc of new methods: Can the method return null? If so, when? What happens if the option does not exist?
Are all these APIs really needed? Couldn't we just add a minimal set following the YAGNI principle. Not my use case, so you tell me ;-)

garydgregory · 2024-11-12T18:59:06Z

src/main/java/org/apache/commons/cli/CommandLine.java

+                result[i] = clazz.cast(option.getConverter().apply(values[i]));
+            }
+            return result;
+        } catch (Throwable t) {


Catching Throwable is a bit of an anti-pattern here IMO because you certainly don't want to demote Errors as ParseExceptions

You should catch more specific exceptions, even if means catching all of Exception.

True, I was following the pattern from getParsedValue(). I will fix it there as well, but since the Converter throws Throwable we have to deal with it as a Throwable.

Not anymore ;-) Please rebase git on master (my comment from the preview review).

garydgregory · 2024-11-12T19:01:52Z

src/main/java/org/apache/commons/cli/CommandLine.java

@@ -691,6 +692,203 @@ public <T> T getParsedOptionValue(final String opt, final T defaultValue) throws
        return getParsedOptionValue(resolveOption(opt), defaultValue);
    }

+    /**
+     * Gets a version of this {@code Option} converted to an array of a particular type.
+     *


Can the method return null? If so, when? What happens if the option does not exist?

As with all the value getter methods if the option is not in the command line null is returned unless the default value is defined.

OK, then please update the Javadoc instead of writing it up here ;-)

garydgregory · 2024-11-12T19:02:37Z

src/main/java/org/apache/commons/cli/CommandLine.java

+     * @see PatternOptionBuilder
+     * @since 1.10.0
+     */
+    public <T> T[] getParsedOptionValues(final char opt) throws ParseException {


No need to abbreviate IMO: opt -> option. Also, using opt in Javadoc makes for awkward reading.

I am following the pattern used in the class. Methods from earlier versions used opt, I elected to do the same.

Well, it's slightly awkward (to me) to read "@param opt the name of the option." If 'opt' is the name, then why not call it 'name' or 'optionName'? Might as well do the best we can for new code. We can fix the rest separately if you want or do it all in this PR since.

Claudenw · 2024-11-12T23:10:27Z

For all Javadoc of new methods: Can the method return null? If so, when? What happens if the option does not exist?

I updated the javadoc to reflect the null or default value return as appropriate. I also updated the javadoc for the getParsedOptionValue() methods.

Are all these APIs really needed? Couldn't we just add a minimal set following the YAGNI principle. Not my use case, so you tell me ;-)

There are all here because they follow the same pattern as getParsedOptionValue() methods. I think they need to be here because V1 supports all those access methods for getOptionValue, getParsedOptionValue, and getOptionValues.

I think that in V2 I would limit each set to 4 implementations getOptionValue(Option), getOptionValue(OptionGroup), getOptionValue(Option, default), and getOptionValue(OptionGroup, default). The char and String versions are just fluff. I think that since the application has to create the Options it can keep them around and use them directly without referring to the char or String versions keys.

garydgregory

Hi @Claudenw

I've added 3 comments.

garydgregory · 2024-11-12T23:50:19Z

src/main/java/org/apache/commons/cli/CommandLine.java

+                result[i] = clazz.cast(option.getConverter().apply(values[i]));
+            }
+            return result;
+        } catch (Throwable t) {


Not anymore ;-) Please rebase git on master (my comment from the preview review).

garydgregory · 2024-11-12T23:51:28Z

src/main/java/org/apache/commons/cli/CommandLine.java

@@ -691,6 +692,203 @@ public <T> T getParsedOptionValue(final String opt, final T defaultValue) throws
        return getParsedOptionValue(resolveOption(opt), defaultValue);
    }

+    /**
+     * Gets a version of this {@code Option} converted to an array of a particular type.
+     *


OK, then please update the Javadoc instead of writing it up here ;-)

garydgregory · 2024-11-12T23:53:55Z

src/main/java/org/apache/commons/cli/CommandLine.java

+     * @see PatternOptionBuilder
+     * @since 1.10.0
+     */
+    public <T> T[] getParsedOptionValues(final char opt) throws ParseException {


Well, it's slightly awkward (to me) to read "@param opt the name of the option." If 'opt' is the name, then why not call it 'name' or 'optionName'? Might as well do the best we can for new code. We can fix the rest separately if you want or do it all in this PR since.

Claudenw · 2024-11-15T16:26:35Z

If you want me to change "opt" to "option" throughout the CommandLine class I'll do it. But having some method params be "opt" and others be "option" seems sloppy.

garydgregory · 2024-11-15T16:35:42Z

If you want me to change "opt" to "option" throughout the CommandLine class I'll do it. But having some method params be "opt" and others be "option" seems sloppy.

Hi @Claudenw
I don't want to give you more work so I'll leave it up to you and it could be done later.

Claudenw self-assigned this Nov 12, 2024

Claudenw marked this pull request as ready for review November 12, 2024 17:42

Claudenw requested a review from garydgregory November 12, 2024 17:45

garydgregory requested changes Nov 12, 2024

View reviewed changes

Claudenw added 4 commits November 13, 2024 00:00

Fix for CLI-340

44a4fa2

removed unused import

9a5ff69

fixed javadocs and catch(Throwable) issues

d05f194

rebased and fixed catch issue

a55b1ff

Claudenw force-pushed the CLI-340_add_getParsedOptionValues branch from d1df605 to a55b1ff Compare November 13, 2024 00:04

Claudenw requested a review from garydgregory November 15, 2024 16:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix for CLI-340 #334

Fix for CLI-340 #334

Claudenw commented Nov 12, 2024

garydgregory left a comment •

edited

Loading

garydgregory Nov 12, 2024 •

edited

Loading

Claudenw Nov 12, 2024

garydgregory Nov 12, 2024

garydgregory Nov 12, 2024

Claudenw Nov 12, 2024

garydgregory Nov 12, 2024

garydgregory Nov 12, 2024

Claudenw Nov 12, 2024

garydgregory Nov 12, 2024

Claudenw commented Nov 12, 2024 •

edited

Loading

garydgregory left a comment

garydgregory Nov 12, 2024

garydgregory Nov 12, 2024

garydgregory Nov 12, 2024

Claudenw commented Nov 15, 2024

garydgregory commented Nov 15, 2024

Fix for CLI-340 #334

Are you sure you want to change the base?

Fix for CLI-340 #334

Conversation

Claudenw commented Nov 12, 2024

garydgregory left a comment • edited Loading

Choose a reason for hiding this comment

garydgregory Nov 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Claudenw commented Nov 12, 2024 • edited Loading

garydgregory left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Claudenw commented Nov 15, 2024

garydgregory commented Nov 15, 2024

garydgregory left a comment •

edited

Loading

garydgregory Nov 12, 2024 •

edited

Loading

Claudenw commented Nov 12, 2024 •

edited

Loading