-
Notifications
You must be signed in to change notification settings - Fork 422
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
Support case-insensitive (configurable) option name parsing #9
Comments
Hi! I'm considering to contribute to this project, and I'd like to know whether this issue is still opened to PR. It seems that this has not been implemented yet. If the issue is still valid, have you got any advice on how to implement this? |
Hi, thank you for considering contributing! About this particular feature, I haven’t analyzed what would be involved. I was also planning to add support for abbreviated options and commands in the near future. That feature and this feature both increase the probability that options are ambiguous, and we need to be careful about that. For example, picocli currently throws an Also, the There may be other areas where the expected behaviour needs to be clarified. |
Hello, @remkop ! I am evaluating different approaches to implement case-insensitive support and would love to hear some feedbacks.
|
Hello @ifsheldon and @NewbieOrange, I noticed you both submitted a pull request for the same issue. First, thank you both for helping making picocli a better library! 👍 I would like to make this feature work as follows:
Both proposals are part of the way there, but not fully. @ifsheldon and @NewbieOrange, how would you like to proceed? One idea is that you collaborate on a solution together, on a branch. Another option is that you work on different parts of the solution, and contribute each part separately. And there may be other ways to do this. Thoughts? |
Hi @remkop , thanks for your feedbacks! Since Also, since |
Hi @NewbieOrange, here is an example: Suppose that a case-insensitive command has an option The application may have code like Implementation-wise, we can modify the |
Currently my PR should have I have already updated my fork to match all the features you have mentioned, if there is anything missing, please let me know! Edit: The new test for case-insensitive options
|
Indeed we are working together to contribute. Since @NewbieOrange has proposed a complete solution, it is unnecessary to propose another one, so it's fine that you just check and merge his branch. |
@ifsheldon Understood. I agree, your PR is already helping clarify the requirements and improving the solution. Many thanks! 👍 @NewbieOrange I guess it makes sense for applications to query the Different topic: I don't think that the convenience methods Also, did you get a chance to add tests for case insensitive subcommands (and sub-subcommands)? |
@remkop The convenience methods are removed and tests for (sub-)subcommands are added! Edit: I also have changed |
* 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
No description provided.
The text was updated successfully, but these errors were encountered: