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

CLI-217: Optional partial matching #15

Merged
merged 5 commits into from Jul 31, 2017
Merged

CLI-217: Optional partial matching #15

merged 5 commits into from Jul 31, 2017

Conversation

rubin55
Copy link
Contributor

@rubin55 rubin55 commented Jun 23, 2017

At request of Gary, I (re)created an old patch against the current code base to enable partial matching to be set as optional. This fixes problems for people that have short options that, concatenated, also partial-match a long option.

For example:
-d, --debug
-e, --extract

foo -de

Is ambiguous in the case that partial matching is enabled. This patch allows a user to turn off partial matching in such a case.

@coveralls
Copy link

coveralls commented Jun 23, 2017

Coverage Status

Coverage decreased (-0.1%) to 96.122% when pulling 4f17a89 on rubin55:master into c246bd4 on apache:master.

@coveralls
Copy link

coveralls commented Jun 27, 2017

Coverage Status

Coverage increased (+0.04%) to 96.287% when pulling 3d9587c on rubin55:master into c246bd4 on apache:master.

@rubin55
Copy link
Contributor Author

rubin55 commented Jun 30, 2017

Is someone looking at this PR?

@britter
Copy link
Member

britter commented Jul 3, 2017

@rubin55 sorry, I'm currently burried in work. Maybe @chtompki or @kinow can have a look?

@@ -1,5 +1,4 @@
### https://raw.github.com/github/gitignore/f2ce448f2ba7a092da05482ceca99209127c0884/maven.gitignore

# maven
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure we want to change the .gitignore file. I would think these entries would be in one's global .gitignore file as the project itself does not generate these files

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you want me to undo that change? it's not a huge issue from my perspective, but no ish if you'd rather have it without!

Copy link
Member

Choose a reason for hiding this comment

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

Nah...I'll work around it...plus it's a change to an SVN project, so I've got to copy in your changes. That reminds me, maybe I'll take collections and promote it to being a git repo in the apache infrastructure.

Copy link
Member

Choose a reason for hiding this comment

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

Ha...pardon...wrong project. Nevermind about the copy bit. @britter just promoted CLI to git.

@chtompki
Copy link
Member

chtompki commented Jul 7, 2017

Yes aside from the .gitignore changes, this all looks quite reasonable.

@chtompki
Copy link
Member

chtompki commented Jul 7, 2017

Looking at CLI-217.patch, I was wondering if we shouldn't also include changes to PosixParser? The changes would be at https://github.com/apache/commons-cli/blob/master/src/main/java/org/apache/commons/cli/PosixParser.java#L127 and https://github.com/apache/commons-cli/blob/master/src/main/java/org/apache/commons/cli/PosixParser.java#L134 based on the bottom of that patch file. No worries if there is a distinct reason for not including those, just curious if you saw that in the patch.

@rubin55
Copy link
Contributor Author

rubin55 commented Jul 7, 2017

Yes, I did see that PosixParser was also a part of the patch, but I thought not to touch it since it's marked as Deprecated (It might not have been before, the patch was written a while ago. Personally I would not expect changes to deprecated classes as a user at least).

@rubin55
Copy link
Contributor Author

rubin55 commented Jul 21, 2017

Hey :-) Any progress on this one? Anything I can do to make it go forward? Greets!

@rubin55
Copy link
Contributor Author

rubin55 commented Jul 28, 2017

@chtompki Any progress on this one?

@chtompki
Copy link
Member

I'll try to get to it today.

@chtompki
Copy link
Member

Working on this now.

@chtompki
Copy link
Member

chtompki commented Jul 29, 2017

Upon reading CLI-216 and CLI-217, this pull request looks like it resolves CLI-216 and not CLI-217. I might be missing something though. @rubin55 - I'm definitely on board with this change. I just want to document it appropriately. Thoughts?

@rubin55
Copy link
Contributor Author

rubin55 commented Jul 29, 2017

@chtompki Well, 216 is about disabling concatenated options, and 217 is about disabling partial matching; my patch implements the latter, i.e, you still have concatenated options, but can opt to disable partial matching:

foo -ver
    !=
foo --version

So in the above, -ver could be a concatenated set of options, which, with the help of this patch, does not erronuously get interpreted as a partially matched long option 'version'.

@chtompki
Copy link
Member

Agreed, pardon my misread. Pulling this in now...only considering documentation on partial matching.

@asfgit asfgit merged commit 3d9587c into apache:master Jul 31, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants