Skip to content
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

Refactor(Typing): Add and update type annotations for core Redis commands #3281

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

ElayGelbart
Copy link

Pull Request check-list

Please make sure to review and check all of these items:

  • Do tests and lints pass with this change?
  • Do the CI tests pass with this change (enable it first in your forked repo and wait for the github action build to finish)?
  • Is the new or changed code fully tested?
  • Is a documentation update included (if this change modifies existing APIs, or introduces new ones)?
  • Is there an example added to the examples folder (if applicable)?
  • Was the change added to CHANGES file?

NOTE: these things are not required to open a PR and can be done
afterwards / while the PR is open.

Description of change

Comprehensive Refactoring of Type Annotations for Redis Core Commands

Overview

This pull request introduces a comprehensive update and correction of the type annotations
for all core Redis commands. The primary focus of this effort was to enhance the precision
and usefulness of the type information based on the official Redis documentation.
The task involved meticulously revisiting each command's return type to ensure
accuracy and completeness.

Key Changes

  • Standardization of Return Types: Many core commands lacked proper return type
    annotations.
    some used overly generic types, while others did not accurately reflect
    all possible return values. This update corrects these issues, providing a clear and
    reliable type for each function based on the Redis command specifications.
  • Correction of Types: Adjustments were made to type annotations to
    accurately include possible nil and other replies, addressing previous oversights.
  • Optional Arguments: Some function arguments were incorrectly marked as non-optional.
    This has been corrected, ensuring that optional parameters are appropriately typed.
  • Handling Awaitable Types: Discrepancies in the typing of awaitable and non-awaitable
    unions in return types have been standardized - some commands mention it and some not, now all commands is handled in the same direction of union sync and async responses.
  • Usage of 'OK' Type: The 'OK' type annotation was missing in certain commands
    where it should have been applied. This has now been consistently integrated where applicable.
  • Class Initializations: Instances of classes being initialized more than once have
    been corrected.
  • Compatibility Adjustments: Replaced unsupported below Python 3.8 'list' type usage with
    'typing.List' to maintain compatibility across supported versions.

Challenges and Acknowledgements

  • The task involved extensive review and application of type annotations one by one
    from the Redis documentation. While I have made effort to ensure accuracy, some types might still need adjustments. This effort significantly improves the clarity and reliability of the codebase, even though some
    minor discrepancies might remain.

  • Binary response of 1\0 is marked as integer, need to be corrected after the progression of this PR.

  • Async\Sync - Generic way to handle the differences between async commands and sync commands to get correct typing, for now I didn't want to change logic and only change the typing and continue the same type decision as before.

  • core.py - 6000+ lines of code made it hard to edit and refactor, different classes of core commands can be separated to multiple files to enhance future code contributions.

@ElayGelbart
Copy link
Author

Looks like CI / Python pypy-3.9 cluster-hiredis tests (pull_request) test is failing on something unrelated to this PR changes, can maintainer rerun test ?

Copy link
Contributor

@gerzse gerzse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is pretty impressive, @ElayGelbart ! I left some comments, more about principles I think.

@@ -32,7 +32,14 @@
PatternT = _StringLikeT # Patterns matched against keys, fields etc
FieldT = EncodableT # Fields within hash tables, streams and geo commands
KeysT = Union[KeyT, Iterable[KeyT]]
ResponseT = Union[Awaitable[Any], Any]
OldResponseT = Union[Awaitable[Any], Any] # Deprecated
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not used anymore, I would say just get rid of it.

AnyResponseT = TypeVar("AnyResponseT", bound=Any)
ResponseT = Union[AnyResponseT, Awaitable[AnyResponseT]]
OKT = Literal[True]
ArrayResponseT = List
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two aspects here.

First, List is itself generic, wouldn't it be cleaner to keep that into account? I guess in most places it would end up being ArrayResponseT[Any], but still it would feel more correct.

Second, why define aliases to single types? You can just use List and it would be easier on the reader, I think.

