TYP: remove ignore from pandas/tseries/frequencies.py #52623
Conversation
mroeschke
added
Frequency
pandas/tseries/frequencies.py
Outdated
| # "Union[ExtensionArray, ndarray[Any, Any]]", variable has type | ||
| # "Union[DatetimeIndex, TimedeltaIndex, Series, DatetimeLikeArrayMixin]") | ||
| index = index._values # type: ignore[assignment] | ||
| index = values |
There was a problem hiding this comment.
I'm surprised this works but I feel this might potentially hide bugs/inconsistencies in the type annotations. I think this mypy warning is "correct" given the type annotations.
I think this if statement could be if isinstance(index, TimedeltaIndex): If that is the case, TimedeltaIndex._values should be of type TimedeltaArray which is a sub-class of DatetimeLikeArrayMixin - meaning it wouldn't trigger this error. @jbrockmendel
If the above is the case, I think there are two ways to fix it:
- simplify the if condition to
if isinstance(index, TimedeltaIndex):and useindex = cast(TimedeltaArray, index._values) - or better, annotate
TimedeltaIndex._valuesasTimedeltaArray(and still change the if condition)
There was a problem hiding this comment.
or better, annotate TimedeltaIndex._values as TimedeltaArray
seems reasonable
I think this if statement could be if isinstance(index, TimedeltaIndex)
No, TDI is specifically handled a few lines up in the elif is_timedelta64_dtype(index.dtype): section. Looks like this bit is for object dtype. though id be OK with deprecating allowing that here
natmokval
force-pushed
the
37715-remove-mypy-ignore-V
branch
from
9fe8154
to
a1d990b
Compare
|
@twoertwein, could you please take a look at my last changes? Looks like there are no failures in ci, but the last check |
| if isinstance(index, Index) and not isinstance(index, DatetimeIndex): | ||
| if not hasattr(index, "dtype"): | ||
| pass | ||
| elif isinstance(index.dtype, object): |
There was a problem hiding this comment.
I'm sorry, I'm not familiar enough with the code to review these code changes (more comfortable to review only type annotation changes)
|
@natmokval can you merge main |
@jbrockmendel, I can merge main but these changes are in main already (see pr #53120). So I think it's better to close this pr. Sorry, I had to do it earlier. |
Related to #37715
mypy ignore[assignment] was removed from pandas/tseries/frequencies.py