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

[#1466] Fix autocomplete for aliased subcommands #1467

Merged
merged 5 commits into from Nov 24, 2021
Merged

[#1466] Fix autocomplete for aliased subcommands #1467

merged 5 commits into from Nov 24, 2021

Conversation

rsenden
Copy link
Contributor

@rsenden rsenden commented Nov 17, 2021

This PR fixes #1466

@remkop
Copy link
Owner

remkop commented Nov 17, 2021

This is great, thank you!
Can you add a test for this, or modify one of the existing tests?

@remkop remkop added this to the 4.6.3 milestone Nov 17, 2021
@remkop remkop added theme: auto-completion An issue or change related to auto-completion type: bug 🐛 labels Nov 17, 2021
@rsenden
Copy link
Contributor Author

rsenden commented Nov 17, 2021

I just added aliases to the existing AutoComplete tests, and updated the expected outputs. The only thing that I'm not sure about is why in AutoCompleteTest#testComplete() I need to repeat each alias three times. Any idea?

@remkop
Copy link
Owner

remkop commented Nov 18, 2021

The only thing that I'm not sure about is why in AutoCompleteTest#testComplete() I need to repeat each alias three times. Any idea?

I will have to take a look in the debugger to see what is going on. It may take some time before I can get to this.

@remkop
Copy link
Owner

remkop commented Nov 24, 2021

I finally had some time to take a look at this.
It appears that the AutoComplete#filterAndTrimMatchingPrefix method does not filter out duplicates, even though it should. This can be fixed by modifying the first line of this method to use a Set instead of a List:

    private static void filterAndTrimMatchingPrefix(String prefix, List<CharSequence> candidates) {
        Set<CharSequence> replace = new HashSet<CharSequence>();
        ...

The expectations in testComplete can then be changed to also have only one occurrence of each of the subcommand aliases, which makes much more sense.

Can you change the PR accordingly?

Copy link
Owner

@remkop remkop left a comment

Choose a reason for hiding this comment

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

I also found some minor things with variables used in the test result templates that should be preserved. Can you take a look?

src/test/resources/picocompletion-demo_completion.bash Outdated Show resolved Hide resolved
src/test/resources/picocompletion-demo_completion.bash Outdated Show resolved Hide resolved
@rsenden
Copy link
Contributor Author

rsenden commented Nov 24, 2021

PR has been updated with all requested changes

@remkop remkop merged commit ff96cc9 into remkop:master Nov 24, 2021
@remkop
Copy link
Owner

remkop commented Nov 24, 2021

Merged.
Thank you for the contribution!

I will update the release notes when I get to my PC.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
theme: auto-completion An issue or change related to auto-completion type: bug 🐛
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Autocompletion script doesn't properly handle aliases
2 participants