Skip to content

Fix for CLI-271 #9

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.

Sign up for GitHub

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

Closed
laeubi wants to merge 1 commit into from
Closed

Fix for CLI-271 #9

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
169 changes: 146 additions & 23 deletions src/main/java/org/apache/commons/cli/CommandLine.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,18 @@ protected CommandLine()
{
// nothing to do
}

/**
* Query to see if an option has been set.
*
* @param opt the option to check
* @return true if set, false if not
* @since 1.4
*/
public boolean hasOption(Option opt)
{
return options.contains(opt);
}

/**
* Query to see if an option has been set.
Expand All @@ -64,9 +76,9 @@ protected CommandLine()
*/
public boolean hasOption(String opt)
{
return options.contains(resolveOption(opt));
return hasOption(resolveOption(opt));
}

/**
* Query to see if an option has been set.
*
Expand Down Expand Up @@ -98,39 +110,87 @@ public Object getOptionObject(String opt)
return null;
}
}

/**
* Return a version of this <code>Option</code> converted to a particular type.
*
* @param opt the name of the option
* @param option the name of the option
* @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.2
* @since 1.4
*/
public Object getParsedOptionValue(String opt) throws ParseException
public Object getParsedOptionValue(Option option) throws ParseException
{
String res = getOptionValue(opt);
Option option = resolveOption(opt);

if (option == null || res == null)
if (option == null)
{
return null;
}
String res = getOptionValue(option);
if (res == null)
{
return null;
}

return TypeHandler.createValue(res, option.getType());
}

/**
* Return a version of this <code>Option</code> converted to a particular type.
*
* @param opt the name of the option
* @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.2
*/
public Object getParsedOptionValue(String opt) throws ParseException
{
return getParsedOptionValue(resolveOption(opt));
}

/**
* Return a version of this <code>Option</code> converted to a particular type.
*
* @param opt the name of the option
* @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.2
*/
public Object getParsedOptionValue(char opt) throws ParseException
{
return getParsedOptionValue(String.valueOf(opt));
}

/**
* Return the <code>Object</code> type of this <code>Option</code>.
*
* @deprecated due to System.err message. Instead use getParsedOptionValue(char)
* @param opt the name of the option
* @return the type of opt
*/
public Object getOptionObject(char opt)
{
return getOptionObject(String.valueOf(opt));
}

/**
* Retrieve the first argument, if any, of this option.
*
* @param option the name of the option
* @return Value of the argument if option is set, and has an argument,
* otherwise null.
* @since 1.4
*/
public String getOptionValue(Option option)
{
if (option == null)
{
return null;
}
String[] values = getOptionValues(option);
return (values == null) ? null : values[0];
}

/**
* Retrieve the first argument, if any, of this option.
Expand All @@ -141,9 +201,7 @@ public Object getOptionObject(char opt)
*/
public String getOptionValue(String opt)
{
String[] values = getOptionValues(opt);

return (values == null) ? null : values[0];
return getOptionValue(resolveOption(opt));
}

/**
Expand All @@ -157,29 +215,42 @@ public String getOptionValue(char opt)
{
return getOptionValue(String.valueOf(opt));
}

/**
* Retrieves the array of values, if any, of an option.
*
* @param opt string name of the option
* @param option string name of the option
* @return Values of the argument if option is set, and has an argument,
* otherwise null.
* @since 1.4
*/
public String[] getOptionValues(String opt)
public String[] getOptionValues(Option option)
{
List<String> values = new ArrayList<String>();

for (Option option : options)
for (Option processedOption : options)
{
if (opt.equals(option.getOpt()) || opt.equals(option.getLongOpt()))
if (processedOption.equals(option))
{
values.addAll(option.getValuesList());
values.addAll(processedOption.getValuesList());
}
}

return values.isEmpty() ? null : values.toArray(new String[values.size()]);
}

/**
* Retrieves the array of values, if any, of an option.
*
* @param opt string name of the option
* @return Values of the argument if option is set, and has an argument,
* otherwise null.
*/
public String[] getOptionValues(String opt)
{
return getOptionValues(resolveOption(opt));
}

