-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
5-ary izip #1042
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
5-ary izip #1042
Conversation
stdlib/2/itertools.pyi
Outdated
@overload | ||
def izip(iter1: Iterable[_T1], iter2: Iterable[_T2], iter3: Iterable[_T3], | ||
iter4: Iterable[_T4], iter5: Iterable[_T5]) | ||
-> Iterator[Tuple[_T1, _T2, _T3, _T4, _T5]]: ... # TODO more than five iterables |
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.
This is a syntax error - the "->" needs to be on the same line.
Also, would
@overload
def izip(*iter: Iterable) -> Iterator[tuple]: ...
do the trick?
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.
I believe that while that would work for n-ary calls, it would drop type information for the individual tuple positions, so spelling it out may be worth the tradeoff here and work in more cases. The use cases I've seen for izip are often of the form
for foo, bar in itertools.izip(foos, bars):
frobnicate(foo, bar)
and it would make sense to keep the type information for foo and bar around.
What lint error? |
We use pytype as a linter, on our side. @gnoack is talking about the error you get when you do something like
|
Ooh, I got it working. :) Up to what number of arguments should we go? (I'm unfamiliar with the type specification language, I'm assuming that it doesn't technically work yet to both support n-ary arguments and still retain the types within the resulting tuples?) |
Thanks for the clarification. LGTM. I'll leave it to @matthiaskramm to merge. |
stdlib/2/itertools.pyi
Outdated
def izip(iter1: Iterable[_T1], iter2: Iterable[_T2], iter3: Iterable[_T3], | ||
iter4: Iterable[_T4], iter5: Iterable[_T5]) -> Iterator[Tuple[_T1, _T2, | ||
_T3, _T4, _T5]]: ... | ||
# TODO more than five iterables |
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.
Could you add the following:
def izip(*iter: Iterable) -> Iterator[tuple]
?
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.
Oh, I see you already commented on that. Feel free to do the explicit version for as many arguments as you see fit, but the list of signatures should finish with the general case, even if it's less precise.
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.
Sounds good, adding 6-ary variant and n-ary fallback.
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.
Removed the fallback again, that didn't work in Python3.5 (see the more recent Travis failures).
Using izip with up to 6 arguments will retain the arguments' type information, using izip with 7 and more arguments will discard type information about the generated tuple items.
I had trouble adding the fallback case as you asked; It worked for the other two Python versions, but for Python3.5, Travis says:
I tried to make the return type |
The Python versions given in Travis are misleading; in fact "3.6" is flake8 (our linter), "3.5" is mypy, and "2.7" is pytype. I think it's a mypy bug that it rejects your last overload (the types are actually compatible), so feel free to file a bug against mypy. |
Merged, thanks! I hope nobody needs to izip seven iterables before we implement variadic type variables. :) |
I implemented the following fallback for builtin
See #1041 |
Obviously this is not the real solution, but it gets rid of the linter warning for now. :)