added explicit typing to all async pipeline commands#8325
added explicit typing to all async pipeline commands#8325srittau merged 11 commits intopython:masterfrom
Conversation
This comment has been minimized.
This comment has been minimized.
stubs/redis/redis/asyncio/client.pyi
Outdated
| CommandStackT: TypeAlias = list[CommandT] | ||
|
|
||
| class Pipeline(Redis[_StrType], Generic[_StrType]): | ||
| class Pipeline(Generic[_StrType]): |
There was a problem hiding this comment.
Not a full review yet, but since Pipeline derives from Redis at runtime, we should keep that relationship. This unfortunately means that we will need lots of # type: ignore[override]. See also redis.client.Pipeline.
This comment has been minimized.
This comment has been minimized.
1 similar comment
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
srittau
left a comment
srittau
left a comment
There was a problem hiding this comment.
Thanks! While I didn't verify every argument type, things are looking good. There's only one thing I noticed: Could you replace instances of Any | None with Incomplete | None? _typeshed.Incomplete is a fairly new alias for Any (so it's not used much yet in typeshed) that's supposed to mark types that can be improved as opposed to Any, which is used for types where we can't do better within the capabilities of the type system.
This comment has been minimized.
This comment has been minimized.
| redis.asyncio.client.Pipeline.command_info | ||
| redis.asyncio.client.Pipeline.debug_segfault | ||
| redis.asyncio.client.Pipeline.memory_doctor | ||
| redis.asyncio.client.Pipeline.memory_help | ||
| redis.asyncio.client.Pipeline.script_debug | ||
| redis.asyncio.client.Pipeline.shutdown |
There was a problem hiding this comment.
These allowlist entries are all because the methods in question are async def methods at runtime but not async def methods in the stub.
- Are you sure it's correct to have them as not being
async defin the stub? - If you are sure, can you put these allowlist entries in a separate section, and add a comment explaining why we're ignoring the stubtest errors? I don't think these entries belong in the "unclear problems" section, since we know why stubtest is complaining here.
There was a problem hiding this comment.
Hi @AlexWaygood
I'm sure in the sense that it doesn't really matter, in principle this function should be syncronous in the pipeline, just like all other redis commands. In practice, because the functions are defined as async in the superclass unlike most commands (I can't really tell why), then any attempt to use these commands in a pipeline would result in a bug.
I will place these entreis in their own section
|
According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉 |
AlexWaygood
dismissed
their stale review
Not sure I really understand what's going on here, but I'm happy for @srittau to merge if he's happy!
srittau
left a comment
srittau
left a comment
There was a problem hiding this comment.
LGTM, if we need to tweak the stubs later, we can always do so.
3 participants
No description provided.