/**
* Retrieves the option object given the long or short option as a String
*
Expand Down Expand Up @@ -216,6 +287,22 @@ public String[] getOptionValues(char opt)
{
return getOptionValues(String.valueOf(opt));
}

/**
* Retrieve the first argument, if any, of an option.
*
* @param option name of the option
* @param defaultValue is the default value to be returned if the option
* is not specified
* @return Value of the argument if option is set, and has an argument,
* otherwise <code>defaultValue</code>.
* @since 1.4
*/
public String getOptionValue(Option option, String defaultValue)
{
String answer = getOptionValue(option);
return (answer != null) ? answer : defaultValue;
}

/**
* Retrieve the first argument, if any, of an option.
Expand All @@ -228,9 +315,7 @@ public String[] getOptionValues(char opt)
*/
public String getOptionValue(String opt, String defaultValue)
{
String answer = getOptionValue(opt);

return (answer != null) ? answer : defaultValue;
return getOptionValue(resolveOption(opt), defaultValue);
}

/**
Expand All @@ -246,6 +331,44 @@ public String getOptionValue(char opt, String defaultValue)
{
return getOptionValue(String.valueOf(opt), defaultValue);
}

/**
* Retrieve the map of values associated to the option. This is convenient
* for options specifying Java properties like <tt>-Dparam1=value1
* -Dparam2=value2</tt>. The first argument of the option is the key, and
* the 2nd argument is the value. If the option has only one argument
* (<tt>-Dfoo</tt>) it is considered as a boolean flag and the value is
* <tt>"true"</tt>.
*
* @param option name of the option
* @return The Properties mapped by the option, never <tt>null</tt>
* even if the option doesn't exists
* @since 1.4
*/
public Properties getOptionProperties(Option option)

@britter britter Mar 15, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is a Properties instance returned and not a Map as it says in the first line of the JavaDoc?

@laeubi laeubi Mar 17, 2017 •
edited
Loading

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just copied the String variant of this including the javadoc, so I assume it was changed some time in the past and the java doc was not changed. In fact Properties extends Hashtable and that implements Map so Properties are also a Map.

@britter britter Mar 25, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I see. Makes sense make this symmetric with the String param based version.

{
Properties props = new Properties();

for (Option processedOption : options)
{
if (processedOption.equals(option))
{
List<String> values = processedOption.getValuesList();
if (values.size() >= 2)
{
// use the first 2 arguments as the key/value pair
props.put(values.get(0), values.get(1));
}
else if (values.size() == 1)
{
// no explicit value, handle it as a boolean
props.put(values.get(0), "true");
}
}
}

return props;

@britter britter Mar 15, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method does not belong to CLI-271. We should better create a separate jira to be able to discuss its usefulness separately.

@laeubi laeubi Mar 17, 2017

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does it not belong to CLI271? Its an adoption of the previous String variant (what is now delegating to this method) that I adjusted a bit to remove the requirements of string matching.

@britter britter Mar 25, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, thank you for the clarification.

}

