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

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

Merged
merged 13 commits into from
Oct 23, 2019

Conversation

jjlkant
Copy link
Contributor

@jjlkant jjlkant commented Oct 15, 2019

@TomAugspurger
Copy link
Contributor

Merging master should fix the CI failures.

@simonjayhawkins simonjayhawkins added the Typing type annotations, mypy/pyright type checking label Oct 16, 2019
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.

@jjlkant Thanks for the PR. looks good.

@simonjayhawkins simonjayhawkins added this to the 1.0 milestone Oct 16, 2019
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.

@@ -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

@jjlkant jjlkant requested a review from WillAyd October 21, 2019 08:53
@WillAyd WillAyd merged commit a8a6929 into pandas-dev:master Oct 23, 2019
@WillAyd
Copy link
Member

WillAyd commented Oct 23, 2019

Thanks @jjlkant

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.

4 participants