ResponseT = Union[AnyResponseT, Awaitable[AnyResponseT]]
OKT = Literal[True]
ArrayResponseT = List
IntegerResponseT = int
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same point here, just use int, why alias it? At first sight this is a bit of speculative generality.

OKT = Literal[True]
ArrayResponseT = List
IntegerResponseT = int
NullResponseT = type(None)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems not used at all.

ArrayResponseT = List
IntegerResponseT = int
NullResponseT = type(None)
BulkStringResponseT = str
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, why not use str directly?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for clarity, it isn't just any str it is a BulkStringResponseT even better would be a uage of newtype https://docs.python.org/3/library/typing.html#newtype

@ElayGelbart
Copy link
Author

@gerzse Thanks for the response and the comments !

Thank you for your insights on the PR. I understand the concerns about introducing generics and appreciate the complexities it brings.

The primary intent of transitioning from a generic ANY to List types and the introduction of BulkStringResponseT and IntegerResponseT is to make our responses more predictable and iterables clear, which is crucial for robust type development. This is just a starting point, and I recognize it's far from perfect but necessary for gradual, meaningful improvements.

Regarding the differences between RESP2 and RESP3 and the decode options, this PR lays the groundwork for future enhancements. The use of IntegerResponseT as a Boolean in certain contexts is an interim solution, and I'm open to refining these aspects based on your feedback.

I've invested significant effort in these changes and am truly grateful for your time in reviewing them. If the consensus is that generics are redundant at this stage, I am willing to adjust the approach accordingly. I deeply respect the community’s direction and decisions. Thank you for considering these enhancements.

@gerzse
Copy link
Contributor

gerzse commented Jul 3, 2024

@gerzse Thanks for the response and the comments !

Thank you for your insights on the PR. I understand the concerns about introducing generics and appreciate the complexities it brings.

The primary intent of transitioning from a generic ANY to List types and the introduction of BulkStringResponseT and IntegerResponseT is to make our responses more predictable and iterables clear, which is crucial for robust type development. This is just a starting point, and I recognize it's far from perfect but necessary for gradual, meaningful improvements.

Regarding the differences between RESP2 and RESP3 and the decode options, this PR lays the groundwork for future enhancements. The use of IntegerResponseT as a Boolean in certain contexts is an interim solution, and I'm open to refining these aspects based on your feedback.

I've invested significant effort in these changes and am truly grateful for your time in reviewing them. If the consensus is that generics are redundant at this stage, I am willing to adjust the approach accordingly. I deeply respect the community’s direction and decisions. Thank you for considering these enhancements.

@ElayGelbart This PR is a huge step forward, and personally I think we should merge it. I see nobody else voiced an opinion from the community.

I think I understand your intention for introducing IntegerResponseT and BulkStringResponseT. My though is: what if IntegerResponseT will always be int? What if no other type will ever be assimilated to an "integer response"? Does it make sense to have this custom type name introduced at this early stage?

From my point of view generics are definitely not redundant, but the above mentioned type aliases might be. In any case, we must merge this contribution, to take us further in the direction of typing hints alignment. The day when we can run mypy and get no errors is far away, but we'll get closer to it.

Besides the RESP2 vs RESP3 differences you mention, another point to consider in the future is the difference between sync and async. In some places the Union[T, Awaitable[T]] does not fit either in sync code, for example because Awaitable is not iterable, or in async code, because T is not awaitable. Big changes, but let's thing about them later.

@ElayGelbart
Copy link
Author

ElayGelbart commented Aug 3, 2024

any updates ? @gerzse

@@ -5016,8 +4998,8 @@ def hset(
key: Optional[str] = None,
value: Optional[str] = None,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Due to the encoder class and its encode function:

def encode(self, value):

inputs of values can be more than just string, I think something like the following would be more accurate:
Union[str, float, int, bool, bytes, memoryview]

@Graeme22
Copy link

This PR is great! Any change it'll get merged soon/anything I can do to help?
My code is currently littered with # type: ignores on almost every redis function, this would solve it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants