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

fix: Error when the user puts options before the subcommand #834

Closed
wants to merge 1 commit into from

Conversation

mrexodia
Copy link

These will be silently ignored due to a bug in picocli (#833)

Does this need localization for the error message itself?

@mrexodia mrexodia mentioned this pull request Aug 22, 2024
@mrexodia mrexodia changed the title Error when the user puts options before the subcommand fix: Error when the user puts options before the subcommand Aug 22, 2024
Copy link

codecov bot commented Aug 23, 2024

Codecov Report

Attention: Patch coverage is 0% with 5 lines in your changes missing coverage. Please review.

Project coverage is 65.84%. Comparing base (2c15c57) to head (b0563cb).

Files Patch % Lines
src/main/java/com/crowdin/cli/Cli.java 0.00% 5 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main     #834      +/-   ##
============================================
- Coverage     65.90%   65.84%   -0.05%     
  Complexity     1514     1514              
============================================
  Files           223      223              
  Lines          6160     6165       +5     
  Branches        932      933       +1     
============================================
  Hits           4059     4059              
- Misses         1562     1567       +5     
  Partials        539      539              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@andrii-bodnar andrii-bodnar left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution!

A few comments:

  • There could be options starting with -, it should also be handled correctly.
  • move the text to the resource bundle (resources/messages/messages.properties).

@andrii-bodnar
Copy link
Member

Honestly, it looks more like a hack. @yevheniyJ maybe you'll have some other ideas?

These will be silently ignored due to a bug in picocli (crowdin#833)
@mrexodia
Copy link
Author

Addressed your review points. I of course agree it's a hack, but this error really would have saved me a lot of debugging frustrations 😅

@yevheniyJ
Copy link
Collaborator

yevheniyJ commented Aug 23, 2024

First of all this fix won't help if option was passed in the middle of the command, e.g. cli.jar string --verbose list. Fix only considers first argument.

Second point: there is just no way to detect whether or not order is correct, especially where we have root commands with subcommands and multiple arguments with their values (or array of values).

The proper fix would be I guess to use inherit scopes for options https://picocli.info/#_inherited_options and bypass them from root to target subcommands. But this require extra research.

@mrexodia
Copy link
Author

I tried the inherited options, but this explodes in picocli because multiple classes inherit from GenericCommand and that creates duplicate options:

picocli.CommandLine$DuplicateOptionAnnotationsException: Option name '--no-progress' is used by both field boolean com.crowdin.cli.commands.picocli.GenericCommand.noProgress and field boolean com.crowdin.cli.commands.picocli.GenericCommand.noProgress
	at picocli.CommandLine$DuplicateOptionAnnotationsException.create(CommandLine.java:18698)
	at picocli.CommandLine$DuplicateOptionAnnotationsException.access$2900(CommandLine.java:18692)
	at picocli.CommandLine$Model$CommandSpec.addOption(CommandLine.java:6786)
	at picocli.CommandLine$Model$CommandSpec.add(CommandLine.java:6770)
	at picocli.CommandLine$Model$CommandSpec.addSubcommand(CommandLine.java:6601)
	at picocli.CommandLine$Model$CommandReflection.initSubcommands(CommandLine.java:11885)
	at picocli.CommandLine$Model$CommandReflection.extractCommandSpec(CommandLine.java:11850)
	at picocli.CommandLine$Model$CommandSpec.forAnnotatedObject(CommandLine.java:6392)
	at picocli.CommandLine.<init>(CommandLine.java:230)
	at picocli.CommandLine.toCommandLine(CommandLine.java:3635)
	at picocli.CommandLine.access$16700(CommandLine.java:148)
	at picocli.CommandLine$Model$CommandReflection.initSubcommands(CommandLine.java:11884)
	at picocli.CommandLine$Model$CommandReflection.extractCommandSpec(CommandLine.java:11850)
	at picocli.CommandLine$Model$CommandSpec.forAnnotatedObject(CommandLine.java:6392)
	at picocli.CommandLine.<init>(CommandLine.java:230)
	at picocli.CommandLine.<init>(CommandLine.java:224)
	at picocli.CommandLine.<init>(CommandLine.java:199)
	at com.crowdin.cli.commands.picocli.PicocliRunner.init(PicocliRunner.java:57)
	at com.crowdin.cli.commands.picocli.PicocliRunner.<init>(PicocliRunner.java:21)
	at com.crowdin.cli.commands.picocli.PicocliRunner.getInstance(PicocliRunner.java:26)
	at com.crowdin.cli.Cli.main(Cli.java:15)

That being said, I don't think it's very likely the a user will pass --no-color between sub commands. I'm used to other command line tools where the root options mentioned in --help will work before the first sub command, which is why I did it wrong.

@yevheniyJ
Copy link
Collaborator

It seems like if we allow to pass option in another position, then picocli won’t allow any other position. At least this is my impression. And there is no obvious fix. I think for now we won’t do anything since it’s anyway not the correct usage of our CLI and in documentation we multiple times showing how, with what commands, options, in which order, tool should be used.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants