TYP: pandas/core/dtypes/dtypes.py #31384
Conversation
pandas/core/dtypes/dtypes.py
Outdated
| str_type = str | ||
|
|
||
| ExtensionDtypeT = TypeVar("ExtensionDtypeT", bound=ExtensionDtype) |
There was a problem hiding this comment.
When do you need to use ExtensionDtypeT and when can you use ExtensionDtype (both are used in this file/diff)
There was a problem hiding this comment.
not using a typevar for find may be an oversight, I'll look into whether necessary.
There was a problem hiding this comment.
the only callsites are in pandas\core\dtypes\common.py, so find may need typevars once types are added there, and the callsites to there are typed etc. etc.
There does seem to be some resistance to typevars, shall I add now or wait till becomes necessity?
I've also just noticed that the type annotation for dtype in find is also incorrect...
the code is
if not isinstance(dtype, type):
dtype_type = type(dtype)
if issubclass(dtype_type, ExtensionDtype):
return dtype
so dtype parameter can also accept an instance as well as a type. again could update here or in a subsequent PR. only the return type was changed in this PR as this caused errors once construct_from_string was typed as this returns an instance, return annotation on master is only
type.
There was a problem hiding this comment.
The typevar has been removed for now.
pandas/core/dtypes/dtypes.py
Outdated
| raise ValueError(msg) | ||
| else: | ||
| raise ValueError("DatetimeTZDtype only supports ns units") | ||
| self._unit, self._tz = unit.unit, unit.tz |
There was a problem hiding this comment.
Just out of curiosity why did this need to change?
There was a problem hiding this comment.
This gives pandas\core\dtypes\dtypes.py:684: error: "str" has no attribute "tz" which looks weird considering the isinstance check and maybe a mypy issue.
if we split the tuple assignment (which would obviously be wrong), may explain the message
if isinstance(unit, DatetimeTZDtype):
reveal_type(unit)
reveal_type(unit.unit)
reveal_type(unit.tz)
unit = unit.unit
tz = unit.tz
reveal_type(unit)
pandas\core\dtypes\dtypes.py:683: note: Revealed type is 'pandas.core.dtypes.dtypes.DatetimeTZDtype'
pandas\core\dtypes\dtypes.py:684: note: Revealed type is 'builtins.str'
pandas\core\dtypes\dtypes.py:685: note: Revealed type is 'Any'
pandas\core\dtypes\dtypes.py:687: error: "str" has no attribute "tz"
pandas\core\dtypes\dtypes.py:688: note: Revealed type is 'builtins.str'
There was a problem hiding this comment.
Yea that seems weird. If we just type ignore does anything else need to change? Slight preference for keeping as is and opening an issue on mypy for narrowing issue, if one doesn’t already exist
There was a problem hiding this comment.
reversing the assignment does not give a false positive, so could do this instead..
tz, unit = unit.tz, unit.unit
There was a problem hiding this comment.
or we could ignore.
There was a problem hiding this comment.
Reversing works as well but in any case would be helpful to raise with mypy
|
can you rebase |
pandas/core/dtypes/dtypes.py
Outdated
| @@ -65,7 +85,7 @@ def register(self, dtype: Type[ExtensionDtype]) -> None: | |||
| """ | |||
| Parameters | |||
| ---------- | |||
| dtype : ExtensionDtype | |||
| dtype : Type[ExtensionDtype] | |||
There was a problem hiding this comment.
Can you put this in a non-typing way? Eg "ExtensionDtype class"
|
this looks fine; any sticking points? |
|
@simonjayhawkins can you merge master? |
| @@ -13,6 +24,15 @@ | |||
| from pandas.core.dtypes.generic import ABCCategoricalIndex, ABCDateOffset, ABCIndexClass | |||
| from pandas.core.dtypes.inference import is_bool, is_list_like | |||
|
|
|||
| if TYPE_CHECKING: | |||
| import pyarrow # noqa: F401 | |||
There was a problem hiding this comment.
This means that running mypy (eg in a pre-commit hook) requires pyarrow to be installed? (which is not a required dependency?)
Are we fine with that? (do we already do that for other deps?)
There was a problem hiding this comment.
This means that running mypy (eg in a pre-commit hook) requires pyarrow to be installed?
no. mypy is a static checker.
The behavior if pyarrow is not installed depends on the strictness of the type checking.(ignore_missing_imports config option)
in general,"mypy will assume the type of that module is Any, the dynamic type. This means attempting to access any attribute of the module will automatically succeed"
https://mypy.readthedocs.io/en/stable/running_mypy.html#missing-imports
|
I think this was lost in the shuffle @simonjayhawkins . If you can merge master to be safe just merge on green |
No description provided.