-
-
Notifications
You must be signed in to change notification settings - Fork 682
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
✨ Upgrade autocompletion
functionality for compatibility with shell_complete
#1006
base: master
Are you sure you want to change the base?
Conversation
📝 Docs preview for commit 249b70e at: https://6ae37f21.typertiangolo.pages.dev Modified Pages |
📝 Docs preview for commit c5aab0a at: https://9a5f770f.typertiangolo.pages.dev Modified Pages |
autocompletion
functionality for compatibility with shell_complete
Thanks for the PR! We'll get this reviewed and get back to you 🙏 |
📝 Docs preview for commit 340c459 at: https://985aa99d.typertiangolo.pages.dev Modified Pages |
@bckohan: the tests are failing because there isn't a 100% code coverage with the new commits. Could you look into this? 🙏 |
📝 Docs preview for commit 26581c0 at: https://9afeb588.typertiangolo.pages.dev Modified Pages |
📝 Docs preview for commit 1037702 at: https://c58d58a9.typertiangolo.pages.dev Modified Pages |
📝 Docs preview for commit 8b300ba at: https://9a9692fe.typertiangolo.pages.dev Modified Pages |
📝 Docs preview for commit 00e6af5 at: https://b52048c2.typertiangolo.pages.dev Modified Pages |
@svlandeg I've updated the docs and added the necessary tests. Should be good to go! |
Great, thanks! I've got this on my TODO list to review in detail, but realistically it might take me 1 or 2 more weeks to get to it 🙏 |
📝 Docs preview for commit 8668bb7 at: https://2142af69.typertiangolo.pages.dev Modified Pages |
📝 Docs preview for commit da62b3b at: https://09f6ce46.typertiangolo.pages.dev Modified Pages |
Ok so looking at the diffs of these two PRs (this one and #949) it appears that all the changes from #949 are still included here verbatim. As such I think it's actually easier to review these changes separately, i.e. one PR at a time. I've also pulled out the typo changes to avoid cluttering the diff: #1077 (I should have done that from the get-go). @bckohan: do you see any reason not to merge #949 first, then continue working on the functionality from this PR? |
📝 Docs preview for commit 365e374 at: https://e719418f.typertiangolo.pages.dev Modified Pages |
@bckohan in which way would it break downstream software? |
@tiangolo Its not exactly broken - but deprecation warnings are being thrown with no recourse to fix them because autocompletion does not pass the Parameter objects. There are also downstream completers that expect to be able to pass a CompletionItem back because they're setting the type field. |
I would think now there would be fewer warnings as the And the other
I'm not sure I got this, could you share an example? It might also be useful to start in a GitHub Discussion with a minimal example we can copy paste that shows the problem (or if there are multiple problems, multiple examples). |
📝 Docs preview for commit 876bcab at: https://989516f8.typertiangolo.pages.dev Modified Pages |
HI! I think its reasonable to expect significant usage of shell_complete for the following reasons:
I'm extensively using shell_complete with Parameter objects downstream. And there's evidence here of others trying. I also added example/explanation of why this is needed to the tutorial in the docs. And here's real world production code where I'm using
|
…s through CompletionItem return values, fix args to match what it was in click 7
876bcab
to
46c406b
Compare
📝 Docs preview for commit 46c406b at: https://402b6ae8.typertiangolo.pages.dev Modified Pages |
I rebased - hopefully it will be easier to review! |
📝 Docs preview for commit 031a59e at: https://21b1706e.typertiangolo.pages.dev Modified Pages |
📝 Docs preview for commit 05cb6f0 at: https://fb6de8b0.typertiangolo.pages.dev Modified Pages |
Cf issue: #949
Note, this includes (and supersedes) the changes from #974 and implements these suggestions #949 (comment), with a few differences.
I attempted to push these changes to #974 but was asked to open a new PR here instead.
This PR does 3 things:
Allows autocompletion functions to accept a
click.core.Parameter
argument. I think this is necessary because without it you can't define generic completer functions that don't know what CLI options or arguments they may be attached to and that need to alter their behavior based on how click has processed the arguments before the incomplete argument. Concrete example: you have an option or argument that accepts multiple values but you do not wish those values to be repeated.Allows CompletionItems to be returned in addition to strings and 2-tuples from autocompletion functions.
Fixes arg: List[str] parameter to be passed what it used to be given in click 7 (i.e. the raw command line string minus the script and incomplete string). In click 7 this parameter was being set to the arguments passed in the environment variables via the completion scripts installed on the system that are invoked when you hit tab. These parameters do not show up in ctx.args or sys.argv. Click has a general purpose obj attribute on the context that is available for downstream use. This PR sets that object to a dictionary in the completer classes and attaches the args from the environment variables to it. If obj is already defined it'll just not do that and nothing will break accept the args will be passed as an empty list to the autocompletion functions - which is what the original PR was doing anyway even though the tutorial docs said otherwise.
I think numbers 1 and 2 are important. Number 3 is mostly for backwards compatibility but I don't think its that critical - I just wanted to show that there is a way to do it using public Click 8 interfaces.
A reusable completer function tutorial has been added to the autocompletion tutorials