Skip to content

Conversation

@twoertwein
Copy link
Member

@@ -1,4 +1,3 @@
# this becomes obsolete when reportGeneralTypeIssues can be enabled in pyproject.toml
Copy link
Member Author

Choose a reason for hiding this comment

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

Had to remove these comments as the JSON format does not allow comments (pyright seems to use a stricter parser now)

Copy link
Member

Choose a reason for hiding this comment

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

what happens if you dont upgrade pyright?

Copy link
Member Author

Choose a reason for hiding this comment

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

Upgrading pyright is not needed for this PR. Just upgraded it because there is a newer version.

Copy link
Member

Choose a reason for hiding this comment

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

OK. everything but this im happy to merge. for this can we ping someone who knows pyright better than me?

Copy link
Member Author

Choose a reason for hiding this comment

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

I reverted this change for this PR as it is not yet needed.

@mroeschke mroeschke added the Typing type annotations, mypy/pyright type checking label Mar 9, 2023
from pandas.core.indexers.objects import BaseIndexer
from pandas.core.resample import Resampler

_NDFrameT = TypeVar("_NDFrameT", bound="NDFrame")
Copy link
Member

Choose a reason for hiding this comment

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

can you add a comment on why this is necessary? e.g. i think once Self is available we can swap this out?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, this can be removed when we can use Self. If we want to link two parameters to be of the same NDFrame (sub-class) NDFrameT is enough, but if we have two pairs, we need to use a second TypeVar for the second pair. Otherwise, all four parameters are bound to the same (sub)class.

Copy link
Member

Choose a reason for hiding this comment

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

i know, im asking for a comment in the code to that effect

Copy link
Member Author

Choose a reason for hiding this comment

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

Added a comment. I realized that we cannot remove it when Self is available as Self doesn't help _align_as_utc.

Copy link
Member Author

Choose a reason for hiding this comment

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

Probably better to move it to pandas._typing since this will not be a temporary fix.

Copy link
Member

Choose a reason for hiding this comment

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

i think when the time comes we can make _align_as_utc a NDFrame method

Copy link
Member Author

Choose a reason for hiding this comment

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

moved to pandas._typing

@jbrockmendel
Copy link
Member

one comment request, otherwise LGTM

Copy link
Member

@jbrockmendel jbrockmendel left a comment

Choose a reason for hiding this comment

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

LGTM

@jbrockmendel jbrockmendel merged commit dee2bac into pandas-dev:main Mar 11, 2023
@jbrockmendel
Copy link
Member

thanks @twoertwein

@twoertwein twoertwein deleted the align branch August 9, 2023 15: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.

TYP: NDFrame.align is wrong

3 participants