Skip to content
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.

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

PHOENIX-6349 Add and use commons-cli to phoenix-thirdparty #2

Closed
wants to merge 2 commits into from

Conversation

stoty
Copy link
Contributor

@stoty stoty commented Jan 29, 2021

includes a patch that lets us disable the double quote removing "feature"

includes a patch that lets us disable the double quote removing "feature"
Copy link

@virajjasani virajjasani left a comment

Choose a reason for hiding this comment

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

Just a small question around patch generation that i am not much aware of, else +1 (non-binding)

Comment on lines +55 to +56
- currentOption.addValueForProcessing(Util.stripLeadingAndTrailingQuotes(token));
+ currentOption.addValueForProcessing(conditionallyStripLeadingAndTrailingQuotes(token));

Choose a reason for hiding this comment

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

This gets applied when we build phoenix-thirdparty?

Choose a reason for hiding this comment

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

Also, 254 in CLI-254 is somehow relevant in commons-cli or is it just a number that we provide in patch file?

Copy link
Contributor Author

@stoty stoty Jan 29, 2021

Choose a reason for hiding this comment

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

Yes. The pom was copied from hbase-thirdparty:hbase-shaded-protobuf with minimal changes.

The patch is a backport of my proposed fix for https://issues.apache.org/jira/browse/CLI-254
apache/commons-cli#58

to commons-cli 1.4

Copy link
Member

@joshelser joshelser left a comment

Choose a reason for hiding this comment

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

Looks great. A couple comments, but nothing that needs to be addressed immediately.

Comment on lines +56 to +58
<includes>
<include>**/**</include>
</includes>
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this implied?

Copy link
Contributor Author

@stoty stoty Jan 29, 2021

Choose a reason for hiding this comment

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

You're right.
I left it in when removing the exclusions when copying this from hbase-shaded-porotobuf.

src/main/java)-->
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-dependency-plugin</artifactId>
<version>3.1.1</version>
Copy link
Member

Choose a reason for hiding this comment

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

Would be good to pull versions into pluginManagement in the parent in a follow-on.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll update it in a few minutes.

@stoty
Copy link
Contributor Author

stoty commented Jan 29, 2021

Thanks for the reviews @virajjasani @joshelser

@stoty stoty closed this Jan 29, 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
3 participants