Skip to content

Conversation

@topper-123
Copy link
Contributor

@topper-123 topper-123 commented Feb 19, 2023

Progress towards #51233.

There were more updates to do than I expected, so I split it up a bit. In this PR I do pandas/core/generic.py, pandas/core/frame.py, pandas/core/series.py & pandas/indexes/.

@topper-123 topper-123 changed the title TYP: Use Self instead of class-bound TypeVar TYP: Use Self instead of class-bound TypeVar (generic.py/frame.py/series.py) Feb 20, 2023
@simonjayhawkins simonjayhawkins added the Typing type annotations, mypy/pyright type checking label Feb 21, 2023
@simonjayhawkins simonjayhawkins added this to the 2.1 milestone Feb 21, 2023
@simonjayhawkins simonjayhawkins marked this pull request as draft February 24, 2023 15:19
@topper-123 topper-123 marked this pull request as ready for review March 14, 2023 06:05

@final
def _left_indexer_unique(self: _IndexT, other: _IndexT) -> npt.NDArray[np.intp]:
def _left_indexer_unique(self, other: Index) -> npt.NDArray[np.intp]:
Copy link
Member

Choose a reason for hiding this comment

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

Is this change intended? Is _left_indexer_unique(self, other: Self) to strict?

Copy link
Member

Choose a reason for hiding this comment

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

(same for the following methods)

Copy link
Contributor Author

@topper-123 topper-123 Mar 14, 2023

Choose a reason for hiding this comment

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

Yeah, I was in doubt about this too. Fir the built-in index classes I think it will always require same class (so other: Self), but custom user-created classes:

>>> MyIndex(Index):
...     pass

In this case, MyIndex has same dtype as the base index, so should be comparable to Index in this method? (I'm leaning to yes; that this should be possible, but I'm not 100% sure).

Copy link
Member

Choose a reason for hiding this comment

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

For the built-in index classes I think it will always require same class (so other: Self)

I would then stick with Self it is also stricter which could help catch bugs (or causes a false mypy error which can then be fixed by someone who knows for sure).

@twoertwein twoertwein merged commit f26031e into pandas-dev:main Mar 15, 2023
@twoertwein
Copy link
Member

Thanks @topper-123!

@topper-123 topper-123 deleted the use_typing.Self branch March 15, 2023 19:08
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