-
Notifications
You must be signed in to change notification settings - Fork 418
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
Add abbreviation recognition for subcommands and options #1047
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1047 +/- ##
============================================
- Coverage 94.35% 94.26% -0.09%
- Complexity 443 466 +23
============================================
Files 2 3 +1
Lines 6482 6573 +91
Branches 1735 1757 +22
============================================
+ Hits 6116 6196 +80
- Misses 94 99 +5
- Partials 272 278 +6
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall very nice, thanks for working on this!
There may be some interesting edge cases when abbreviations are combined with case-insensitive mode or POSIX clustered options, I haven't given this much thought yet.
This currently does not work with case-insensitive mode, because the match function does not take it into consideration. Edit: Make methods in For POSIX options, the current implementation will prioritize full name, and in this case, also the abbreviation. For example, if we have three options: Also, if there are options I think the behaviour of this feature should be: |
Another way is to limit abbreviations to long options only (options starts with Edit: This seems to be the GNU way. |
Not sure how to Edit: With case-insensitivity, it may make sense to match prefix only, since it is impossible to trivially split words. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work!
I added some more comments.
Any thoughts on behaviour about case-insensitivity and POSIX options mentioned above? |
Bedtime for me. I’ll take another look tomorrow. |
I haven't had a chance yet to sit down and think clearly about the interaction between case insensitive mode, posix short options, negatable options and abbreviated options. It is possible that some combinations will always give counter-intuitive results. I would be fine with reducing scope, like leaving negatable options out of scope: for example, we could recognize the abbreviated original option, but the negatable form must always be specified fully and cannot be abbreviated. Just an idea. The examples you provided are useful to help list up and think through the possible combinations. I just need to find a block of time where I can focus on this. I may not be able to do so this weekend. Abbreviated commands are easier. One idea is to (initially?) only provide this feature for subcommands. |
Actually the negatable form options does work since 747720c. Feel free to take your time, I am not in a hurry. 😀 |
It makes sense to me that we only split blocks using hyphen (ignore camelCase) in case-insensitive mode. |
Also, if there are options -A, -B and -AB, if user inputs -AB, picocli will match -AB instead of -A and -B (full option name match has higher priority than POSIX cluster-options). So we can define a priority for options matching and let the users to avoid ambiguity. |
Package private is fine. :-) About the test, if we pass |
If we pass --AB then it causes a If we pass --a-b then it causes a Edit: Fixed an issue in |
If we pass --aB and --a-B then both options are matched. |
So the current impl will find exact match first, if there is exact match, then any ambiguity is not considered. Otherwise (no exact match), if there is ambiguity, a Edit: typo |
The latest code should have addressed most issues. |
Great, thank you! I’m not sure if I’ll be able to review today, I’ll try to get it done this weekend. |
I went ahead and merged the PR. |
Great, glad I could contribute to the picocli community! 😀 |
Currently using code snippet from @remkop #732 with minor changes.
For ease of debugging, the
AbbreviationMatcher
classis in separate file for now.This is a draft, no javadoc are added yet, and tests may not cover all cases.Feedbacks are welcomed!
This draft's goal is to resolve #10 and resolve #732.
Edit: All javadoc have been added, and diff coverage is 94.79%.