Skip to content

Commit

Permalink
address review comments, improve backwards compatibility
Browse files Browse the repository at this point in the history
  • Loading branch information
@stoty
stoty committed Oct 12, 2021
1 parent 3b932f6 commit 93d13ca
Show file tree
Hide file tree
Showing 4 changed files with 177 additions and 88 deletions.
86 changes: 51 additions & 35 deletions src/main/java/org/apache/commons/cli/DefaultParser.java
View file
Expand Up @@ -55,8 +55,9 @@ public class DefaultParser implements CommandLineParser {
/** Flag indicating if partial matching of long options is supported. */
private final boolean allowPartialMatching;

/** Flag indicating if balanced leading and trailing double quotes should be stripped from option arguments. */
private final boolean stripLeadingAndTrailingQuotes;
/** Flag indicating if balanced leading and trailing double quotes should be stripped from option arguments.
* null represents the historic arbitrary behaviour */
private final Boolean stripLeadingAndTrailingQuotes;

/**
* Creates a new DefaultParser instance with partial matching enabled.
Expand All @@ -79,7 +80,7 @@ public class DefaultParser implements CommandLineParser {
*/
public DefaultParser() {
this.allowPartialMatching = true;
this.stripLeadingAndTrailingQuotes = true;
this.stripLeadingAndTrailingQuotes = null;
}

/**
Expand All @@ -105,18 +106,18 @@ public DefaultParser() {
*/
public DefaultParser(final boolean allowPartialMatching) {
this.allowPartialMatching = allowPartialMatching;
this.stripLeadingAndTrailingQuotes = true;
this.stripLeadingAndTrailingQuotes = null;
}

/**
* Create a new DefaultParser instance with the specified partial matching and quote
* Creates a new DefaultParser instance with the specified partial matching and quote
* stripping policy.
*
* @param allowPartialMatching if partial matching of long options shall be enabled
* @param stripLeadingAndTrailingQuotes if balanced outer double quoutes should be stripped
*/
private DefaultParser(final boolean allowPartialMatching,
final boolean stripLeadingAndTrailingQuotes) {
final Boolean stripLeadingAndTrailingQuotes) {
this.allowPartialMatching = allowPartialMatching;
this.stripLeadingAndTrailingQuotes = stripLeadingAndTrailingQuotes;
}
Expand Down Expand Up @@ -214,7 +215,7 @@ protected void handleConcatenatedOptions(final String token) throws ParseExcepti

if (currentOption != null && token.length() != i + 1) {
// add the trail as an argument of the option
currentOption.addValueForProcessing(token.substring(i + 1));
currentOption.addValueForProcessing(stripLeadingAndTrailingQuotesDefaultOff(token.substring(i + 1)));
break;
}
}
Expand Down Expand Up @@ -260,7 +261,7 @@ private void handleLongOptionWithEqual(final String token) throws ParseException

if (option.acceptsArg()) {
handleOption(option);
currentOption.addValueForProcessing(value);
currentOption.addValueForProcessing(stripLeadingAndTrailingQuotesDefaultOff(value));
currentOption = null;
} else {
handleUnknownToken(currentToken);
Expand Down Expand Up @@ -332,7 +333,7 @@ private void handleProperties(final Properties properties) throws ParseException

if (opt.hasArg()) {
if (opt.getValues() == null || opt.getValues().length == 0) {
opt.addValueForProcessing(value);
opt.addValueForProcessing(stripLeadingAndTrailingQuotesDefaultOff(value));
}
} else if (!("yes".equalsIgnoreCase(value) || "true".equalsIgnoreCase(value) || "1".equalsIgnoreCase(value))) {
// if the value is not yes, true or 1 then don't add the option to the CommandLine
Expand Down Expand Up @@ -379,12 +380,12 @@ private void handleShortAndLongOption(final String token) throws ParseException

if (opt != null && options.getOption(opt).acceptsArg()) {
handleOption(options.getOption(opt));
currentOption.addValueForProcessing(t.substring(opt.length()));
currentOption.addValueForProcessing(stripLeadingAndTrailingQuotesDefaultOff(t.substring(opt.length())));
currentOption = null;
} else if (isJavaProperty(t)) {
// -SV1 (-Dflag)
handleOption(options.getOption(t.substring(0, 1)));
currentOption.addValueForProcessing(t.substring(1));
currentOption.addValueForProcessing(stripLeadingAndTrailingQuotesDefaultOff(t.substring(1)));
currentOption = null;
} else {
// -S1S2S3 or -S1S2V
Expand Down Expand Up @@ -433,7 +434,7 @@ private void handleToken(final String token) throws ParseException {
} else if ("--".equals(token)) {
skipParsing = true;
} else if (currentOption != null && currentOption.acceptsArg() && isArgument(token)) {
currentOption.addValueForProcessing(conditionallyStripLeadingAndTrailingQuotes(token));
currentOption.addValueForProcessing(stripLeadingAndTrailingQuotesDefaultOn(token));
} else if (token.startsWith("--")) {
handleLongOption(token);
} else if (token.startsWith("-") && !"-".equals(token)) {
Expand Down Expand Up @@ -646,12 +647,28 @@ private void updateRequiredOptions(final Option option) throws AlreadySelectedEx

/**
* Strip balanced leading and trailing quotes if the stripLeadingAndTrailingQuotes is set
* If stripLeadingAndTrailingQuotes is null, then do not strip
*
* @param token a string
* @return token with the quotes stripped (if set)
*/
private String stripLeadingAndTrailingQuotesDefaultOff(final String token) {
if (stripLeadingAndTrailingQuotes != null && stripLeadingAndTrailingQuotes) {
return Util.stripLeadingAndTrailingQuotes(token);
} else {
return token;
}
}

/**
* Strip balanced leading and trailing quotes if the stripLeadingAndTrailingQuotes is set
* If stripLeadingAndTrailingQuotes is null, then do not strip
*
* @param token a string
* @return token with the quotes stripped (if set)
*/
protected String conditionallyStripLeadingAndTrailingQuotes(final String token) {
if (stripLeadingAndTrailingQuotes) {
private String stripLeadingAndTrailingQuotesDefaultOn(final String token) {
if (stripLeadingAndTrailingQuotes == null || stripLeadingAndTrailingQuotes) {
return Util.stripLeadingAndTrailingQuotes(token);
} else {
return token;
Expand All @@ -665,15 +682,14 @@ protected String conditionallyStripLeadingAndTrailingQuotes(final String token)
* @return a new {@link Builder} instance
* @since 1.5
*/
public static Builder builder()
{
public static Builder builder() {
return new Builder();
}

/**
* A nested builder class to create <code>DefaultParser</code> instances
* using descriptive methods.
* <p>
*
* Example usage:
* <pre>
* DefaultParser parser = Option.builder()
Expand All @@ -684,23 +700,21 @@ public static Builder builder()
*
* @since 1.5
*/
public static final class Builder
{
public static final class Builder {

/** Flag indicating if partial matching of long options is supported. */
private boolean allowPartialMatching = true;

/** Flag indicating if balanced leading and trailing double quotes should be stripped from option arguments. */
private boolean stripLeadingAndTrailingQuotes = true;
private Boolean stripLeadingAndTrailingQuotes;

/**
* Constructs a new <code>Builder</code> for a <code>DefaultParser</code> instance.
*
* Both allowPartialMatching and stripLeadingAndTrailingQuotes are true by default,
* mimicking the argument-less constructor.
*/
private Builder()
{
private Builder() {
}

/**
Expand All @@ -718,44 +732,46 @@ private Builder()
* }
* </pre>
*
* with "partial matching" turned on, {@code -de} only matches the {@code "debug"} option. However, with
* If "partial matching" is turned on, {@code -de} only matches the {@code "debug"} option. However, with
* "partial matching" disabled, {@code -de} would enable both {@code debug} as well as {@code extract}
*
* @param allowPartialMatching whether to allow partial matching of long options
* @return this builder, to allow method chaining
* @since 1.5
*/
public Builder setAllowPartialMatching(final boolean allowPartialMatching)
{
public Builder setAllowPartialMatching(final boolean allowPartialMatching) {
this.allowPartialMatching = allowPartialMatching;
return this;
}

/**
* Sets if balanced leading and trailing double quotes should be stripped from option arguments.
*
* with "stripping of balanced leading and trailing double quotes from option arguments" turned
* on, the outermost balanced double quotes of option arguments values will be removed.
* ie.
* for <code>-o '"x"'</code> getValue() will return <code>x</code>, instead of <code>"x"</code>
* If "stripping of balanced leading and trailing double quotes from option arguments" is true,
* the outermost balanced double quotes of option arguments values will be removed.
* For example, <code>-o '"x"'</code> getValue() will return <code>x</code>, instead of <code>"x"</code>
*
* If "stripping of balanced leading and trailing double quotes from option arguments" is null,
* then quotes will be stripped from option values separated by space from the option, but
* kept in other cases, which is the historic behaviour.
*
* @param stripLeadingAndTrailingQuotes whether balanced leading and trailing double quotes should be stripped from option arguments.
* @return this builder, to allow method chaining
* @since 1.5
*/
public Builder setStripLeadingAndTrailingQuotes(final boolean stripLeadingAndTrailingQuotes)
{
public Builder setStripLeadingAndTrailingQuotes(final Boolean stripLeadingAndTrailingQuotes) {
this.stripLeadingAndTrailingQuotes = stripLeadingAndTrailingQuotes;
return this;
}

/**
* Constructs an DefaultParser with the values declared by this {@link Builder}.
* Builds an DefaultParser with the values declared by this {@link Builder}.
*
* @return the new {@link DefaultParser}
* @since 1.5
*/
public DefaultParser build()
{
public DefaultParser build() {
return new DefaultParser(allowPartialMatching, stripLeadingAndTrailingQuotes);

}
}
}
12 changes: 12 additions & 0 deletions src/test/java/org/apache/commons/cli/BasicParserTest.java
View file
Expand Up @@ -173,4 +173,16 @@ public void testUnambiguousPartialLongOption4() throws Exception {
@Ignore("not supported by the BasicParser")
public void testUnrecognizedOptionWithBursting() throws Exception {
}

@Override
@Test
@Ignore("not supported by the BasicParser")
public void testShortOptionConcatenatedQuoteHandling() throws Exception {
}

@Override
@Test
@Ignore("not supported by the BasicParser")
public void testLongOptionWithEqualsQuoteHandling() throws Exception {
}
}
93 changes: 93 additions & 0 deletions src/test/java/org/apache/commons/cli/DefaultParserTest.java
View file
Expand Up @@ -17,7 +17,10 @@ Licensed to the Apache Software Foundation (ASF) under one or more

package org.apache.commons.cli;

import static org.junit.Assert.assertEquals;

import org.junit.Before;
import org.junit.Test;

public class DefaultParserTest extends ParserTestCase {

Expand All @@ -27,4 +30,94 @@ public void setUp() {
super.setUp();
parser = new DefaultParser();
}

@Override
@Test
public void testShortOptionConcatenatedQuoteHandling() throws Exception {
final String[] args = new String[] {"-b\"quoted string\""};

final CommandLine cl = parser.parse(options, args);

//This is behaviour is not consistent with the other parsers, but is required for backwards compatibility
assertEquals("Confirm -b\"arg\" keeps quotes", "\"quoted string\"", cl.getOptionValue("b"));
}

@Override
@Test
public void testLongOptionWithEqualsQuoteHandling() throws Exception {
final String[] args = new String[] {"--bfile=\"quoted string\""};

final CommandLine cl = parser.parse(options, args);

assertEquals("Confirm --bfile=\"arg\" strips quotes", "\"quoted string\"", cl.getOptionValue("b"));
}

@Test
public void testShortOptionQuoteHandlingWithStrip() throws Exception {
parser = DefaultParser.builder().setStripLeadingAndTrailingQuotes(true).build();
final String[] args = new String[] {"-b", "\"quoted string\""};

final CommandLine cl = parser.parse(options, args);

assertEquals("Confirm -b \"arg\" strips quotes", "quoted string", cl.getOptionValue("b"));
}

@Test
public void testShortOptionQuoteHandlingWithoutStrip() throws Exception {
parser = DefaultParser.builder().setStripLeadingAndTrailingQuotes(false).build();
final String[] args = new String[] {"-b", "\"quoted string\""};

final CommandLine cl = parser.parse(options, args);

assertEquals("Confirm -b \"arg\" keeps quotes", "\"quoted string\"", cl.getOptionValue("b"));
}

@Test
public void testLongOptionQuoteHandlingWithStrip() throws Exception {
parser = DefaultParser.builder().setStripLeadingAndTrailingQuotes(true).build();
final String[] args = new String[] {"--bfile", "\"quoted string\""};

final CommandLine cl = parser.parse(options, args);

assertEquals("Confirm --bfile \"arg\" strips quotes", "quoted string", cl.getOptionValue("b"));
}

@Test
public void testLongOptionQuoteHandlingWithoutStrip() throws Exception {
parser = DefaultParser.builder().setStripLeadingAndTrailingQuotes(false).build();
final String[] args = new String[] {"--bfile", "\"quoted string\""};

final CommandLine cl = parser.parse(options, args);

assertEquals("Confirm --bfile \"arg\" keeps quotes", "\"quoted string\"", cl.getOptionValue("b"));
}

@Test
public void testLongOptionWithEqualsQuoteHandlingWithStrip() throws Exception {
parser = DefaultParser.builder().setStripLeadingAndTrailingQuotes(true).build();
final String[] args = new String[] {"--bfile=\"quoted string\""};

final CommandLine cl = parser.parse(options, args);

assertEquals("Confirm --bfile=\"arg\" strips quotes", "quoted string", cl.getOptionValue("b"));
}

@Test
public void testLongOptionWithEqualsQuoteHandlingWithoutStrip() throws Exception {
parser = DefaultParser.builder().setStripLeadingAndTrailingQuotes(false).build();
final String[] args = new String[] {"--bfile=\"quoted string\""};

final CommandLine cl = parser.parse(options, args);

assertEquals("Confirm --bfile=\"arg\" keeps quotes", "\"quoted string\"", cl.getOptionValue("b"));
}

@Test
public void testBuilder() throws Exception {
parser = DefaultParser.builder()
.setStripLeadingAndTrailingQuotes(false)
.setAllowPartialMatching(false)
.build();
assertEquals(DefaultParser.class, parser.getClass());
}
}

0 comments on commit 93d13ca

Please sign in to comment.