-
-
Notifications
You must be signed in to change notification settings - Fork 18.1k
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 type annotations in pandas.core.dtypes #26029
Conversation
Codecov Report
@@ Coverage Diff @@
## master #26029 +/- ##
==========================================
- Coverage 91.82% 91.82% -0.01%
==========================================
Files 175 175
Lines 52540 52544 +4
==========================================
Hits 48247 48247
- Misses 4293 4297 +4
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #26029 +/- ##
==========================================
- Coverage 91.9% 91.89% -0.01%
==========================================
Files 175 175
Lines 52485 52489 +4
==========================================
- Hits 48235 48234 -1
- Misses 4250 4255 +5
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you provide the errors this was resolving?
pandas/core/dtypes/dtypes.py
Outdated
@@ -1,5 +1,7 @@ | |||
""" define extension dtypes """ | |||
import builtins |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I found some more information on this error:
The suggested approach there is to alias the type rather than import builtins.
However, this comment by Guido might also be of importance:
His example cites a method where a subsequent annotation mangles a method with the name of bytes
with the builtin. I imagine this is the same with the static variable (if you could confirm would be great) so we would have to be pretty careful when typing this module with str
and type
to avoid nuanced errors
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can confirm that within the body of the classes, the class variables str
and type
are clobbering the builtins. So, e.g, in the class itself (but not in its methods, which each have their own scope) isintance("asdf", str)
will throw a TypeError
because the second argument isn't a type or tuple of types. Likewise, isinstance(int, type)
in that location will throw the same error.
Within the methods, things work normally, see e.g.,
pandas/pandas/core/dtypes/dtypes.py
Line 541 in d8af650
if isinstance(dtype, str) and dtype == 'category': |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As mentioned in comment would prefer a type alias to importing builtins based off of approach suggested in referenced issue
lgtm. @WillAyd merge when satisfied |
git diff upstream/master -u -- "*.py" | flake8 --diff
This one was more complicated than other annotation corrections I've done. The following errors persisted even if the two type definitions in
PandasExtensionDtype
andExtensionDtype
were identical:I suspect this has something to do with how mypy handles classes with multiple inheritance and maybe even @property-decorated accessors. See, e.g., python/mypy#6585.
So to address the issue, I left the annotations in pandas.core.dtypes.base.ExtensionDtype.type and .kind (abstract-ish with @Property decorators) alone but for a format change. I set the corresponding
type
andkind
variables inPandasExtensionDtype
to take theAny
type explicitly to "correct" the mypy error. Then to provide actual type checking I explicitly typed thekind
andtype
variables in the subclasses, i.e., CategoricalDtype, DatetimeTZDtype, etc.Not sure if this is the best approach, but it is a functioning approach that provides static type checking.
This commit addresses all the following errors: