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

Disable completion of cache subcommand #2399

Merged
merged 3 commits into from
Dec 18, 2022
Merged

Disable completion of cache subcommand #2399

merged 3 commits into from
Dec 18, 2022

Conversation

cyqsimon
Copy link
Contributor

See #2085 (comment).

TLDR: autocompleting the cache subcommand causes usability issues. This PR disables it.

I am not terribly familiar with completion scripts TBH. If you are more competent please help check my edits, thanks.

Copy link
Collaborator

@Enselic Enselic left a comment

Choose a reason for hiding this comment

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

I'm no shell completion expert, but these changes look reasonable to me. And I am strongly in favor of the what the PR wants to achieve.

I'll set this as Approved. I'll give some more time for people to look at this. If no more input comes in, I'll merge this.

I think it is better to merge it so that problems can be found, if any, rather than letting the PR sit untouched for much longer.

Ideally we would have regression tests for completion scripts, but life is not always ideal.

@Enselic
Copy link
Collaborator

Enselic commented Dec 11, 2022

@cyqsimon Can you also add an entry to CHANGELOG.md please?

@sharkdp
Copy link
Owner

sharkdp commented Dec 11, 2022

I agree. Thank you @cyqsimon. I would appreciate if we could remove the code in question, instead of commenting it. We would still keep the comments explaining why we're not completing cache, of course.

@cyqsimon
Copy link
Contributor Author

@Enselic @sharkdp all done.

@Enselic Enselic merged commit b6b9d3a into sharkdp:master Dec 18, 2022
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