Fix some small list -> Iterable/Sequence typing issues #1590
Conversation
list is not working as it is invairant, see https://mypy.readthedocs.io/en/stable/common_issues.html#variance
Given be the usage of words, it does not need to be List nor Sequence only an Iterable
| StyleAndTextTuples = List[OneStyleAndTextTuple] | ||
|
|
||
| # For typing stuff, we prefer an Iterable as it is more flexible | ||
| StyleAndTextTuplesType = Iterable[OneStyleAndTextTuple] |
There was a problem hiding this comment.
I'm not sure that Iterable is desirable, because it means it's not indexable and possibly, can only be consumed once.
Without reading again through all of the prompt_toolkit codebase, I think Sequence is at least what we need.
| def __init__( | ||
| self, | ||
| words: Union[List[str], Callable[[], List[str]]], | ||
| words: Union[Iterable[str], Callable[[], Iterable[str]]], |
There was a problem hiding this comment.
Please make this at least a Sequence.
Otherwise, users are going to pass in a generator of strings, and wonder why the second time, they're not getting any completions.
Is there a reason it should be Iterable here?
There was a problem hiding this comment.
Thanks for the review @jonathanslenders
Nope. I was just going to use the lowest possible. You are right a Sequence would be more fitting.
I stopped working on this PR as some other troubles occured (see failing tests)
Once I found time to resolve them, I will undraft the PR and ping you.
2 participants
"List" are invariant (see https://mypy.readthedocs.io/en/stable/common_issues.html#variance), so we really should try to avoid them, especially as input parameter for our functions.
Stubled upon the following places where it clashed with my type checking.
Maybe there are more but start small ;-)