Skip to content

Fix some issues in bash autocompletion#1674

Merged
dearchap merged 2 commits intourfave:mainfrom
MrNaif2018:fix-bash-autocomplete
Feb 1, 2023
Merged

Fix some issues in bash autocompletion#1674
dearchap merged 2 commits intourfave:mainfrom
MrNaif2018:fix-bash-autocomplete

Conversation

@MrNaif2018
Copy link
Contributor

@MrNaif2018 MrNaif2018 commented Jan 29, 2023

What type of PR is this?

  • bug

What this PR does / why we need it:

With current version of autocomplete script, shell quotes gets expanded incorrectly

Consider the following example:
my-cli --url http://localhost:8000 he (pressing tab should auto-complete to help, but it doesn't)
Why doesn't it auto-complete?
Because it calls the following command:
my-cli --url http : //localhost : 8000 --generate-bash-completion
And if we put it in quotes my-cli --url "http://localhost:8000" he
It calls:
my-cli --url '"http://localhost:8000"' --generate-bash-completion

Both fail, so CLI fails to call correct URL and return a list of completions.
With this PR it sends correct data
Actually I am not sure if that's the best solution, I checked how cobra does this and converted to our use case
It looks like the _init_completion function does all the magic and makes it work, I don't understand why unfortunately

P.S. I hope it'll be backported to 2.x series too (:

Testing

I tested multiple cases, i.e. sending one positional argument, 2 of them, or argument + flag, the CLI receives same arguments as before, but now more cases work

Release Notes

Fixed shell expansion in bash autocompletion scripts

@MrNaif2018 MrNaif2018 requested a review from a team as a code owner January 29, 2023 20:03
@dearchap dearchap changed the base branch from main to v2-maint January 29, 2023 21:47
@dearchap dearchap changed the base branch from v2-maint to main January 29, 2023 21:48
@dearchap
Copy link
Contributor

@MrNaif2018 Can you add a test case(s) for this ? I will attempt to backport to v2.x after that. Thanks

@MrNaif2018
Copy link
Contributor Author

Sure! Could you guide me where to check? Because bash behavior I tested was via tab, so not sure how to test it non-interactively

@dearchap
Copy link
Contributor

@MrNaif2018 You could use this test as a starting point. You can either add additional cases for this test or add a new test

cli/help_test.go

Line 1177 in f983141

func TestDefaultCompleteWithFlags(t *testing.T) {

@MrNaif2018
Copy link
Contributor Author

@dearchap I've added a simple test, but the issue is, it won't catch the bug anyway :D

No go-based code can catch the bug
The library is parsing and doing everything correctly, just that the bundled bash autocomplete script did something wrong, which caused arguments to be transformed wrong, hence by default bash parsing rules, go cli couldn't parse arguments correctly (because they were passed so)

@dearchap
Copy link
Contributor

dearchap commented Feb 1, 2023

@MrNaif2018 Just to confirm you want to merge this in v2 or v3 ?

@MrNaif2018
Copy link
Contributor Author

I am currently using v2 version, so I would like if it would land in v2 too. v3 probably needs this too if it uses same setup

@dearchap dearchap changed the base branch from main to v2-maint February 1, 2023 14:30
@dearchap dearchap changed the base branch from v2-maint to main February 1, 2023 14:30
@dearchap
Copy link
Contributor

dearchap commented Feb 1, 2023

@MrNaif2018 Can you re-submit a PR for v2 ? I will merge this into main for now

@dearchap dearchap merged commit c2f301a into urfave:main Feb 1, 2023
@MrNaif2018
Copy link
Contributor Author

Is it v2-maint?

@dearchap
Copy link
Contributor

dearchap commented Feb 1, 2023

Yes

@MrNaif2018
Copy link
Contributor Author

#1676

@MrNaif2018 MrNaif2018 deleted the fix-bash-autocomplete branch February 7, 2023 17:01
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.

2 participants