Fix: Improve pipeline chaining type hints in typing.py#3919
Fix: Improve pipeline chaining type hints in typing.py#3919gautamraj8044 wants to merge 2 commits intoredis:masterfrom
Conversation
|
Hi, I’m Jit, a friendly security platform designed to help developers build secure applications from day zero with an MVS (Minimal viable security) mindset. In case there are security findings, they will be communicated to you as a comment inside the PR. Hope you’ll enjoy using Jit. Questions? Comments? Want to learn more? Get in touch with us. |
|
Hi @gautamraj8044, thank you for your contribution! We will make a review soon. |
There was a problem hiding this comment.
Pull request overview
This pull request attempts to improve type hints for pipeline command chaining by introducing a PipelineCommandsProtocol and adding it to the ResponseT type union. The goal is to help IDEs and type checkers infer return types when chaining pipeline commands like pipe.set("a", "a1").set("b", "b1").get("a").
Changes:
- Added
PipelineCommandsProtocolto theResponseTtype union - Introduced
PipelineCommandsProtocolprotocol withexecute_commandandpipeline_execute_commandmethods
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| FieldT = EncodableT # Fields within hash tables, streams and geo commands | ||
| KeysT = Union[KeyT, Iterable[KeyT]] | ||
| ResponseT = Union[Awaitable[Any], Any] | ||
| ResponseT = Union[Awaitable[Any], Any, "PipelineCommandsProtocol"] |
There was a problem hiding this comment.
The PipelineCommandsProtocol type is added to the ResponseT union, but this creates a semantic issue. ResponseT is used as the return type for all Redis command methods (e.g., set(), get()). Adding PipelineCommandsProtocol to this union means that when calling redis_client.get("key"), the type checker will infer the return type could be a PipelineCommandsProtocol, which is incorrect - a regular Redis client should never return a Pipeline.
The underlying issue is that command methods are shared between Redis and Pipeline classes, but they should have different return types. A more appropriate solution would involve using TypeVars or Self types to properly model this polymorphism, rather than adding Pipeline to the general response type union.
|
|
||
|
|
||
| class PipelineCommandsProtocol(Protocol): | ||
| def execute_command(self, *args, **options) -> ResponseT: ... |
There was a problem hiding this comment.
The PipelineCommandsProtocol is defined but never actually used as a type constraint anywhere in the codebase. No Pipeline class declares that it implements this protocol, nor is it used in any type annotations besides being added to the ResponseT union. For this protocol to be useful, Pipeline classes should either explicitly state they implement it, or it should be used as a type bound in relevant places. Additionally, the execute_command method in this protocol returns ResponseT, which creates a circular type reference since ResponseT now includes PipelineCommandsProtocol.
| def execute_command(self, *args, **options) -> ResponseT: ... | |
| def execute_command(self, *args, **options) -> Any: ... |
This pull request fixes Issue #3615, which relates to incorrect type hints for chained pipeline commands in the Redis Python client.
What was changed
ResponseTto support more accurate type inference.PipelineCommandsProtocolintyping.pyto correctly model pipelinecommand chaining behavior.
return types when pipeline commands are chained.
Why this improves the codebase
These changes enhance developer experience by:
All changes are limited to type definitions and do not affect runtime behavior.