Skip to content

CLN: Fix typing in pandas\tests\arrays\test_datetimelike.py (#28926) #29014

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.

Sign up for GitHub

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 13 commits into from
Oct 23, 2019
Merged

CLN: Fix typing in pandas\tests\arrays\test_datetimelike.py (#28926) #29014

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
d4046de
Merge pull request #1 from pandas-dev/master
jjlkant Oct 14, 2019
871971b
Merge remote-tracking branch 'upstream/master'
jjlkant Oct 14, 2019
89fdfb3
Merge remote-tracking branch 'upstream/master'
jjlkant Oct 15, 2019
9953ad1
CLN: Fix typing in test_datetimelike.py (#28926)
jjlkant Oct 15, 2019
136b9a8
CLN: Fix typing in test_datetimelike.py (#28926)
jjlkant Oct 15, 2019
066da9d
Merge branch 'typing_test_datetimelike' of github.com:jjlkant/pandas …
jjlkant Oct 15, 2019
cc5614f
CLN: Sorted imports (GH28926)
jjlkant Oct 16, 2019
a4d0bc6
Merge remote-tracking branch 'upstream/master' into typing_test_datet…
jjlkant Oct 16, 2019
5184b16
ENH: Added DatetimeLikeIndex alias (GH29014)
jjlkant Oct 16, 2019
1e52b08
Revert to local Union defined type (GH29014)
jjlkant Oct 17, 2019
9376f81
Merge remote-tracking branch 'upstream/master' into typing_test_datet…
jjlkant Oct 17, 2019
f94b7e4
Merge remote-tracking branch 'upstream/master' into typing_test_datet…
jjlkant Oct 20, 2019
166fce9
Fixed merge conflict
jjlkant Oct 21, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion pandas/tests/arrays/test_datetimelike.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,12 @@
from typing import Type, Union

import numpy as np
import pytest

from pandas._libs import OutOfBoundsDatetime

import pandas as pd
from pandas import DatetimeIndex, PeriodIndex, TimedeltaIndex
from pandas.core.arrays import DatetimeArray, PeriodArray, TimedeltaArray
import pandas.util.testing as tm

Expand Down Expand Up @@ -52,7 +55,7 @@ def timedelta_index(request):


class SharedTests:
index_cls = None
index_cls = None # type: Type[Union[DatetimeIndex, TimedeltaIndex, PeriodIndex]]
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 put an alias for DatetimeLikeIndex in pandas._typing and use that here instead? I could see this being generally useful and we already have a DatetimeLikeScalar defined there, so would be consistent

@jbrockmendel in case he has other thoughts

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Very fair comment! Went ahead with an alias like:

DatetimeLikeIndex = Union["DatetimeIndex", "PeriodIndex", "TimedeltaIndex"]

Tried the route of using TypeVar as used for DatetimeLikeScalar, but this sent me down a road with unbound type variables as a result of index_cls being a class variable. In the case of DatetimeLikeScalar, the type hint was used on a @property instead of a class variable, and without hinting inside of a comment. Do we want to go that way as well with this case?

Copy link
Member

Choose a reason for hiding this comment

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

Yea let's go with a TypeVar. Did that come when doing Type[DatetimeLikeIndex]? If so not sure off hand of solution so curious to see what you can find, but in any case the TypeVar is the right way to go here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reading up on python/mypy#4590 as a result of looking into changing the class variables to @property, I think it is not possible to actually use a TypeVar? The @property looks as follows for example:

class SharedTests:
    @property
    def index_cls(self):
        # type: (SharedTests) -> Optional[Type[DatetimeLikeIndex]]
        return None

    ...

I'm currently not seeing a route that will lead to the TypeVar behaving as expected. Will dive deeper into the current usage of other TypeVar entries in pandas._typing like ArrayLike tomorrow, but any pointers are welcome in meantime!

Copy link
Member

Choose a reason for hiding this comment

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

What error would you get with TypeVar?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

error: Incompatible return value type for every entry that's not matching:

pandas\tests\arrays\test_datetimelike.py:236: error: Incompatible return value type (got "Type[DatetimeIndex]", expected "Type[PeriodIndex]")
pandas\tests\arrays\test_datetimelike.py:236: error: Incompatible return value type (got "Type[DatetimeIndex]", expected "Type[TimedeltaIndex]")
pandas\tests\arrays\test_datetimelike.py:489: error: Incompatible return value type (got "Type[TimedeltaIndex]", expected "Type[DatetimeIndex]")
pandas\tests\arrays\test_datetimelike.py:489: error: Incompatible return value type (got "Type[TimedeltaIndex]", expected "Type[PeriodIndex]")
pandas\tests\arrays\test_datetimelike.py:602: error: Incompatible return value type (got "Type[PeriodIndex]", expected "Type[DatetimeIndex]")
pandas\tests\arrays\test_datetimelike.py:602: error: Incompatible return value type (got "Type[PeriodIndex]", expected "Type[TimedeltaIndex]")

With for example at L230-L238:

class TestDatetimeArray(SharedTests):
    array_cls = DatetimeArray

    @property
    def index_cls(self):
        # type: (TestDatetimeArray) -> Type[DatetimeLikeIndex]
        return pd.DatetimeIndex

    ...

Copy link
Member

Choose a reason for hiding this comment

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

Hmm OK I suppose these tests aren't fully generic so maybe the TypeVar doesn't work. I guess best to just revert to what you had, i.e. the Union def directly in the test module. Will leave the generic DatetimeLikeArray to a separate exercise as required

Sorry for back and forth but appreciate your thoroughness

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, if the regular __init__(self) would be available, the TypeVar was no issue at all, however pytest doesn't take classes that actually use an __init__(self). Rolled back to the Union[] version for now


def test_compare_len1_raises(self):
# make sure we raise when comparing with different lengths, specific
Expand Down
3 changes: 0 additions & 3 deletions setup.cfg
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,9 +139,6 @@ ignore_errors=True
[mypy-pandas.tests.arrays.test_array]
ignore_errors=True

[mypy-pandas.tests.arrays.test_datetimelike]
ignore_errors=True

[mypy-pandas.tests.arrays.test_period]
ignore_errors=True

Expand Down