-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
File and path completion again #1403
Conversation
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like that this adds an API to the types.
To ensure cleanup
I added a PR against your PR to add variable and tilde expansion, @dimbleby. |
So I see! Looks good, I'll hit the green button in a moment... |
Auto-completion of paths with shell vars or home shortcuts
This comment has been minimized.
This comment has been minimized.
I've been trying to review this and get it ready for 7.1, and I'm starting to realize just how messy completion is. Documentation for completion systems is hard to find and hard to follow. Bash doesn't quote completions, so filenames with spaces (or any other values with special characters) are invalid. ZSH does quote, but that means it also quotes the trailing space that this PR is manually adding now. But since ZSH uses a menu for completions, not adding a space makes it impossible to continue completing paths, as pressing tab or arrows scrolls through the menu instead of listing the next directory. Just like this PR turns on The problem is the limited way that Click implements completion right now. There's no reason we just have to return completions. In fact, we already support returning descriptions, presumably we could extend that more. Wouldn't it be cool if Click could indicate to the completion script that it should use other functions that Bash or ZSH provide, not just I'm trying to figure out a way to add some of the enhancements here like the |
How about an env var that determines default behaviour? And/or the bash completion code generated (the code meant to be sourced to get completion hooked into bash) could be slightly different based on some arg given. Eg
is current behavior vs
would generate hook code that turns off autospace. |
That wouldn't help the issue here. Completion is already pretty opaque as it is, adding configuration through more env vars is adding complexity. Users shouldn't have to care about that. |
@davidism Then how about a setting that can be toggled in the CLI main script code eg in my_click_app.py I could add a line click.set_auto_spaces() to get the old behavior, or just keep current behavior by default but have click.set_no_auto_spaces() for those starting fresh. Would also be nice to have an additional option on individual commands to override the global so that commands can be tweaked one at a time, as you can in a bash script, but maybe too difficult in click. At least a global for all-or-none would be better than current situation. |
No, I don't think adding settings anywhere is the right idea. Also, turning on or off spaces doesn't make Zsh completion any better, it's broken either way. The whole system needs to be redesigned, not made configurable. |
This is almost entirely @gtristan's work from #782, but updated for changes that have been made to
click
in the meantime. fixes #780In my experience, failing autocompletion on paths and filenames is by far the most annoying part about using otherwise lovely
click
CLIs. Which is why I have been motivated to put this together!As at #782, the approach is:
-o nospace
to the bash completion options, so that spaces are not automatically added to completions. This is necessary so that nested paths can be completed without constantly having to delete the added space.click
I expect that the possibly-controversial bit is that, as of today, any user-implemented autocompletions won't include spaces; and so the addition of
-o nospace
will cause their CLIs to behave slightly differently.I suggest that we either:
I'm happy to implement the second of these if that's what maintainers want - especially if it helps this longstanding fix finally to get over the line!