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

Multiple short options not working anymore #757

Closed
bjorntj opened this issue May 30, 2023 · 9 comments
Closed

Multiple short options not working anymore #757

bjorntj opened this issue May 30, 2023 · 9 comments
Labels
type/bug Is a bug report
Milestone

Comments

@bjorntj
Copy link

bjorntj commented May 30, 2023

I have a method with the following:

@ShellOption(valueProvider = ServerValueProvider.class, help = "Choose which server to connect to", value = "-s") @NotBlank String server, @ShellOption(help = "Comma separated list of filegroups to update, no spaces", value = "-f") @NotBlank String filegroups, @ShellOption(help = "Add parameter to commit the update", value = "-c") boolean commit

If I run the app, I get the following error:

install-vareviews -s xlbygg -f j41
2000E:(pos 0): Missing mandatory option, longnames='', shortnames='f'

And if I switch the arguments, I get the following:

install-vareviews -f j14 -s xlbygg
2000E:(pos 0): Missing mandatory option, longnames='', shortnames='s'

So it seems like it does not catch the second argument?

This works fine using Spring Shell 3.0.x but not using version 3.1.0.

@github-actions github-actions bot added the status/need-triage Team needs to triage and take a first look label May 30, 2023
@tincore
Copy link

tincore commented May 30, 2023

There is also the case of using short names and failing to calculate the arity

argument input, i with arity 1

commandX -i arg0 -o arg1

Fails reporting that -i only supports one argument (thinking that -o arg1 are more arguments)

commandX -i arg0 --output arg1

Succeeds.

It has to be something recent. In the project I'm working atm I've been jumping from 3.0.x to 3.1.0-RCx a few times during last weeks and all my automated tests passed without changes (before 3.1.0)

@jvalkeal
Copy link
Contributor

Thanks reporting this. I'll post more info when I find what happened as I see the same issue.

@jvalkeal jvalkeal added type/bug Is a bug report and removed status/need-triage Team needs to triage and take a first look labels May 30, 2023
@jvalkeal jvalkeal added this to the 3.1.1 milestone May 30, 2023
@jvalkeal jvalkeal changed the title After upgrading to Spring shell 3.1.0, arguments is not working anymore Multiple short options not working anymore May 31, 2023
jvalkeal added a commit to jvalkeal/spring-shell that referenced this issue May 31, 2023
- Add short options as valid tokens in a `CommandModel` so that Lexer
  create tokens with accurate info and doesn't then cascade this issue
  in Ast and Parser.
- Previously with a command `command -a aaa -b bbb` tokenisation resulted
  `COMMAND OPTION ARGUMENT ARGUMENT ARGUMENT` with multiple short options
  while it should have been `COMMAND OPTION ARGUMENT OPTION ARGUMENT`.
- Relates spring-projects#757
@jvalkeal
Copy link
Contributor

I've pushed a commit which hopefully fixes this issue. I'll keep issue open if you want to try snapshots and add a comment.

This bug was in a parser tokenisation which i.e. with install-vareviews -s xlbygg -f j41 created tokens of

TokenType.COMMAND TokenType.OPTION TokenType.ARGUMENT TokenType.ARGUMENT TokenType.ARGUMENT

while it should have been:

TokenType.COMMAND TokenType.OPTION TokenType.ARGUMENT TokenType.OPTION TokenType.ARGUMENT

then a parser got confused.

@tincore
Copy link

tincore commented Jun 1, 2023

I can confirm that with latest snapshot all my tests pass. In my case I can go from 3.0.x to 3.1.x without code changes.

@tincore
Copy link

tincore commented Jun 1, 2023

Still there are other related errors!

:>get-value -k MY_KEY
2004E:(pos 0): Too many arguments for option 'key', requires at most '1'

:>get-value --key MY_KEY

correct response

@jvalkeal
Copy link
Contributor

jvalkeal commented Jun 1, 2023

@tincore I can't reproduce. Can you show a bit more how that command is defined.

@tincore
Copy link

tincore commented Jun 1, 2023

Sure. Maybe its the position argument or that redundancy with arity and required. This is one of the cases were it would be convenient to be able to autocomplete based on position to avoid typing the option name.

 .command("get-value")
            .withAlias()
            .command("g")
            .and()
            .withOption()
            .longNames("key")
            .shortNames('k')
            .label("PROPERTY_KEY")
            .description("Properties key. Possible values " + Arrays.stream(PropertiesGetKey.values()).map(Enum::toString).toList())
            .type(PropertiesGetKey.class)  // Its an enum
            .required()
            .arity(CommandRegistration.OptionArity.EXACTLY_ONE)
            .position(0)
            .completion(enumValueProvider::complete)
            .and()
            .withOption()
            .longNames(OPTION_VERBOSE)
            .shortNames('v')
            .label("BOOLEAN")
            .description("Verbose output")
            .type(Boolean.class)
            .and()
        

@jvalkeal
Copy link
Contributor

jvalkeal commented Jun 1, 2023

Not sure as copying that format doesn't make things to fail. Would you be able to make a sample I could run?

@tincore
Copy link

tincore commented Jun 1, 2023

I was building the sample and I upgraded again to latest 3.1.1-SNAPSHOT and now it seems to work fine! Both the sample and the original code that was failing before.

Thanks for all! :)

Btw. If you have a chance. Check that PR that I left some time ago ;) It makes for better auto-completion for filesystem paths.

@jvalkeal jvalkeal closed this as completed Jun 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/bug Is a bug report
Projects
None yet
Development

No branches or pull requests

3 participants