-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Improved type annotations #3619
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
base: master
Are you sure you want to change the base?
Conversation
136b0a2
to
b6d4a89
Compare
Hi @mahenzon, thank you for your contribution. We will go over it soon. |
def0478
to
38eb7c3
Compare
38eb7c3
to
de3cd7e
Compare
Would this solve #3574? |
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.
Pull Request Overview
This PR improves type annotations across the codebase to ensure that both synchronous and asynchronous Redis clients report more precise return types that are compatible with static type checkers. Key changes include the introduction of new type variables in redis/typing.py, refined method return types in redis/commands/core.py (including generic-based command classes), and updates to command parser mappings in redis/_parsers/helpers.py.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
File | Description |
---|---|
redis/typing.py | Added new type definitions and ResponseType* type variables |
redis/commands/helpers.py | Updated function signature for list_or_args to support Optional args |
redis/commands/core.py | Refactored return types for various command methods using generics |
redis/_parsers/helpers.py | Adjusted SMISMEMBER parser entries |
Awaitable[List[Union[Literal[0], Literal[1]]]], | ||
List[Union[Literal[0], Literal[1]]], | ||
]: | ||
def smismember(self, name: str, values: List, *args: List) -> ResponseTypeBoolean: |
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.
The return type for smismember is annotated as ResponseTypeBoolean but the underlying parser returns a list of booleans. Consider defining and using a dedicated type (e.g., ResponseTypeBooleanList) to accurately reflect the returned value.
Copilot uses AI. Check for mistakes.
@@ -777,6 +777,7 @@ def string_keys_to_dict(key_string, callback): | |||
"SENTINEL SET": bool_ok, | |||
"SLOWLOG GET": parse_slowlog_get, | |||
"SLOWLOG RESET": bool_ok, | |||
"SMISMEMBER": lambda r: list(map(bool, r)), |
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.
[nitpick] Duplicate entries for the SMISMEMBER key are added in this dictionary. Consolidating these duplicates could improve maintainability and clarity.
Copilot uses AI. Check for mistakes.
Would love to see this merged! |
BooleanType = bool | ||
IntegerType = int | ||
OptionalStringType = Optional[str] | ||
StringListType = List[str] |
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.
StringListType = List[str] | |
StringListType = list[str] |
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.
Though it won't be a problem for me since I'm on the latest Python, but using the small list
version will cause issues with those on 3.8 which means something like this cannot be used with say Kodi
|
||
ResponseTypeBoolean = TypeVar( | ||
"ResponseTypeBoolean", | ||
bound=Union[Awaitable[BooleanType], BooleanType], |
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.
bound=Union[Awaitable[BooleanType], BooleanType], | |
bound=Union[Awaitable[bool], 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.
If we're not after 3.8 compatibility, then I think it should be
bound=bool | Awaitable[bool]
ruff has a rule that would do that swap along with removing Optional
in place of | None
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.
If we're not after 3.8 compatibility, then I think it should be
According to the README, the current version of this project (6.2.0) seems to support Python 3.9+ meaning the list[str]
syntax above is usable, but the bool | Awaitable[bool]
syntax would not be, as this was introduced in Python 3.10 (source)
IntegerType = int | ||
OptionalStringType = Optional[str] | ||
StringListType = List[str] | ||
OptionalStringListType = Optional[List[str]] |
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.
OptionalStringListType = Optional[List[str]] | |
OptionalStringListType = Optional[list[str]] |
) | ||
ResponseTypeInteger = TypeVar( | ||
"ResponseTypeInteger", | ||
bound=Union[Awaitable[IntegerType], IntegerType], |
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.
bound=Union[Awaitable[IntegerType], IntegerType], | |
bound=Union[Awaitable[int], int], |
@@ -1820,7 +1838,7 @@ def expiretime(self, key: str) -> int: | |||
""" | |||
return self.execute_command("EXPIRETIME", key) | |||
|
|||
def get(self, name: KeyT) -> ResponseT: | |||
def get(self, name: KeyT) -> ResponseTypeOptionalString: |
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.
get() returns bytes, not str.
>>> import redis
>>> client = redis.Redis()
>>> client.set("foo", b"abcdef")
True
>>> client.get("foo")
b'abcdef'
>>> client.set("bar", "abcdef")
True
>>> client.get("bar")
b'abcdef'
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.
Yeah, that's a major issue with decode_responses=True
or decode_responses=False
If True, then str is returned. Otherwise bytes.
I'm going to try to fix this with overrides and Literals, not sure yet how to implement this. Other way is just to implement types, like in some other libs. Still working on a solution here
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.
It may need to be asserted by the user e.g.
if not isinstance(res, str):
msg = f"Expected str type got {type(res)}"
raise TypeError(msg)
But if decodeResponses is False
then return bytes
But if decodeResponses is True
then return any
that should be doable by @overload
right?
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.
Other way is just to implement types, like in some other libs. Still working on a solution here
I assume you mean having .pyi
files for typing in some of the files? My opinion carries no weight in this project, but I think I'd be a +1 on this approach. Having stub files for these commands seems like a nice balance between several options.
It seems quite difficult to accurately represent the type annotations in a clean way due to the way the command classes are structured. I think the approach taken in this PR is interesting, and works for this scenario, but "feels" quite hacky (I realise this is not necessarily the most constructive criticism), I had to double-take a couple of times to figure out what each generic represents.
Alternative Approaches
Higher-Kinded Generics (Would be nice)
If Python supported higher-kinded TypeVars, it could be semi-cleanly implemented with something like
from collections.abc import Awaitable
from typing import Annotated, Generic, TypeVar
U = TypeVar("U")
A = Annotated[U, ...]
T = TypeVar("T", A, Awaitable)
class Base(Generic[T]):
def ping(self) -> T[bool]:
return True
class AsyncClient(Base[Awaitable]):
pass
class Client(Base[A]):
pass
Client().ping() # Should be `bool`
AsyncClient().ping() # Should be `Awaitable[bool]`
Where A
here represents an opaque type designed to be "invisible" to the end-user.
Type Overriding
It is also possible to override the types of the superclass functions in a subclass like
class Superclass:
def get(self, name: KeyT) -> Any | Awaitable[Any]: ...
class Subclass(Superclass):
if TYPE_CHECKING:
def get(self, name: KeyT) -> Awaitable[str | byets]: ...
But this will become very verbose for this project having to re-type all of the functions inside the source code.
Stubs
Which brings me back to the idea of stubs, the stub files provided by typeshed appeared to accurately represent most of the cases for redis-py including overloads for decode_responses
which shows how it is possible to represent this behaviour in the stub files.
IMO, this is a nice balance between having a "straightforward" implementation while also not polluting the code with hundreds of lines of typing information.
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 think it's best to just leave it as any
You cannot have
redis.Redis(decode_responses: Literal[True])
redis.Redis(decode_responses: Literal[False])
having two different signatures on construction.
I am thinking the use of str | bytes
will cause more problems for the users of the library. Ideally there would be a
make_redis(decode_responses: Literal[True]) -> ResponseDecodingRedis
make_redis(decode_responses: Literal[False]-> Redis
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.
Well, yes, we can't benefit from overloading __init__
directly since it doesn't return anything. But there're ways around. I see it this way:
protocols
from typing import Protocol
class RedisInterface(Protocol):
def get(self, key: str) -> str | bytes | None: ...
class RedisDecoded(Protocol):
def get(self, key: str) -> str | None: ...
class RedisEncoded(Protocol):
def get(self, key: str) -> bytes | None: ...
implementation
from redis.protocols import RedisInterface
# fake implementation just for example
class RedisImplementation(RedisInterface):
def __init__(self, decoded: bool = False):
self.decoded = decoded
def get(self, key: str) -> str | bytes | None:
if key == "get-none":
return None
if self.decoded:
return "foo"
return b"bar"
overloads (for example, stored in redis.typed
)
@overload
def Redis() -> RedisEncoded: ...
@overload
def Redis(decoded: Literal[False] = ...) -> RedisEncoded: ...
@overload
def Redis(decoded: Literal[True] = ...) -> RedisDecoded: ...
def Redis(decoded: bool = False) -> RedisEncoded | RedisDecoded | RedisImplementation:
return RedisImplementation(decoded=decoded)
usage
from redis.typed import Redis
redis = Redis()
redis_decoded = Redis(decoded=True)
redis_encoded = Redis(decoded=False)
reveal_type(redis.get("get-none"))
# MyPy: Revealed type is "Union[builtins.bytes, None]"
# Runtime type is 'NoneType'
reveal_type(redis.get("f"))
# MyPy: Revealed type is "Union[builtins.bytes, None]"
# Runtime type is 'bytes'
reveal_type(redis_encoded.get("get-none"))
# MyPy: Revealed type is "Union[builtins.bytes, None]"
# Runtime type is 'NoneType'
reveal_type(redis_encoded.get("f"))
# MyPy: Revealed type is "Union[builtins.bytes, None]"
# Runtime type is 'bytes'
reveal_type(redis_decoded.get("get-none"))
# MyPy: Revealed type is "Union[builtins.str, None]"
# Runtime type is 'NoneType'
reveal_type(redis_decoded.get("f"))
# MyPy: Revealed type is "Union[builtins.str, None]"
# Runtime type is 'str'
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.
So basically have a separate .typed
package and leave the originals untyped. I guess considering the current state of the code that would be a good way of deferring the debt.
Pull Request check-list
Reviewed and checked all of these items:
Is a documentation update included (if this change modifies existing APIs, or introduces new ones)?- no API changeschecks.py
file example to the repo?Description of change
Hello, there're some issues with type annotations: #2399 #3169
Example:
We can see that sync redis client may return awaitable result, but it will never do.
This is made for compatibility with async redis, but it introduces some challenges when checking code with static type checkers like mypy.
Also it's
Any
instead ofstr
orbytes
because we can't predict, ifdecode_responses
isTrue
orFalse
- it'll be addressed later.I'd like to make it work this way:
I started reworking annotations, so type annotations for sync / async redis work as expected - sync redis doesn't return
Awaitable
, async redis returnsAwaitable
.Important
The goal is not to make the whole
redis-py
source code mypy-comliant and fully annotated.The goal is to make usage of
redis-py
in python projects compatible with mypy - make return type annotations more precise. So devs don't need to addcast
s andassert
s to their code.Example code. where I checked new typing
File:
checks.py
:I checked it with mypy with
strict=True
enabled:mypy --strict checks.py
, added comments to the code example above.If these changes are ok for
redis-py
, I'll continue working on the update.There's one major issue: these annotations are ok when
decode_responses=True
, but if not passed, ordecode_responses=False
, then some methods will returnbytes
instead ofstr
- I'm working on a solution for this.Also I fixed parsing for some commands which should return bools: 23ff994