/**
* Retrieve the map of values associated to the option. This is convenient
Expand Down
67 changes: 67 additions & 0 deletions src/test/java/org/apache/commons/cli/CommandLineTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@

import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertNotNull;
import static org.junit.Assert.assertNull;

import java.util.Properties;

Expand Down Expand Up @@ -49,6 +50,31 @@ public void testGetOptionProperties() throws Exception

assertEquals("property with long format", "bar", cl.getOptionProperties("property").getProperty("foo"));
}

@Test
public void testGetOptionPropertiesWithOption() throws Exception
{
String[] args = new String[] { "-Dparam1=value1", "-Dparam2=value2", "-Dparam3", "-Dparam4=value4", "-D", "--property", "foo=bar" };

Options options = new Options();
Option option_D = OptionBuilder.withValueSeparator().hasOptionalArgs(2).create('D');

@britter britter Mar 15, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please use camel case for variable names

Option option_property = OptionBuilder.withValueSeparator().hasArgs(2).withLongOpt("property").create();
options.addOption(option_D);
options.addOption(option_property);

Parser parser = new GnuParser();
CommandLine cl = parser.parse(options, args);

Properties props = cl.getOptionProperties(option_D);
assertNotNull("null properties", props);
assertEquals("number of properties in " + props, 4, props.size());
assertEquals("property 1", "value1", props.getProperty("param1"));
assertEquals("property 2", "value2", props.getProperty("param2"));
assertEquals("property 3", "true", props.getProperty("param3"));
assertEquals("property 4", "value4", props.getProperty("param4"));

assertEquals("property with long format", "bar", cl.getOptionProperties(option_property).getProperty("foo"));
}

@Test
public void testGetOptions()
Expand Down Expand Up @@ -76,6 +102,47 @@ public void testGetParsedOptionValue() throws Exception {
assertEquals(123, ((Number) cmd.getParsedOptionValue("i")).intValue());
assertEquals("foo", cmd.getParsedOptionValue("f"));
}

@Test
public void testGetParsedOptionValueWithChar() throws Exception {
Options options = new Options();
options.addOption(Option.builder("i").hasArg().type(Number.class).build());
options.addOption(Option.builder("f").hasArg().build());

CommandLineParser parser = new DefaultParser();
CommandLine cmd = parser.parse(options, new String[] { "-i", "123", "-f", "foo" });

assertEquals(123, ((Number) cmd.getParsedOptionValue('i')).intValue());
assertEquals("foo", cmd.getParsedOptionValue('f'));
}

@Test
public void testGetParsedOptionValueWithOption() throws Exception {
Options options = new Options();
Option opt_i = Option.builder("i").hasArg().type(Number.class).build();
Option opt_f = Option.builder("f").hasArg().build();
options.addOption(opt_i);
options.addOption(opt_f);

CommandLineParser parser = new DefaultParser();
CommandLine cmd = parser.parse(options, new String[] { "-i", "123", "-f", "foo" });

assertEquals(123, ((Number) cmd.getParsedOptionValue(opt_i)).intValue());
assertEquals("foo", cmd.getParsedOptionValue(opt_f));
}

@Test
public void testNullhOption() throws Exception {
Options options = new Options();
Option opt_i = Option.builder("i").hasArg().type(Number.class).build();
Option opt_f = Option.builder("f").hasArg().build();
options.addOption(opt_i);
options.addOption(opt_f);
CommandLineParser parser = new DefaultParser();
CommandLine cmd = parser.parse(options, new String[] { "-i", "123", "-f", "foo" });
assertNull(cmd.getOptionValue((Option)null));
assertNull(cmd.getParsedOptionValue((Option)null));
}

@Test
public void testBuilder()
Expand Down
8 changes: 8 additions & 0 deletions src/test/java/org/apache/commons/cli/OptionTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@

import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertNotEquals;
import static org.junit.Assert.assertNotSame;
import static org.junit.Assert.assertTrue;

Expand Down Expand Up @@ -71,6 +72,13 @@ public void testClone()
assertEquals(0, a.getValuesList().size());
assertEquals(2, b.getValues().length);
}

@Test
public void testHashCode() {

@britter britter Mar 15, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, but unrelated to CLI-271

@laeubi laeubi Mar 17, 2017

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain how I remove this from the PR? Must I create a new commit/PR?

@britter britter Mar 25, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since, I have to apply this via SVN, I can remove it for you.

assertNotEquals(Option.builder("test").build().hashCode(), Option.builder("test2").build().hashCode()) ;
assertNotEquals(Option.builder("test").build().hashCode(), Option.builder().longOpt("test").build().hashCode()) ;
assertNotEquals(Option.builder("test").build().hashCode(), Option.builder("test").longOpt("long test").build().hashCode()) ;
}

private static class DefaultOption extends Option
{
Expand Down
Loading