Skip to content

TYP: pandas/core/dtypes/base.py #31352

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

Merged
merged 14 commits into from
Feb 12, 2020

Conversation

simonjayhawkins
Copy link
Member

No description provided.

@simonjayhawkins simonjayhawkins added the Typing type annotations, mypy/pyright type checking label Jan 27, 2020
f"'construct_from_string' expects a string, got {type(string)}"
)

if hasattr(cls, "_match"):
Copy link
Contributor

Choose a reason for hiding this comment

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

what uses this?

Copy link
Member Author

Choose a reason for hiding this comment

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

some repeated logic in pandas/core/dtypes/dtypes.py can be removed with this. will remove for now til needed. (will also be adding a test in the base extension tests for consistent error messages for wrong type to construct_from_string.)

I'll open a PR to break off changes to pandas/core/dtypes/dtypes.py without the deduplication for now.

Copy link
Member

@WillAyd WillAyd left a comment

Choose a reason for hiding this comment

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

lgtm one question

@@ -119,11 +121,11 @@ def __eq__(self, other: Any) -> bool:
def __hash__(self) -> int:
return hash(tuple(getattr(self, attr) for attr in self._metadata))

def __ne__(self, other) -> bool:
def __ne__(self, other: Any) -> bool:
Copy link
Member

Choose a reason for hiding this comment

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

Did we ever decide on consistent use of Any vs object here? Thought we were using object but maybe missed that

Copy link
Member Author

Choose a reason for hiding this comment

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

in this class we already have def __eq__(self, other: Any) -> bool: and in #29947, Any was applied throughout.

I think it should be object in order to check the body of the __eq__/__ne__ methods, but since we want consistency these will need to be changed as well...

__eq__

pandas/core/arrays/interval.py:    def __eq__(self, other):
pandas/core/arrays/sparse/dtype.py:    def __eq__(self, other: Any) -> bool:
pandas/core/arrays/_arrow_utils.py:        def __eq__(self, other):
pandas/core/arrays/_arrow_utils.py:        def __eq__(self, other):
pandas/core/dtypes/base.py:    def __eq__(self, other: Any) -> bool:
pandas/core/dtypes/dtypes.py:    def __eq__(self, other: Any) -> bool:
pandas/core/dtypes/dtypes.py:    def __eq__(self, other: Any) -> bool:
pandas/core/dtypes/dtypes.py:    def __eq__(self, other: Any) -> bool:
pandas/core/dtypes/dtypes.py:    def __eq__(self, other: Any) -> bool:
pandas/core/indexes/frozen.py:    def __eq__(self, other: Any) -> bool:
pandas/io/pytables.py:    def __eq__(self, other: Any) -> bool:
pandas/io/pytables.py:    def __eq__(self, other: Any) -> bool:
pandas/io/stata.py:    def __eq__(self, other: Any) -> bool:
pandas/tests/groupby/test_function.py:        def __eq__(self, other):
pandas/tests/indexing/test_indexing.py:            def __eq__(self, other) -> bool:
pandas/tests/scalar/timedelta/test_timedelta.py:            def __eq__(self, other):
pandas/tests/scalar/timestamp/test_comparisons.py:        def __eq__(self, other) -> bool:
pandas/tests/series/test_ufunc.py:        def __eq__(self, other) -> bool:
pandas/tests/test_algos.py:            def __eq__(self, other) -> bool:
pandas/tseries/offsets.py:    def __eq__(self, other: Any) -> bool:
pandas/_libs/tslibs/offsets.pyx:    def __eq__(self, other: Any) -> bool:

__ne__

pandas/core/arrays/interval.py:    def __ne__(self, other):
pandas/io/pytables.py:    def __ne__(self, other) -> bool:
pandas/tests/series/test_duplicates.py:        def __ne__(self, other):
pandas/tseries/offsets.py:    def __ne__(self, other):
pandas/_libs/tslibs/offsets.pyx:    def __ne__(self, other):

However, doing this raises issues with Extension Array equalities, see #31646. So should be done in a separate PR, I think though that this does affirm the need to use object instead of Any for more thorough checking.

@@ -74,7 +74,7 @@ def register(self, dtype: Type[ExtensionDtype]) -> None:

def find(
self, dtype: Union[Type[ExtensionDtype], str]
) -> Optional[Type[ExtensionDtype]]:
) -> Optional[Union[ExtensionDtype, Type[ExtensionDtype]]]:
Copy link
Contributor

Choose a reason for hiding this comment

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

why is this changed?

Copy link
Member Author

Choose a reason for hiding this comment

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

construct_from_string on L98 returns an instance. However, since the type annotations for construct_from_string were removed following #31352 (comment), this can also be removed for now.

Copy link
Member

@WillAyd WillAyd left a comment

Choose a reason for hiding this comment

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

lgtm outside of Jeff's question

@jbrockmendel
Copy link
Member

This looks fine, any sticking points?

@WillAyd WillAyd added this to the 1.1 milestone Feb 12, 2020
@WillAyd WillAyd merged commit dd03c19 into pandas-dev:master Feb 12, 2020
@WillAyd
Copy link
Member

WillAyd commented Feb 12, 2020

Thanks @simonjayhawkins

@simonjayhawkins simonjayhawkins deleted the pandas/core/dtypes/base.py branch February 12, 2020 20:57
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