-
Notifications
You must be signed in to change notification settings - Fork 403
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 typing issues around Range/RangeValue #1196
base: master
Are you sure you want to change the base?
Conversation
asyncpg/types.py
Outdated
@@ -63,10 +63,10 @@ class _RangeValue(typing.Protocol): | |||
def __eq__(self, __value: object) -> bool: | |||
... | |||
|
|||
def __lt__(self, __other: _RangeValue) -> bool: | |||
def __lt__(self: _RV, __other: _RV) -> bool: |
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 believe def __lt__(self, __other: Self) -> bool:
should work
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.
Unfortunately it does not- I tried that first and settled on what was submitted to get it all working. Here's the output with your suggestion (made to both __lt__
and __gt__
):
% pyright tests/test_types.py
/Users/dmcgee/projects/sbl/asyncpg/tests/test_types.py
/Users/dmcgee/projects/sbl/asyncpg/tests/test_types.py:18:25 - error: Argument of type "Literal[1]" cannot be assigned to parameter "lower" of type "_RV@Range | None" in function "__init__"
Type "Literal[1]" is not assignable to type "_RV@Range | None"
Type "Literal[1]" is not assignable to type "_RangeValue"
"Literal[1]" is incompatible with protocol "_RangeValue"
"__lt__" is an incompatible type
Type "(value: int, /) -> bool" is not assignable to type "(__other: _RV@__lt__, /) -> bool"
"__gt__" is an incompatible type
Type "(value: int, /) -> bool" is not assignable to type "(__other: _RV@__gt__, /) -> bool"
"Literal[1]" is not assignable to "None" (reportArgumentType)
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.
def __lt__(self: _RV, __other: _RV) -> bool: | |
def __lt__(self, __other: Self, /) -> bool: |
This will fix it, checked locally with pyright
as well.
See typeshed
which has the definition of int.__lt__
that pyright
(correctly) consider to not be compatible with _RangeValue.__lt__
:
https://github.com/python/typeshed/blob/ca65e087f1f9dfc28e89192b60ce7cfc2e92c674/stdlib/builtins.pyi#L321
asyncpg/types.py
Outdated
@@ -63,10 +63,10 @@ class _RangeValue(typing.Protocol): | |||
def __eq__(self, __value: object) -> bool: | |||
... | |||
|
|||
def __lt__(self, __other: _RangeValue) -> bool: | |||
def __lt__(self: _RV, __other: _RV) -> bool: |
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.
def __lt__(self: _RV, __other: _RV) -> bool: | |
def __lt__(self, __other: Self, /) -> bool: |
This will fix it, checked locally with pyright
as well.
See typeshed
which has the definition of int.__lt__
that pyright
(correctly) consider to not be compatible with _RangeValue.__lt__
:
https://github.com/python/typeshed/blob/ca65e087f1f9dfc28e89192b60ce7cfc2e92c674/stdlib/builtins.pyi#L321
In order for the `_RangeValue` protocol to type check properly, it needs to reference `Self`, not itself. Both pyright and mypy show a ton of errors when type checking this before these changes. (I've omitted non-relevant errors here): ``` % pyright tests/test_types.py /Users/dmcgee/projects/sbl/asyncpg/tests/test_types.py /Users/dmcgee/projects/sbl/asyncpg/tests/test_types.py:18:25 - error: Argument of type "Literal[1]" cannot be assigned to parameter "lower" of type "_RV@Range | None" in function "__init__" Type "Literal[1]" is not assignable to type "_RV@Range | None" Type "Literal[1]" is not assignable to type "_RangeValue" "Literal[1]" is incompatible with protocol "_RangeValue" "__lt__" is an incompatible type Type "(value: int, /) -> bool" is not assignable to type "(__other: _RV@__lt__, /) -> bool" "__gt__" is an incompatible type Type "(value: int, /) -> bool" is not assignable to type "(__other: _RV@__gt__, /) -> bool" "Literal[1]" is not assignable to "None" (reportArgumentType) /Users/dmcgee/projects/sbl/asyncpg/tests/test_types.py:18:34 - error: Argument of type "Literal[5]" cannot be assigned to parameter "upper" of type "_RV@Range | None" in function "__init__" Type "Literal[5]" is not assignable to type "_RV@Range | None" Type "Literal[5]" is not assignable to type "_RangeValue" "Literal[5]" is incompatible with protocol "_RangeValue" "__lt__" is an incompatible type Type "(value: int, /) -> bool" is not assignable to type "(__other: _RV@__lt__, /) -> bool" "__gt__" is an incompatible type Type "(value: int, /) -> bool" is not assignable to type "(__other: _RV@__gt__, /) -> bool" "Literal[5]" is not assignable to "None" (reportArgumentType) ... % mypy tests/test_types.py | grep arg-type tests/test_types.py:18: error: Argument "lower" to "Range" has incompatible type "int"; expected "None" [arg-type] tests/test_types.py:18: error: Argument "upper" to "Range" has incompatible type "int"; expected "None" [arg-type] ... (19 more errors) ``` After this change, the type checking comes back clean: ``` % pyright tests/test_types.py 0 errors, 0 warnings, 0 informations % mypy tests/test_types.py | grep arg-type <no output> ```
e00885a
to
80502bf
Compare
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.
Thanks!
Great suggestion - I made the update. I also verified that this works for |
Is there anything else I can do here to get this merged? |
Maybe @elprans can help review this? There are a couple other PRs from me related to typing that are also waiting on a review to get merged but I don't want to ping them too much. |
I'm glad to see some type hints were added in 0.30.0! I tried them out, and encountered a minor issue.
In order for the
_RangeValue
protocol to type check properly, it needs to reference the createdTypeVar
, not itself.Both pyright and mypy show a ton of errors when type checking this before these changes. (I've omitted non-relevant errors here):
After this change, the type checking comes back clean:
This slight change appears to match what was expected when
bound=
was implemented: python/typing#59 (comment)