-
-
Notifications
You must be signed in to change notification settings - Fork 702
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
🐛 Ensure that autocompletion works for Path
arguments/options
#1138
base: master
Are you sure you want to change the base?
Conversation
The coverage test is failing, because there are no autocompletion tests with |
You might have a look at how I do this in django-typer. I run the shells in a pseudo terminal subprocess. This allows me to test end-to-end like you want here. There are a few downsides though: 1). I haven't been able to get this to work with fish yet (when I run it in a pty it decides its not an interactive shell and terminates prematurely - I'm sure theres a way around it I just havent found it yet). It does work with powershell, bash and zsh. |
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.
Amazing! Beautiful job! 👏
The investigation and report make me understand what's happening.
And I like this is the minimal change to fix the problem. 😎
I consider finding short and simple (but correct) solutions tend to be much harder than finding more complicated ones, so this is great. 🧠
I think this could go in, we make a release with the fix, and then later we could refactor Typer's completion logic having all the rest in mind, maybe having the equivalent of the new logic in Click but written in Python... but this would fix the bug right away so we can start with this which solves the most important/urgent part.
About the name TyperPath
I didn't find a better alternative, there's already other Typer<X>
classes like TyperArgument
and TyperGroup
, so it seems fine.
The other words I was playing with were Param
or Type
(as Click's Path
inherits from ParamType
), but I don't think it's any clearer, so I think TyperPath
is fine. 🤓
Resolves #951
Issue
The issue on
master
is that parameters (either anArgument
or anOption
) specifically typed asPath
wouldn't be automatically completed on the commandline. For instance, having two filestest_1.py
andtest_2.py
in a directory, and typing"t"
for an option typed asPath
, would simply put a space after the"t"
, not autocompleting at all.The desired behaviour instead would be to first autocomplete the
"test_"
part, and then after another TAB show the two options"test_1.py"
and"test_2.py"
.Proposed solution
After quite a bit of digging and reading @tiangolo's elaborate background information on issue #951, I've come to the conclusion that this unexpected behaviour is likely caused by an unwanted and inconsistent interplay between Typer and Click.
Stepping back for a second - if you would type the
Argument
orOption
asstr
, ironically then the autocompletion would actually work out of the box. Things go haywire (IMO) in theget_click_type
function where the parameter is converted to the correspondingclick.Path
when a type annotation ofpathlib.Path
is found. Thisclick.Path
class has a specificshell_complete
function that:But Typer's autocompletion functionality doesn't use that marker like Click's scripts (e.g. the Bash one) does. If instead we return the empty list (i.e. the default behaviour of
click.ParamType
), then it all seems to work again.Testing
I could confirm the original bug as described in #951 on Bash and Zsh, and I could confirm that the autocompletion functionality works again with
Path
parameters with this code change.If this is the route forward we want to take, I'll test on more systems.
However, I'm entirely unsure how to actually test this in the test suite, given the dependency on the actual console for completion.
Open questions
Not sure whether this is the kind of fix/resolution that @tiangolo has in mind, so I'm writing this up to solicit feedback. Also the naming of "TyperPath" is probably not ideal - open to suggestions 😁