-
-
Notifications
You must be signed in to change notification settings - Fork 18.6k
TYP: try out TypeGuard #51309
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
TYP: try out TypeGuard #51309
Changes from all commits
76a998c
e12b7a7
1b3ef00
a1d1517
024ed70
0e4d37a
6be7228
b4ee975
e683d19
64e398e
2940b01
35746ac
f8248e5
1d5f3e4
3e0c4a0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,6 @@ | ||
# TODO(npdtypes): Many types specified here can be made more specific/accurate; | ||
# the more specific versions are specified in comments | ||
|
||
from decimal import Decimal | ||
from typing import ( | ||
Any, | ||
Callable, | ||
|
@@ -13,9 +13,12 @@ from typing import ( | |
|
||
import numpy as np | ||
|
||
from pandas._libs.interval import Interval | ||
from pandas._libs.tslibs import Period | ||
from pandas._typing import ( | ||
ArrayLike, | ||
DtypeObj, | ||
TypeGuard, | ||
npt, | ||
) | ||
|
||
|
@@ -38,13 +41,13 @@ def infer_dtype(value: object, skipna: bool = ...) -> str: ... | |
def is_iterator(obj: object) -> bool: ... | ||
def is_scalar(val: object) -> bool: ... | ||
def is_list_like(obj: object, allow_sets: bool = ...) -> bool: ... | ||
def is_period(val: object) -> bool: ... | ||
def is_interval(val: object) -> bool: ... | ||
def is_decimal(val: object) -> bool: ... | ||
def is_complex(val: object) -> bool: ... | ||
def is_bool(val: object) -> bool: ... | ||
def is_integer(val: object) -> bool: ... | ||
def is_float(val: object) -> bool: ... | ||
def is_period(val: object) -> TypeGuard[Period]: ... | ||
def is_interval(val: object) -> TypeGuard[Interval]: ... | ||
def is_decimal(val: object) -> TypeGuard[Decimal]: ... | ||
def is_complex(val: object) -> TypeGuard[complex]: ... | ||
def is_bool(val: object) -> TypeGuard[bool | np.bool_]: ... | ||
def is_integer(val: object) -> TypeGuard[int | np.integer]: ... | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I kind of dislike having composite types inside the type guard (because it probably gives some uglier code in other locations). Maybe only use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i get where youre coming from, but i also have an urge for "maximum accuracy" here. im going to keep it how it is for now but will be OK with changing if consensus develops There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would keep the Union (but it would be nicer without it). I think not having the Unions might cause some incorrect unreachable code warnings from mypy/pyright. if is_integer(x):
if isinstance(x, np.number): # I believe mypy would error here if it wouldn't return a Union
x = x.item() There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To some extent, this change is revealing an issue throughout the pandas code. Sometimes, when we call If it is the case that we accept a numpy integer/bool whenever we have typed an argument as There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think if we want to check that it is a python integer, we'd just do But ok, it was just a preference, I can also accept it, as overall this PR is a nice improvement. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Or for every place that such a change is necessary, it makes you analyze the code and determine whether you really want to change the test to |
||
def is_float(val: object) -> TypeGuard[float]: ... | ||
def is_interval_array(values: np.ndarray) -> bool: ... | ||
def is_datetime64_array(values: np.ndarray) -> bool: ... | ||
def is_timedelta_or_timedelta64_array(values: np.ndarray) -> bool: ... | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -84,13 +84,19 @@ | |
# Name "npt._ArrayLikeInt_co" is not defined [name-defined] | ||
NumpySorter = Optional[npt._ArrayLikeInt_co] # type: ignore[name-defined] | ||
|
||
if sys.version_info >= (3, 10): | ||
from typing import TypeGuard | ||
else: | ||
from typing_extensions import TypeGuard # pyright: reportUnusedImport = false | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd import from standard lib if on python >= 3.10, else from There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This needs to take into account that if PY310:
from typing import TypeGuard
else:
try:
from typing_extensions import TypeGuard
except ImportError:
TypeGuard = bool Also add
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. does that work in an environment.yml file? requirements-dev.txt is auto-generated from the environment.yml. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. checking for py310 requires a circular import which isnt great. im leaning towards abandoning this PR, would be happy if you'd like to get it across the finish line There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh I didn't know it is autogenerated and I not knowledgeable with environment.yml to know if it`s possible there. I could take a look. I can also try take over, no problem. |
||
if sys.version_info >= (3, 11): | ||
from typing import Self | ||
else: | ||
from typing_extensions import Self # pyright: reportUnusedImport = false | ||
else: | ||
npt: Any = None | ||
Self: Any = None | ||
TypeGuard: Any = None | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should probably not be outside of the |
||
|
||
HashableT = TypeVar("HashableT", bound=Hashable) | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1348,4 +1348,5 @@ def _validate_skipfooter_arg(skipfooter: int) -> int: | |
if skipfooter < 0: | ||
raise ValueError("skipfooter cannot be negative") | ||
|
||
return skipfooter | ||
# Incompatible return value type (got "Union[int, integer[Any]]", expected "int") | ||
return skipfooter # type: ignore[return-value] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. could remove def _validate_skipfooter_arg(skipfooter: int | np.integer) -> int | np.integer: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it's not good to change type signatures to keep the type checkers happy, mostly because it could easily spread to be needed everywhere, which would be a PITA. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
But that's part of the point of type checking! The fact that you had to do a |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -250,7 +250,7 @@ def validate_bool_kwarg( | |
""" | ||
good_value = is_bool(value) | ||
if none_allowed: | ||
good_value = good_value or value is None | ||
good_value = good_value or (value is None) | ||
|
||
if int_allowed: | ||
good_value = good_value or isinstance(value, int) | ||
|
@@ -260,7 +260,7 @@ def validate_bool_kwarg( | |
f'For argument "{arg_name}" expected type bool, received ' | ||
f"type {type(value).__name__}." | ||
) | ||
return value | ||
return value # pyright: ignore[reportGeneralTypeIssues] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could also add this file to the long list here https://github.com/pandas-dev/pandas/blob/main/pyright_reportGeneralTypeIssues.json#L17 But I wouldn't mind start using line-by-line ignores. I think we started with file-based ignores as we have in most cases just too many errors. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Things like this will show up many place where we use So I think we will have to either:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You can get rid of the error by changing the sig of the function: def validate_bool_kwarg(
value: bool | int | None | np.bool_, arg_name, none_allowed: bool = True, int_allowed: bool = False
) -> bool | int | None | np.bool_: In this case, the type checker was telling you that |
||
|
||
|
||
def validate_fillna_kwargs(value, method, validate_scalar_dict_value: bool = True): | ||
|
@@ -438,7 +438,7 @@ def validate_insert_loc(loc: int, length: int) -> int: | |
loc += length | ||
if not 0 <= loc <= length: | ||
raise IndexError(f"loc must be an integer between -{length} and {length}") | ||
return loc | ||
return loc # pyright: ignore[reportGeneralTypeIssues] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Change the sig here too: def validate_insert_loc(loc: int | np.integer, length: int) -> int | np.integer: Note - if you really don't want to allow |
||
|
||
|
||
def check_dtype_backend(dtype_backend) -> None: | ||
|
Uh oh!
There was an error while loading. Please reload this page.