✨ Upgrade autocompletion functionality for compatibility with shell_complete
#1006
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 |
svlandeg
changed the title
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 #974) it appears that all the changes from #974 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 #974 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
|
bckohan
force-pushed
the
feat/autocompletion
branch
from
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 |
|
📝 Docs preview for commit 4c356f5 at: https://27478c2f.typertiangolo.pages.dev Modified Pages |
There was a problem hiding this comment.
Hi @bckohan,
Apologies for the delay in reviewing this and the slow iterations on our end. While we've split off some of the functionality to separate PRs already, I wonder whether perhaps we should go even further with this. In particular:
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.
With Number 3 referring to the change of behaviour of args, as reflected by the change in the unit test in test_others.py. While I can follow your reasoning, I can't attest to how breaking this would be for other users. It looks like on the other hand, your first two contributions are clear feature additions that wouldn't change any current behaviour (right)?
Knowing the work load that the team, including Tiangolo, already has on the maintenance of Typer and FastAPI (and friends), I would advice to further split this PR into separate parts, and moving the breaking change 3 with respect to args to a separate PR that might involve more discussion. When doing so, I would further advice to keep all existing unit tests the same as much as possible, to remove any doubt that there would be breaking changes in existing behaviour.
I realise that you've already put quite a bit of time and effort into this work, and totally understand if you don't have the bandwidth to follow up on this right now. If so - just let me know, and I will have a stab at breaking up this PR further myself next week.
| def complete_name(ctx: typer.Context, args: List[str], incomplete: str): | ||
| err_console.print(f"{args}") | ||
| names = ctx.params.get("name") or [] | ||
| def complete_name(ctx: typer.Context, param: Parameter, incomplete: str): | ||
| names = ctx.params.get(param.name) or [] |
There was a problem hiding this comment.
What was the rational to changing these existing 009 tests? I double checked and they succeed in their original form on master so I would opt to keep them as-is, to make it clear that there are no breaking changes introduced from this PR (which otherwise a changing test would alude to). If it's a matter of coverage, then I would try to incorporate that into the new 010 tests.
There was a problem hiding this comment.
I believe my intent was to change this tutorial to using the context to get the parsed CLI parameters as the preferred method, but looking closer at the docs and the tutorials I added later I think this can be reverted and I can tweak some of the tutorials further down the page. I'll review that and make corrections.
There was a problem hiding this comment.
Reading closer I see what I was doing. I'm changing the "Getting the Context and the raw CLI parameters" tutorial to "Getting the Context and the Parsed CLI parameters". The reasoning for this is just to limit the size of the tutorials to keep it from becoming unwieldy. Instead of a second tutorial illustrating usage of args - which should probably be avoided in place of using the context we instead illustrate using the context and the parameter object to get prior parsed options.
If you agree with this I think all I should do here is make the section title change ("raw"->"parsed")
There was a problem hiding this comment.
Ok ok, ignore all previous comments. I think my change here was OBE by my addition of the Reusing Generic Completer Functions section. I can revert this tutorial to its prior state. It will, however, as with the first "raw CLI" parameter section require the args bug fix to be accurate.
There was a problem hiding this comment.
I'm a little confused as to what the conclusion is here (and what "OBE" means). I thought you'd said you'd revert the changes to this part of the tutorial, but they're still in the current PR diff. Also note that due to the added import in tutorial009.py and the code reformatting, the highlighted lines as referred to in the tutorial text won't be accurate anymore (but all of that is irrelevant if we do revert).
There was a problem hiding this comment.
Sorry I forgot to leave a note about this. When I pulled #1134 out I did a run through of the tutorials to make sure everything still flowed well and made sense. There is one tutorial change and one additional tutorial that are part of this PR:
- In main, tutorial009 is the last tutorial and the point of it is to illustrate that you can receive all the arguments if you would like. Since we're adding param as another argument I simply extended this tutorial to show that you can get all 4.
- tutorial010 is new and illustrates using the param object to determine which parameter the generic completer function is being asked to complete.
- OBE == Overcome By Events
|
@svlandeg I'm happy to split out the args bug fix into a separate PR. I'll do that ASAP. To be clear its not really a breaking change, its a bug fix to bring behavior in line with what the current docs say. Args is documented to include all of the CLI options before the incomplete string. Currently args is hardcoded to be an empty list. Before that it was pulled from Context.args which only contains unrecognized/leftover options. I suspect because this behavior is very confusing it was just set to empty list to make the tests pass - which were passing erroneously, so this just slipped through the last update. |
…e included in a separate PR. Also update tutorial 9 to show all supported parameters may be passed
|
📝 Docs preview for commit c4ef5ab at: https://ff62fa70.typertiangolo.pages.dev Modified Pages |
|
📝 Docs preview for commit 3c4bff4 at: https://a055a513.typertiangolo.pages.dev Modified Pages |
|
📝 Docs preview for commit a60d98c at: https://dcbfbb15.typertiangolo.pages.dev Modified Pages |
|
I've separated out the fix to args here: #1134 |
|
📝 Docs preview for commit bd49bcf at: https://88bcb008.typertiangolo.pages.dev Modified Pages |
|
📝 Docs preview for commit 793921e at: https://cfcf57a4.typertiangolo.pages.dev Modified Pages |
|
📝 Docs preview for commit 723cf94 at: https://da01e1a9.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.Parameterargument. 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: edit this change was separated out in 🐛 Fix
argparameter of autocompletion #1134A reusable completer function tutorial has been added to the autocompletion tutorials