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

revert Timestamp and Timedelta constructors typing allowing NaTType return #48112

Merged
merged 4 commits into from
Aug 17, 2022

Conversation

Dr-Irv
Copy link
Contributor

@Dr-Irv Dr-Irv commented Aug 16, 2022

Re discussion at #46171 (comment)

Has Timestamp and Timedelta constructors return union that includes NaTType

@Dr-Irv Dr-Irv requested a review from mroeschke August 16, 2022 17:42
@mroeschke mroeschke added the Typing type annotations, mypy/pyright type checking label Aug 16, 2022
@Dr-Irv
Copy link
Contributor Author

Dr-Irv commented Aug 16, 2022

@mroeschke Not sure what to do here. pyright is reporting issues in other places in the code. See https://github.com/pandas-dev/pandas/runs/7863880652?check_suite_focus=true

I think that the level of pyright checking increased since the original PR was done. Or other changes made to the code were fine without having the constructors return NaTType so when we declare it that way, we need to fix some other places.

On the one hand, pyright is picking up some potential errors. On the other hand, making this simple change means there is more work to do to fix up other parts of the code that were not touched in the original PR.

I will see if I can workaround those issues.

@twoertwein
Copy link
Member

I think the issue is that for mypy __new__ cannot return an object other than from the corresponding class. python/mypy#7188 suggests that mypy supports other type annotations for __new__ IF they are subclasses (so basically it still seems to not support them for Timestamp).

If they cannot be fixed now, it is probably best to add those files to https://github.com/pandas-dev/pandas/blob/main/pyright_reportGeneralTypeIssues.json one day when this ignore list is small enough we can add in-line ignore comments.

@Dr-Irv
Copy link
Contributor Author

Dr-Irv commented Aug 16, 2022

I think the issue is that for mypy __new__ cannot return an object other than from the corresponding class. python/mypy#7188 suggests that mypy supports other type annotations for __new__ IF they are subclasses (so basically it still seems to not support them for Timestamp).

Yes, that is it.

If they cannot be fixed now, it is probably best to add those files to https://github.com/pandas-dev/pandas/blob/main/pyright_reportGeneralTypeIssues.json one day when this ignore list is small enough we can add in-line ignore comments.

Thanks for this suggestion. Will do this in next commit.

Copy link
Member

@twoertwein twoertwein left a comment

Choose a reason for hiding this comment

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

Great that you were able to reduce the number of new files ignored by pyright's reportGeneralTypeIssues to only two!

Looks good to me, assuming the typing tests pass.

@twoertwein
Copy link
Member

Probably the first catch of stubtest :) tz doesn't seem to exist at runtime: https://github.com/pandas-dev/pandas/runs/7865645731?check_suite_focus=true

@Dr-Irv
Copy link
Contributor Author

Dr-Irv commented Aug 16, 2022

Probably the first catch of stubtest :) tz doesn't seem to exist at runtime: https://github.com/pandas-dev/pandas/runs/7865645731?check_suite_focus=true

Thanks for finding that. I'll create tz and tzinfo methods for NaTType that return None

@Dr-Irv
Copy link
Contributor Author

Dr-Irv commented Aug 17, 2022

@mroeschke so this is good to go. I don't know what to do about the failure with the CircleCI Pipeline

@mroeschke mroeschke added this to the 1.5 milestone Aug 17, 2022
Copy link
Member

@mroeschke mroeschke left a comment

Choose a reason for hiding this comment

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

Thanks for the quick resolution @Dr-Irv! Hoping the CircleCI issue is spurious, but the important part is the typing builds passed.

@mroeschke mroeschke merged commit 19a7ba2 into pandas-dev:main Aug 17, 2022
@mroeschke mroeschke added Missing-data np.nan, pd.NaT, pd.NA, dropna, isnull, interpolate Timedelta Timedelta data type Timestamp pd.Timestamp and associated methods labels Aug 17, 2022
@mroeschke mroeschke mentioned this pull request Aug 17, 2022
noatamir pushed a commit to noatamir/pandas that referenced this pull request Nov 9, 2022
…eturn (pandas-dev#48112)

* revert Timestamp and Timedelta constructors typing allowing NaTType return

* exclude 2 files from pyright.  Fix up tz methods for NaTType

* add tz and tzinfo methods

* remove check that tz doesn't exist for NaT
@Dr-Irv Dr-Irv deleted the revert46171 branch February 13, 2023 20:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Missing-data np.nan, pd.NaT, pd.NA, dropna, isnull, interpolate Timedelta Timedelta data type Timestamp pd.Timestamp and associated methods Typing type annotations, mypy/pyright type checking
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants