You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@commons.apache.org by "JoergBudi (via GitHub)" <gi...@apache.org> on 2025/07/20 18:25:19 UTC
[PR] Bugfix/cli 221 add Option.Builder.listValueSeparator() [commons-cli]
JoergBudi opened a new pull request, #382:
URL: https://github.com/apache/commons-cli/pull/382
Strictly speaking, the described behaviour in CLI-221 is not a bug
as it is needed to fulfill the documented behaviour described in
https://issues.apache.org/jira/browse/CLI-325
around interpreting java property like arguments.
However, when you want to parse list values instead of java properties,
the behaviour is bad as the user need to terminate a list argument with
double dash --, which is counterintuitive.
Also changing the option's order changes the behaviour.
The suggested solution in PR adds the new
Option.Builder.listValueSeparator() methods, which must be used instead
of Option.Builder.valueSeparator() to achieve the behaviour desired
in this issues' description, see javadoc for usage.
It is only added in the DefaultParser() and not fixed for the
other deprecated parser implementations.
The user can then use the option in any order, e.g.
- cmd1 -o1 blue,green,yellow -f otheroption a1
- cmd1 -f otheroption -o1 blue,green,yellow a1
- cmd1 a1 -f otheroption -o1 blue,green,yellow
!--
Licensed to the Apache Software Foundation (ASF) under one
or more contributor license agreements. See the NOTICE file
distributed with this work for additional information
regarding copyright ownership. The ASF licenses this file
to you under the Apache License, Version 2.0 (the
"License"); you may not use this file except in compliance
with the License. You may obtain a copy of the License at
https://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing,
software distributed under the License is distributed on an
"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
KIND, either express or implied. See the License for the
specific language governing permissions and limitations
under the License.
-->
Thanks for your contribution to [Apache Commons](https://commons.apache.org/)! Your help is appreciated!
Before you push a pull request, review this list:
- [x] Read the [contribution guidelines](CONTRIBUTING.md) for this project.
- [x] Read the [ASF Generative Tooling Guidance](https://www.apache.org/legal/generative-tooling.html) if you use Artificial Intelligence (AI).
- [ ] I used AI to create any part of, or all of, this pull request.
- [x] Run a successful build using the default [Maven](https://maven.apache.org/) goal with `mvn`; that's `mvn` on the command line by itself.
- [x] Write unit tests that match behavioral changes, where the tests fail if the changes to the runtime are not applied. This may not always be possible, but it is a best-practice.
- [x] Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
- [x] Each commit in the pull request should have a meaningful subject line and body. Note that a maintainer may squash commits during the merge process.
--
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] Bugfix/cli 221 add Option.Builder.listValueSeparator() [commons-cli]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on PR #382:
URL: https://github.com/apache/commons-cli/pull/382#issuecomment-3155941942
> @garydgregory fixed the "low level comments", not sure what your "pr github resolve conversation policy" for tivial fixes is, for now I leave it for you to resolve.
Hello @JoergBudi
There is no official policy, but what I usually do is close the conversations I started.
Note that my comments, "A sentence should start with a capital letter," have not been addressed.
Ty!
--
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] Bugfix/cli 221 add Option.Builder.listValueSeparator() [commons-cli]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on code in PR #382:
URL: https://github.com/apache/commons-cli/pull/382#discussion_r2254163961
##########
src/main/java/org/apache/commons/cli/Option.java:
##########
@@ -103,6 +103,9 @@ private static Class<?> toType(final Class<?> type) {
/** The character that is the value separator. */
private char valueSeparator;
+ /** multiple values are within a single argument separated by valueSeparator char */
Review Comment:
A sentence should start with a capital letter.
##########
src/main/java/org/apache/commons/cli/Option.java:
##########
@@ -350,6 +359,51 @@ public Builder valueSeparator(final char valueSeparator) {
return this;
}
+ /**
+ * The Option will use ',' to invoke listValueSeparator()
+ *
+ * @since 1.11.0
+ * @return this builder.
+ */
+ public Builder listValueSeparator() {
+ return listValueSeparator(Char.COMMA);
+ }
+
+ /**
+ * defines the separator used to split a list of values passed in a single argument.
Review Comment:
A sentence should start with a capital letter.
##########
src/main/java/org/apache/commons/cli/Option.java:
##########
@@ -350,6 +359,51 @@ public Builder valueSeparator(final char valueSeparator) {
return this;
}
+ /**
+ * The Option will use ',' to invoke listValueSeparator()
+ *
+ * @since 1.11.0
+ * @return this builder.
+ */
+ public Builder listValueSeparator() {
+ return listValueSeparator(Char.COMMA);
+ }
+
+ /**
+ * defines the separator used to split a list of values passed in a single argument.
+ *
+ * Method is mutually exclusive to valueSeparator() method. In the resulting option,
+ * isValueSeparatorUsedForSingleArgument() will return true.
+ *
+ * <p>
+ * <strong>Example:</strong>
+ * </p>
+ *
+ * <pre>
+ * final Option colors = Option.builder().option("c").hasArgs().listValueSeparator('|').build();
+ * final Options options = new Options();
+ * options.addOption(colors);
+ *
+ * final String[] args = {"-c", "red|blue|yellow", "b,c"};
+ * final DefaultParser parser = new DefaultParser();
+ * final CommandLine commandLine = parser.parse(options, args, null, true);
+ * final String [] colorValues = commandLine.getOptionValues(colors);
+ * // colorValues[0] will be "red"
+ * // colorValues[1] will be "blue"
+ * // colorValues[2] will be "yellow"
+ * final String arguments = commandLine.getArgs()[0]; // will be b,c
+ *
+ * </pre>
+ *
+ * @since 1.11.0
Review Comment:
The since tag should be after the param tags.
##########
src/main/java/org/apache/commons/cli/Option.java:
##########
@@ -834,6 +892,27 @@ public boolean isRequired() {
return required;
}
+ /**
+ * Tests whether multiple values are expected in a single argument split by a separation character
+ *
+ * @return boolean true when the builder's listValueSeparator() method was used. Multiple values are expected in a single argument and
+ * are split by a separation character.
+ * @since 1.10.0
+ */
+ public boolean isValueSeparatorUsedForSingleArgument() {
+ return valueSeparatorUsedForSingleArgument;
+ }
+
+ /**
+ * Set this to true to use the valueSeparator only on a single argument. See also builder's listValueSeparator() method.
+ *
+ * @param valueSeparatorUsedForSingleArgument the new value for this property
+ * @since 1.10.0
Review Comment:
Since tag is wrong.
##########
src/main/java/org/apache/commons/cli/Option.java:
##########
@@ -430,6 +484,9 @@ public static Builder builder(final String option) {
/** The character that is the value separator. */
private char valueSeparator;
+ /** multiple values are within a single argument separated by valueSeparator char */
+ private boolean valueSeparatorUsedForSingleArgument;
Review Comment:
A sentence should start with a capital letter.
##########
src/main/java/org/apache/commons/cli/Option.java:
##########
@@ -834,6 +892,27 @@ public boolean isRequired() {
return required;
}
+ /**
+ * Tests whether multiple values are expected in a single argument split by a separation character
+ *
+ * @return boolean true when the builder's listValueSeparator() method was used. Multiple values are expected in a single argument and
+ * are split by a separation character.
+ * @since 1.10.0
Review Comment:
Since tag is wrong.
--
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] Bugfix/cli 221 add Option.Builder.listValueSeparator() [commons-cli]
Posted by "JoergBudi (via GitHub)" <gi...@apache.org>.
JoergBudi commented on PR #382:
URL: https://github.com/apache/commons-cli/pull/382#issuecomment-3156627537
Hi Gary,
please double check, I changed the 3 places you mentioned with this commit
https://github.com/apache/commons-cli/pull/382/commits/481d45f5132346a2648bdd83c8c5a67e34fedb79
and in the PR I don't see any other start of a sentence with a lower case letter ...
--
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] Bugfix/cli 221 add Option.Builder.listValueSeparator() [commons-cli]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on PR #382:
URL: https://github.com/apache/commons-cli/pull/382#issuecomment-3154923562
@ppkarwasz @Claudenw do either of you have an opinion here?
--
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] Bugfix/cli 221 add Option.Builder.listValueSeparator() [commons-cli]
Posted by "Claudenw (via GitHub)" <gi...@apache.org>.
Claudenw commented on PR #382:
URL: https://github.com/apache/commons-cli/pull/382#issuecomment-3158735362
@garydgregory @JoergBudi
I posted a longish overview into the CLI-221. I would like to see discussion to identify and resolve the end of option argument frame detection issues.
--
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] Bugfix/cli 221 add Option.Builder.listValueSeparator() [commons-cli]
Posted by "JoergBudi (via GitHub)" <gi...@apache.org>.
JoergBudi commented on PR #382:
URL: https://github.com/apache/commons-cli/pull/382#issuecomment-3155837957
@garydgregory fixed the "low level comments", not sure what your "pr github resolve conversation policy" for tivial fixes is, for now I leave it for you to resolve.
--
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] Bugfix/cli 221 add Option.Builder.listValueSeparator() [commons-cli]
Posted by "JoergBudi (via GitHub)" <gi...@apache.org>.
JoergBudi commented on PR #382:
URL: https://github.com/apache/commons-cli/pull/382#issuecomment-3154262700
@garydgregory : would be nice if you can have a look at it when it suits you. I updated the branch after latest release, I bumped the doc version of the new api to 1.11.0 (as new API is in it). I don't need a release soon, I just seek "closure" one way or the other on this issue.
--
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