Skip to content

TYP: NDFrame.(loc|iloc|at|iat) #30690

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 2 commits into from
Jan 4, 2020

Conversation

topper-123
Copy link
Contributor

Currently, the NDFrame indexers (.loc/.iloc/.at/.iat) are too complex set up to let mypy understand them. This PR makes their implementations understandable for mypy (and humans also maybe, the old imp. was a bit indirect so took som effort to understand).

One issue is that I - for the life of me - can't programatically set doc strings of properties and make mypy understand the properties' types at the same time. Python/mypy seem to demand that doc strings be set directly on the property if the type is to be understandable by mypy. E.g. Appender on a property is not supported by mypy. So I've set the doc string on the NDFrame properties, and reference them from the indexer classes instances. This makes the PR seem large, but it's mostly just moving the doc strings.

@@ -3179,6 +3179,481 @@ def to_csv(
# ----------------------------------------------------------------------
# Fancy Indexing

@property
Copy link
Contributor

Choose a reason for hiding this comment

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

so to avoid moving these multiple times, can you create a mix-in class where these are put (you can define this in pandas.core.indexing i think would be ok), then just include it in the classes for Series/DataFrame

Copy link
Contributor

Choose a reason for hiding this comment

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

maybe better (as we are going to do this anyhow) is create

pandas/core/generic/indexing.py (and put the mxin there)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea with a Mixin. Why do you want to set it on DataFrame/Series directly? Seems simpler to just set it on NDFrame.

Copy link
Contributor

Choose a reason for hiding this comment

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

right that’s totally fine

@jreback jreback added Typing type annotations, mypy/pyright type checking Indexing Related to indexing on series/frames, not to indexes themselves labels Jan 4, 2020
@topper-123 topper-123 force-pushed the type_loc_iloc_at_iat branch from 15f337b to a6f7730 Compare January 4, 2020 20:35
@topper-123 topper-123 force-pushed the type_loc_iloc_at_iat branch from a6f7730 to eda2ca6 Compare January 4, 2020 21:09
@topper-123
Copy link
Contributor Author

The failure is unrelated and will be fixed in #30698. I can reun the tests when #30698 is merged.

@jreback jreback added this to the 1.0 milestone Jan 4, 2020
@jreback jreback merged commit cf400f9 into pandas-dev:master Jan 4, 2020
@jreback
Copy link
Contributor

jreback commented Jan 4, 2020

thanks @topper-123 very nice

@topper-123 topper-123 deleted the type_loc_iloc_at_iat branch January 4, 2020 23:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Indexing Related to indexing on series/frames, not to indexes themselves Typing type annotations, mypy/pyright type checking
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants