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

fix mypy errors in pandas/tests/arithmetic/test_datetime64.py #31716

Conversation

SaturnFromTitan
Copy link
Contributor

Part of #28926

  • tests added / passed
  • passes black pandas
  • passes git diff upstream/master -u -- "*.py" | flake8 --diff

@@ -191,6 +191,7 @@ class DatetimeIndex(DatetimeTimedeltaMixin):

_data: DatetimeArray
tz: Optional[tzinfo]
tz_localize: Callable[..., "DatetimeIndex"]
Copy link
Contributor Author

@SaturnFromTitan SaturnFromTitan Feb 5, 2020

Choose a reason for hiding this comment

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

I try to fake-annotate this tz_localize implementation.

I tried using something like Callable[["DatetimeIndex", Optional[str], str, str], "DatetimeIndex"], but then mypy would raise errors when omitting some arguments like index.tz_localize("US/Eastern")

Copy link
Contributor Author

@SaturnFromTitan SaturnFromTitan Feb 5, 2020

Choose a reason for hiding this comment

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

The original problem was this line of code would raise: "DatetimeIndex" has no attribute "tz_localize"

Copy link
Member

Choose a reason for hiding this comment

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

this is going to be a common problem with inherit_names. best case would be to find a way to fix it at the source

Copy link
Member

Choose a reason for hiding this comment

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

What is the original mypy error? I'd be hesitant to make any changes to the actual code base to silence errors coming from the test suite

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@WillAyd I stated it in my second comment. I think @jbrockmendel is right, but it's not clear how a solution might look like. I'm investigating though ✌️

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems like this would be the last issue where an annotation like this is required to finish #28926 off. Therefore it's maybe worth still merging this one and working on a more general solution afterwards? Wdyt @WillAyd @simonjayhawkins?

Copy link
Member

Choose a reason for hiding this comment

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

I would prefer to either ignore where required in a few places or just leave things as they are today

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright, I'll close this PR then and create a follow-up issue that focuses on refactoring inherit_names

Copy link
Contributor Author

Choose a reason for hiding this comment

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

xref: #32100

Copy link
Member

Choose a reason for hiding this comment

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

Great thanks for understanding - your contributions are very much appreciated

@SaturnFromTitan SaturnFromTitan marked this pull request as ready for review February 5, 2020 19:22
@SaturnFromTitan SaturnFromTitan changed the title fix type errors in pandas/tests/arithmetic/test_datetime64.py fix mypy errors in pandas/tests/arithmetic/test_datetime64.py Feb 5, 2020
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.

lgtm. pending green.

@SaturnFromTitan SaturnFromTitan force-pushed the 28926-fix-mypy-errors-for-pandas-tests-arithmetic-test_datetime64 branch 2 times, most recently from 5cf718c to 583e460 Compare February 8, 2020 14:50
@jreback jreback added the Typing type annotations, mypy/pyright type checking label Feb 9, 2020
@simonjayhawkins
Copy link
Member

@SaturnFromTitan can you merge master.

@SaturnFromTitan SaturnFromTitan force-pushed the 28926-fix-mypy-errors-for-pandas-tests-arithmetic-test_datetime64 branch from 583e460 to a0dfbc4 Compare February 9, 2020 21:02
@SaturnFromTitan SaturnFromTitan force-pushed the 28926-fix-mypy-errors-for-pandas-tests-arithmetic-test_datetime64 branch from a0dfbc4 to 515df25 Compare February 11, 2020 21:17
@@ -191,6 +191,7 @@ class DatetimeIndex(DatetimeTimedeltaMixin):

_data: DatetimeArray
tz: Optional[tzinfo]
tz_localize: Callable[..., "DatetimeIndex"]
Copy link
Member

Choose a reason for hiding this comment

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

Do you see any alternatives? Again my concern is that going in this direction will yield a lot of code duplication

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.

5 participants