Skip to content
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

TYP: NDFrame.pipe, GroupBy.pipe etc. #39093

Merged
merged 6 commits into from
Jan 15, 2021

Conversation

topper-123
Copy link
Contributor

@topper-123 topper-123 commented Jan 10, 2021

Type up pipe function and methods. This allows type checkers to understand the return value type from pipes, which is nice.

@jreback jreback added the Typing type annotations, mypy/pyright type checking label Jan 11, 2021
@jreback jreback added this to the 1.3 milestone Jan 11, 2021
@jreback
Copy link
Contributor

jreback commented Jan 11, 2021

cc @simonjayhawkins

Copy link
Member

@simonjayhawkins simonjayhawkins left a comment

Choose a reason for hiding this comment

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

Thanks @topper-123 generally lgtm

@@ -123,6 +123,7 @@

if TYPE_CHECKING:
from pandas._libs.tslibs import BaseOffset
from pandas._typing import T
Copy link
Member

Choose a reason for hiding this comment

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

imports from _typing are already guarded.

@@ -78,6 +80,10 @@ class providing the base-class of operations.
from pandas.core.sorting import get_group_index_sorter
from pandas.core.util.numba_ import NUMBA_FUNC_CACHE

if TYPE_CHECKING:
from pandas._typing import T
Copy link
Member

Choose a reason for hiding this comment

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

same

@@ -43,6 +45,9 @@
from pandas.tseries.frequencies import is_subperiod, is_superperiod
from pandas.tseries.offsets import DateOffset, Day, Nano, Tick

if TYPE_CHECKING:
from pandas._typing import T
Copy link
Member

Choose a reason for hiding this comment

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

same

@topper-123
Copy link
Contributor Author

Updated.

@topper-123
Copy link
Contributor Author

I think this is ok to merge?

@simonjayhawkins
Copy link
Member

I think so. We may want to have an alias for this, but as the type is not yet complete I think we should leave that to later.

I say incomplete since we will replace the ... of Callable[..., T] at some point, and that will require a second typevar to bind to self/obj.

but in the spirit of gradual typing, this gets the return type sorted.

@topper-123
Copy link
Contributor Author

topper-123 commented Jan 14, 2021

.pipe takes very generic callables, so I don’t think it’s possible to type more. Can you give an example what you’re thinking?

In python 3.10 there’s something coming that may improve typing for .pipe, but that’s far in the future for Pandas...

@simonjayhawkins
Copy link
Member

if func is a tuple, it is indeed not possible to bind to a keyword argument, but if func is not a tuple and args is not specified, then the function should take just 1 positional argument which is the object?

so one of the overloads would be

def pipe(
    obj: S, func: Union[Callable[[S], T], Tuple[Callable[..., T], str]], **kwargs
) -> T:

where S is a TypeVar and args not specified (args may need to be None, I can't remember off the top of my head)

Indeed, this is out of scope for now and I have approved the changes as is.

@topper-123
Copy link
Contributor Author

Ok. I think it’s ok to wait with that...

@simonjayhawkins
Copy link
Member

yeah, since we maybe pulling in some type stubs, #28142 (comment), I suspect that we can't yet inline those stubs where the types are not compatible with Python 3.7 (without typing_extensions) and I suspect the overloads will be in stub file instead of inline ( and may be able to adopt PEP 612 sooner.)

@simonjayhawkins simonjayhawkins merged commit a7085c0 into pandas-dev:master Jan 15, 2021
@simonjayhawkins
Copy link
Member

Thanks @topper-123

@topper-123 topper-123 deleted the type_pipe branch January 15, 2021 14:05
luckyvs1 pushed a commit to luckyvs1/pandas that referenced this pull request Jan 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Typing type annotations, mypy/pyright type checking
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants