Skip to content

Another implementation of case-insensitive support for subcommands and options (#9) #1021

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.

Sign up for GitHub

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

Jump to bottom
Merged
remkop merged 36 commits into from
May 4, 2020
Merged

Another implementation of case-insensitive support for subcommands and options (#9) #1021

remkop merged 36 commits into from
May 4, 2020

Conversation

@NewbieOrange
Copy link
Contributor

@NewbieOrange NewbieOrange commented May 2, 2020 •
edited
Loading

This is an alternative PR to #1020 , follows a somewhat different approach, trying to solve #9 .

Currently it is only a draft, feedbacks are welcomed. Tests are partially added, not fully tested.

No previous tests are broken due to the new changes.

@NewbieOrange NewbieOrange changed the title Another implementation of case-insensitive support for options, command and mixins Another implementation of case-insensitive support for options, command and mixins (#9) May 2, 2020
@NewbieOrange NewbieOrange changed the title Another implementation of case-insensitive support for options, command and mixins (#9) Another implementation of case-insensitive support for options, commands and mixins (#9) May 2, 2020
NewbieOrange added 12 commits May 3, 2020 10:52
@NewbieOrange
Remove mixins from case-insensitivity, add posix options
2a0f279
@NewbieOrange
Add caseInsensitive getter and setter in CommandLine
1022cf9
@NewbieOrange
Fix exception when setting case-insensitive twice
ef7b53b
@NewbieOrange
Add subcommands and options case-insensitive getters and setters
2d7b713
@NewbieOrange
Improve javadoc for case-insensitivity getter and setter
9fbe4fc
@NewbieOrange
Improve javadoc for LinkedCaseAwareMap
4e8d181
@NewbieOrange
Fix containsKey not consider chars in LinkedCaseAwareMap
f9aef1e
@NewbieOrange
LinkedCaseAwareMap now extends AbstractMap to reduce size
4fa0d70
@NewbieOrange
Simplify class checking since both String and Character are final
9accd0c
@NewbieOrange
Change LinkedCaseWareMap default locale to ENGLISH
71684e9 
Since console commands and options should be in ascii characters, the case-insensitive only consider mapping of english characters.
@NewbieOrange
Add Override annotations in LinkedCaseAwareMap
4e6567d
@NewbieOrange
Fix isCaseInsensitiveOptions not return the correct value
1939760
@NewbieOrange NewbieOrange changed the title Another implementation of case-insensitive support for options, commands and mixins (#9) Another implementation of case-insensitive support for subcommands and options (#9) May 3, 2020
NewbieOrange
NewbieOrange commented May 3, 2020
View reviewed changes
NewbieOrange
NewbieOrange commented May 3, 2020
View reviewed changes
@remkop
Copy link
Owner

remkop commented May 3, 2020

I’ll do another check tomorrow.

Can you think of tests for any edge cases introduced by this change? Like ambiguities that didn’t exist before? I can think of cases where the combination of POSIX options now resembles a long option, or a long option with the same name as a subcommand, it may be good to have tests for those.

Can anyone think of other tests?

@remkop
Copy link
Owner

remkop commented May 4, 2020

Great, thank you! I think we are getting close.

Codecov reports that code coverage will go down if this PR is merged.

Can you add tests for the code changes that are not or only partially covered by the tests yet?

@NewbieOrange
Copy link
Contributor Author

NewbieOrange commented May 4, 2020 •
edited
Loading

Since LinkedCaseAwareMap is a private class (and probably should be), some code are not called by picocli (no any public method can reach these lines) and they are there for potential future error-checking.

I could add some tests using reflection on the custom map, but does Codecov count that as 'coverage'?

@NewbieOrange
Copy link
Contributor Author

NewbieOrange commented May 4, 2020

For example, uncovered lines are checks for non-convertible classes, null keys, removing elements, which are not called by any other picocli code.

@NewbieOrange
Copy link
Contributor Author

NewbieOrange commented May 4, 2020 •
edited
Loading

Now the coverage shall increase after this PR is merged! 😃

Edit: The Codecov report seems to be incorrectly comparing wrong commits? The diff coverage is n/a, meh.

@NewbieOrange
Copy link
Contributor Author

NewbieOrange commented May 4, 2020 •
edited
Loading

Pushing a small commit to try to trigger Codecov again for a correct report.

Edit: The full report is now working, no new miss / partial are introduced, coverage +0.07%.
Codecov Report

@remkop
Copy link
Owner

remkop commented May 4, 2020

That is great! Thank you very much.
This is ready to be merged.

Do you think you will be able to add some documentation to the parser configuration section of the user manual (in the docs/index.adoc file)? If not, I can do it. 😄

@remkop remkop added this to the 4.3 milestone May 4, 2020
@remkop remkop added type: API 🔌 type: enhancement ✨ theme: parser An issue or change related to the parser labels May 4, 2020
@NewbieOrange
Copy link
Contributor Author

NewbieOrange commented May 4, 2020

I am not a native English speaker and not really familiar with asciidoc 😅.

It would probably be better if you could add those documentation.

@remkop
Copy link
Owner

remkop commented May 4, 2020

okay, no worries.
Thank you for the contribution!

@jerrylususu
Copy link
Contributor

jerrylususu commented May 4, 2020 •
edited
Loading

okay, no worries.
Thank you for the contribution!

I'm willing to help on writing documentation. Is docs/index.adoc the only file need to change? However, I need some time to look into the changes and figure out how to document it clearly.

@remkop
Copy link
Owner

remkop commented May 4, 2020

Yes index.adoc is the main file for documentation.
I’ll also make an entry in RELEASE-NOTES.md.

To test, run:

gradlew ascii

Then check the generated HTML in build/docs/html5/index.html
Thanks again for helping to make picocli a better library!

@remkop remkop merged commit 3cce6da into remkop:master May 4, 2020
remkop added a commit that referenced this pull request May 4, 2020
@remkop
[#9][#1021][#1020] update release notes
22e5fc4
remkop added a commit that referenced this pull request May 4, 2020
@remkop
[#9][#1021] Renamed getters and setters; minor javadoc changes
9662c3b
remkop added a commit that referenced this pull request May 4, 2020
@remkop
[#9][#1021] added tests
8dd32f5
remkop added a commit that referenced this pull request May 4, 2020
@remkop
[#9][#1021][#1020][#1023] various improvements:
e89ff22 
* update RELEASE-NOTES.md
* simplify user manual section on Case Sensitivity
* throw DuplicateNameException instead of IllegalStateException in CaseInsensitiveLinkedMap
* throw DuplicateNameException instead of InitializationException in addSubcommand
* suppress warnings for unchecked cast
* minor improvements to POSIXOptionResembleDemo
remkop added a commit that referenced this pull request May 4, 2020
@remkop
[#9][#1021][#1020][#1023] renamed LinkedCaseAwareMap to CaseAwareLink…
8dff308 
…edMap; made class package-protected to facilitate testing
remkop added a commit that referenced this pull request May 5, 2020
@remkop
[#9][#1021][#1020][#1023] fix typo in RELEASE-NOTES.md
a03d2a8
remkop added a commit that referenced this pull request May 23, 2020
@remkop
[#9][#1021][#1020][#1023][#154] DOC update user manual for case-insen…
37f581b 
…sitive mode
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

theme: parser An issue or change related to the parser type: API 🔌 type: enhancement ✨

Projects

None yet

Milestone

4.3

Development

Successfully merging this pull request may close these issues.

4 participants

@NewbieOrange @remkop @codecov-io @jerrylususu