TYP: narrow type bounds on extract_array #46942
Conversation
| @@ -329,7 +337,8 @@ def array( | |||
| if dtype is None: | |||
| inferred_dtype = lib.infer_dtype(data, skipna=True) | |||
| if inferred_dtype == "period": | |||
| return PeriodArray._from_sequence(data, copy=copy) | |||
| period_data = cast(Union[Sequence[Optional[Period]], AnyArrayLike], data) | |||
There was a problem hiding this comment.
wouldn't mind a mypy ignore instead of the cast
There was a problem hiding this comment.
Do you prefer the ignore? I personally think the cast is more semantically correct, since we checked the type using lib.infer_dtype. This pattern also seems to be used in different parts of the codebase already, too ( example).
There was a problem hiding this comment.
I have no strong opinion (slightly in favor of ignore, unless it creates many follow up ignores) @simonjayhawkins has a better overview when to use casts I am/was a bit too liberal using them :)
There was a problem hiding this comment.
we have checks for redundant casts and also unused ignores so if, in general, the typing issue is likely to be fixed in the future then either are OK (but I personally prefer an ignore since we traditionally add the mypy error as a comment, so when looking at the code, the error message is perhaps more helpful than a cast)
In this case, where the issue is that a type if not correctly narrowed by a condition then I am equally happy with a cast adjacent to the condition.
iasoon
force-pushed
the
typing/extract_array
branch
from
dfe1d6f to
28f5759
Compare
iasoon
force-pushed
the
typing/extract_array
branch
from
28f5759 to
ac1fb20
Compare
|
@twoertwein @simonjayhawkins good here? |
|
Thanks @iasoon. #46942 (comment) can be followed up with in a subsequent PR if needed |
not needed. Thanks @iasoon |
xref #37715
Narrowing the type bound allows resolving some ignored mypy errors.
The other modified code is needed because
extract_arrayno longer returnsAny, causing more strict type checking in the calling methods.