Skip to content

Incorrect completion for options #495

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

Closed
bmiller1009 opened this issue Aug 1, 2022 · 2 comments · Fixed by #499
Closed

Incorrect completion for options #495

bmiller1009 opened this issue Aug 1, 2022 · 2 comments · Fixed by #499
Labels
for/backport For backporting type/bug Is a bug report
Milestone

Comments

@bmiller1009
Copy link

I'm testing out the new 2.1.0 release and it looks like the ValueProviderSupport class has been deprecated. I have a flow where I have multiple parameters which need a value provider and the suggested value from the first returned and used to filter the value provider for the second.

Currently tabbing on ValueProvider for the first parameter returns the correctly it also incorrectly returns both parameters in addition to the provided value. The second ValueProvider seems never to be triggered.

I am using Kotlin, and my method signature would look something like this:

@ShellMethod("Get details")
fun getDetails(
@ShellOption(valueProvider = TestValueProvider1::class) test1: String,
@ShellOption(valueProvider = TestValueProvider2::class) test2: String
)

In the 2.0.0 API I used ValueProviderSupport and this gave me exactly what I needed. What is the correct way to achieve this now in 2.1.x?

@bmiller1009 bmiller1009 changed the title Replacement for ValueProviderSupport in the 2.1.x API Help: Replacement for ValueProviderSupport in the 2.1.x API Aug 1, 2022
@jvalkeal
Copy link
Contributor

jvalkeal commented Aug 3, 2022

This feels a regression from a rework. We moved closer to jline what comes for completion and getting both parameters is coming from there which we need to fix as it's wrong and annoying.

Thanks for reporting as I now see the issue you mention about second provider not getting called if first argument is already completed and placed in a command line.

@jvalkeal jvalkeal added the type/bug Is a bug report label Aug 3, 2022
@jvalkeal jvalkeal changed the title Help: Replacement for ValueProviderSupport in the 2.1.x API Incorrect completion for options Aug 4, 2022
@jvalkeal jvalkeal added the for/backport For backporting label Aug 4, 2022
@jvalkeal jvalkeal added this to the 3.0.0-M1 milestone Aug 4, 2022
jvalkeal added a commit to jvalkeal/spring-shell that referenced this issue Aug 4, 2022
- This commit fixes two issues.
- Firstly complete with correct option as existing bug was
  to wrongly always complete with first option which used
  wrong provider.
- Secondly filter out duplicate option proposals giving better
  result when options is already in place.
- Fixes spring-projects#495
jvalkeal added a commit that referenced this issue Aug 4, 2022
- This commit fixes two issues.
- Firstly complete with correct option as existing bug was
  to wrongly always complete with first option which used
  wrong provider.
- Secondly filter out duplicate option proposals giving better
  result when options is already in place.
- Fixes #495
jvalkeal added a commit that referenced this issue Aug 4, 2022
- This commit fixes two issues.
- Firstly complete with correct option as existing bug was
  to wrongly always complete with first option which used
  wrong provider.
- Secondly filter out duplicate option proposals giving better
  result when options is already in place.
- Backport #495
- Fixes #498

(cherry picked from commit a143d25)
@PakhomovAlexander
Copy link

Hi @bmiller1009, I have the same issue with ValueProviderSupport. I wonder if you could share with me your solution for using the new API. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
for/backport For backporting type/bug Is a bug report
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants