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

Add pack_command to support writing via hiredis-py #147

Merged
merged 21 commits into from
Jan 30, 2023

Conversation

prokazov
Copy link
Contributor

As of now, hiredis-py only exposes Reader functionality, that helps to speed up the RESP deserialization.
pack_command serializes redis-py command (tuple of str, bytes, int or float) to a bytes object with the RESP representation of the command.

hiredis/version.py Outdated Show resolved Hide resolved
src/pack.c Outdated Show resolved Hide resolved
Copy link
Contributor

@chayim chayim left a comment

Choose a reason for hiding this comment

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

Big fan, very small changes. Any chance you've finished up on the redis-py PR we were discussing that shows covered usage?

@chayim chayim added the feature label Jan 23, 2023
2) Reverted version bump.
3) Added an entry to the changelog.
2) Reverted version bump.
3) Added an entry to the changelog.
@prokazov
Copy link
Contributor Author

Any chance you've finished up on the redis-py PR ...
I'll publish a draft PR soon but as of now:
redis/redis-py@master...prokazov:redis-py:master
It's kind of non-trivial to properly test redis-py against unpublished hiredis-py.

@prokazov prokazov requested a review from chayim January 23, 2023 18:13
@prokazov
Copy link
Contributor Author

I mean I tested the redis-py with the new pack_command locally, but its CI/CD actions target the stock hiredis-py.
The changes above only cover the regular ("sync") API. The async one would be basically a "copy-paste" of the sync API (with a review feedback). For some reason the async redis-py follows that powerful design pattern.

@chayim
Copy link
Contributor

chayim commented Jan 25, 2023

@prokazov Want to tag me and @dvora-h there... we can continue there. This looks good. Once CI reports in, etc, I'll prep a release - for today or Sunday.

setup.py Outdated Show resolved Hide resolved
Copy link
Contributor

@chayim chayim left a comment

Choose a reason for hiding this comment

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

Changes, as per Bsymbolic build fails

@zalmane
Copy link
Contributor

zalmane commented Jan 25, 2023

@chayim I pushed a commit with conditional flags. Can you re-run the CI? Are there any wheels built not covered by the CI? I excluded win and darwin platforms which don't support the flag. The CI failure of the ubuntu on 3.7 seemed unrelated.

@chayim
Copy link
Contributor

chayim commented Jan 30, 2023

Looks great. Given a past bug, I just added #148. I'll merge that into your branch, and see it through. If we're good there, we're ready to merge!

@chayim chayim changed the title Add pack_command function. Add pack_command to support writing via hiredis-py Jan 30, 2023
@chayim chayim merged commit 4b913ef into redis:master Jan 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants