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

ElayGelbart commented 2 weeks ago

Pull Request check-list

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

[x] 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 commented 2 weeks ago

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 ?

ElayGelbart commented 1 week ago

@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 commented 11 hours ago

@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.

redis / redis-py