Skip to content

[ WIP] Support RESP3 with hiredis-py parser #3648

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

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

uglide
Copy link
Contributor

@uglide uglide commented May 15, 2025

Requires hiredis-py built from this PR redis/hiredis-py#208

@uglide uglide requested a review from petyaslavova May 15, 2025 15:41
@petyaslavova petyaslavova requested a review from Copilot May 19, 2025 13:57
Copy link

@Copilot Copilot AI left a 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 enhances RESP3 support by integrating a custom hiredis-py parser and aligning the code and tests with the new RESP3 push notification handling. Key changes include:

  • Removing skipif conditions for HIREDIS_AVAILABLE from multiple test suites.
  • Updating the HIREDIS_AVAILABLE version check logic to require hiredis ≥3.2.0.
  • Modifying both synchronous and asynchronous RESP3/hiredis parser implementations to handle push notifications without relying on HIREDIS_AVAILABLE.

Reviewed Changes

Copilot reviewed 16 out of 16 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
tests/test_search.py Adds ResponseError handling in waitForIndex.
tests/test_pubsub.py Removes skipif markers for hiredis in pubsub tests.
tests/test_connection_pool.py Removes skipif markers for hiredis in connection pool tests.
tests/test_cache.py Removes skipif markers for hiredis in cache tests.
tests/test_asyncio/test_search.py Adds ResponseError handling for async waitForIndex.
tests/test_asyncio/test_pubsub.py Removes skipif markers for hiredis in async pubsub tests.
redis/utils.py Updates HIREDIS_AVAILABLE version check logic.
redis/connection.py Removes HIREDIS_AVAILABLE check for protocol 3 push handler.
redis/cluster.py Simplifies push handler condition in cluster executions.
redis/client.py Removes HIREDIS_AVAILABLE check for setting pubsub push handler.
redis/asyncio/connection.py Removes HIREDIS_AVAILABLE check in async read_response.
redis/asyncio/client.py Removes HIREDIS_AVAILABLE check for async pubsub push handler.
redis/_parsers/resp3.py Refactors push response handling by removing extraneous parameters.
redis/_parsers/hiredis.py Implements async and sync push response handling with logging.
redis/_parsers/base.py Adds PushNotificationsParser and AsyncPushNotificationsParser interfaces.
redis/_parsers/init.py Updates exported parser names to include push notifications support.

response = self.handle_push_response(
response, disable_decoding, push_request
)
response = self.handle_push_response(response)
Copy link
Preview

Copilot AI May 19, 2025

Choose a reason for hiding this comment

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

[nitpick] The updated handle_push_response signature no longer accepts disable_decoding and push_request parameters, which deviates from a typical protocol interface. If intentional, please update the documentation to clearly describe the new behavior.

Copilot uses AI. Check for mistakes.

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.

2 participants