Fix for CLI-271#9
Conversation
Add some Tests for coverage
britter
left a comment
There was a problem hiding this comment.
Looks promising, but there are some unrelated changes. I think we should discuss the method returning a Properties object in a separated thread.
| 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'); |
There was a problem hiding this comment.
please use camel case for variable names
| * even if the option doesn't exists | ||
| * @since 1.4 | ||
| */ | ||
| public Properties getOptionProperties(Option option) |
There was a problem hiding this comment.
Why is a Properties instance returned and not a Map as it says in the first line of the JavaDoc?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Okay, I see. Makes sense make this symmetric with the String param based version.
| } | ||
| } | ||
|
|
||
| return props; |
There was a problem hiding this comment.
This method does not belong to CLI-271. We should better create a separate jira to be able to discuss its usefulness separately.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I see, thank you for the clarification.
| } | ||
|
|
||
| @Test | ||
| public void testHashCode() { |
There was a problem hiding this comment.
Can you explain how I remove this from the PR? Must I create a new commit/PR?
There was a problem hiding this comment.
Since, I have to apply this via SVN, I can remove it for you.
Add the posibility to use Options as a parameter for fetching values from the CommandLine.