Skip to content

CLI-254: "test" gets parsed as test, quotes die :-(#58

Merged
garydgregory merged 2 commits into
apache:masterfrom
stoty:CLI-254
Oct 17, 2021
Merged

CLI-254: "test" gets parsed as test, quotes die :-(#58
garydgregory merged 2 commits into
apache:masterfrom
stoty:CLI-254

Conversation

@stoty

@stoty stoty commented Jan 29, 2021

Copy link
Copy Markdown
Contributor

add a new constructor parameter to DefaultParser that disables quote
processing on option arguments

@coveralls

coveralls commented Jan 29, 2021

Copy link
Copy Markdown

Coverage Status

Coverage increased (+0.07%) to 96.023% when pulling 93d13ca on stoty:CLI-254 into 6c94af3 on apache:master.

@garydgregory

Copy link
Copy Markdown
Member

-1: Changing the default behavior is a no go.

@stoty

stoty commented Oct 6, 2021

Copy link
Copy Markdown
Contributor Author

@garydgregory But this doesn't change the default behaviour.

It is adding a new parameter to a new constructor, that must be explicitly set to false to enable the sane behavior.
You get the old broken behaviour with the old constructors.

@stoty

stoty commented Oct 8, 2021

Copy link
Copy Markdown
Contributor Author

Rebased and added the Builder to DefaultParser.

@garydgregory garydgregory left a comment

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.

@stoty
Thank you for the updated PR. Please see my comments scattered throughout.

}

/**
* Create a new DefaultParser instance with the specified partial matching and quote

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.

"Create" -> "Creates"

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done

Comment thread src/main/java/org/apache/commons/cli/DefaultParser.java
Comment thread src/main/java/org/apache/commons/cli/DefaultParser.java
* @since 1.5
*/
public static final class Builder
{

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.

Place { at the end of the previous line.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done.

Comment thread src/main/java/org/apache/commons/cli/DefaultParser.java
/**
* 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

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.

Odd start for a sentence. Sentences should start with a capitalized word.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Rewritten

*
* 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.

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.

Odd formatting where "ie." is a line by itself. Better to spell it out anyway.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Rewritten.

}

/**
* Constructs an DefaultParser with the values declared by this {@link Builder}.

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.

In general, a build methods "Builds", a contructor "Constructs", so "Constructs" -> "Builds".

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Rewritten.

@Test
public void testLongOptionWithEqualsQuoteHandling() throws Exception
{
assumeTrue(parser instanceof DefaultParser);

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 do all these methods say assumeTrue(parser instanceof DefaultParser);?

If these tests are only for DefaultParser, then put the new tests in the proper subclass.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done.

@stoty

stoty commented Oct 12, 2021

Copy link
Copy Markdown
Contributor Author

While working on this, I discovered that quote handlings needs to be a tri-valued parameter.
I've changed the parameter into a Boolean, with null representing the old inconsistent behaviour.
This way backwards compatibility is kept.

@garydgregory garydgregory merged commit 4417394 into apache:master Oct 17, 2021
asfgit pushed a commit that referenced this pull request Oct 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants