Skip to content

Added Optional[] around key argument to itertools.groupby() #2790

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

Merged
merged 1 commit into from
Feb 12, 2019

Conversation

achampion
Copy link
Contributor

See #2788

The simplest case of:

import itertools
iterator = itertools.groupby(range(10), None)

gives the type error: error: No overload variant of "groupby" matches argument types "range", "None"

iterator = itertools.groupby(range(10))

has no error.
This example is a little contrived though legal, the case came up in the context of a Optional[] argument to function, e.g.:

T = TypeVar('T')
S = TypeVar('S')

def fn(iterable: Iterable, key: Optional[Callable[[T], S]] = None) -> Iterator[Tuple[S, Iterator[T]]]:
    iterator: Iterator[Tuple[S, Iterator[T]]] = itertools.groupby(iterable, key)
    return iterator

error: Argument 2 to "groupby" has incompatible type "Optional[Callable[[T], S]]"; expected "Callable[[T], S]"

…o allow `None` to pass the type

checker.
Copy link
Collaborator

@srittau srittau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@srittau srittau merged commit 9b545a0 into python:master Feb 12, 2019
@achampion achampion deleted the itertools-groupby-optional branch February 12, 2019 19:05
@@ -35,7 +35,7 @@ def ifilterfalse(predicate: Optional[Callable[[_T], Any]],
def groupby(iterable: Iterable[_T]) -> Iterator[Tuple[_T, Iterator[_T]]]: ...
@overload
def groupby(iterable: Iterable[_T],
key: Callable[[_T], _S]) -> Iterator[Tuple[_S, Iterator[_T]]]: ...
key: Optional[Callable[[_T], _S]]) -> Iterator[Tuple[_S, Iterator[_T]]]: ...
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we add a separate overload instead? If key is None, there's nothing to constrain the value of _S.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JelleZijlstra Good point, opened #2794.

srittau added a commit to srittau/typeshed that referenced this pull request Feb 13, 2019
JelleZijlstra pushed a commit that referenced this pull request Feb 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants