Static typing improvements in click.shell_completion#3460
Conversation
AndreasBackx
left a comment
AndreasBackx
left a comment
There was a problem hiding this comment.
Just the underscore, the reason for the Any, and I think this should be rebased on main, not stable? Otherwise looks like fine typing improvements. Someone else can confirm the stable vs main thing.
| complete_var: str, | ||
| instruction: str, | ||
| ) -> int: | ||
| ) -> t.Literal[0, 1]: |
There was a problem hiding this comment.
We should perhaps create a type for ExitCode, but at the same time, I'm not sure there's that much value. Kind of good to know the specific exit codes as well. 🤷️
| from typing_extensions import TypeVar | ||
|
|
||
| # `Any` is used as default for backwards compatibility (instead of e.g. `str`) | ||
| _ValueT_co = TypeVar("_ValueT_co", covariant=True, default=t.Any) |
There was a problem hiding this comment.
I don't think we need the underscore prefix, or at least I don't think we should as we don't do it elsewhere.
| return args, incomplete | ||
|
|
||
| def format_completion(self, item: CompletionItem) -> str: | ||
| def format_completion(self, item: CompletionItem[t.Any]) -> str: |
There was a problem hiding this comment.
Any reason it's Any here instead of str?
| Unreleased | ||
|
|
||
|
|
||
| Version 8.4.1 |
There was a problem hiding this comment.
Let's not include version changes I think?
|
If the types tighten at all, then I think it needs to be put on main, but I don't know types too well. If I were running runtime type checking, I would not expect types to change on a bug fix release. @davidism @kdeldycke can you both weigh in so we have a clear answer. |
|
@jorenham sorry for the back and forth on branches. We have not accepted many types prs from outside contributors, so still figuring it out. |
As I said earlier at #3463 (comment):
So to me it is OK to target |
|
I'm fine with typing changes in fix releases. |
|
Okay fix releases it is. |
6 participants
As promised in #3455, this improves several (static) types in
click.shell_completion:shell_completenarrowed return typeCompletionItemnow has an optional (defaulting toAnyfor backcompat) generic type parameter for its.value. I did not update the other modules to use this type parameter to avoid merge conflicts, but because of thedefault=Anythere's no downside to this.ShellComplete.source_varsnow returns a typed dictadd_completion_classis now parametrized on theclsinstance type instead of the type type, so that it's easier to see by looking at the annotations thatclsis supposed to be a type rather than an instance. If I remember correctly, now all type checkers support using it as class decorator otherwise, but don't quote me on this.get_completion_classnow has overloads for the builtin shell completion strings, so thatget_completion_class("zsh")will now be inferred astype[ZshComplete]instead oftype[ShellComplete] | None(the| Nonecan be pretty annoying to deal with)