-
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
Built-in Help being used on subcommands, still case-sensitive even if case-sensitivity disabled #1156
Comments
Thank you for raising this! Thank you for the kind words about picocli! Can I ask: what is it that you like about picocli, and what would you do if picocli didn't exist? |
I like the simplicity of Pico, and how elegant the structure of the code is when using the subcommand structure. Also the availability of features that I'm looking for (I've had bad experiences in the past with Java libs where the answer to how to make a somewhat basic feature was "extend this class but a bunch of the stuff in it is If Pico didn't exist, I probably would've either used gone through with my plan of manually taking the user's input and handling the args array manually (ugh), or gone to find another CLI library. Pico was actually a suggestion from a co-worker, when I mentioned my plan for input parsing he said that trying to do it myself was insane and listed a few Java CLI libraries. Thanks for this interest in the user experience! I'll circle back to a PR for this later. |
Thanks for the quick feedback! |
It seems the issue is related to how CommandLine returns the subcommands. CommnadLine.java: Line 367
public Map<String, CommandLine> getSubcommands() {
return new LinkedHashMap<String, CommandLine>(getCommandSpec().subcommands());
} A new LinkedHashMap is returned, while the impl actually uses the case insensitive-able map now. Now there are two options, one is changing the bahaviour of CommandLine.getSubcommand() (and options?) to return a case aware map (or simply the unmodifiable-wrap from CommandSepc.subcommands()), another is to make HelpCommand get subcommand from CommandSpec directly. I think we should probably correct the bahaviour of CommandLine.getSubcommand(). Should it return the unmodifiable map (who will want to make changes to the map since it is not affecting anything), or keep the current "return a modifiable copy" state for (maybe) backward-compatibility? I will make a PR after hearing your feedbacks. @remkop Edit: typo |
@NewbieOrange thanks for the analysis!
But it may also be a good idea to return a I prefer to keep the current behaviour of returning a modifiable map. I don't want to break existing applications that rely on the fact that it is modifiable. So, we probably need to add a
|
CommandLine.java: Line 14078
public static class Help {
... ...
private final Map<String, Help> allCommands = new LinkedHashMap<String, Help>();
private final Map<String, Help> visibleCommands = new LinkedHashMap<String, Help>();
} |
This is now fixed thanks to @NewbieOrange's pull request. The fix will be in the next release. Thank you @NewbieOrange for the pull request and thank you @Shadowsoftime99 for raising this! |
Picocli 4.5.2, which contains the fix for this issue, has been released. |
Hey, first of all I'll start with saying that I'm just about done using Pico for a work project and it's been absolutely amazing.
One small inconsistency that I still have left (and just put down in a "common issues" wiki section) is that even with case-sensitivity disabled, for example:
[Tools] help SUBCOMMAND
Gives an "Unknown subcommand" error, despite "subcommand" existing with different capitalization. This is inconsistent with everything else, and the only thing that the user needs to be case-sensitive for, as:
[Tools] HELP subcommand
[Tools] SUBCOMMAND
[Tools] subcommand -OpTiOn
All work perfectly fine. It's just when trying to run help on a subcommand, the subcommand's name must be perfect.
I have a hunch that the issue lies in the implementation of the Help command, but am not sure. I'm open to working on this myself if I have some free time in a few weeks, but figured I'd put this issue out there now.
The text was updated successfully, but these errors were encountered: