Use hiredis::pack_command to serialized the commands.#2570
Conversation
2) now hiredis.pack_bytes is the default choice. Though it's still possible to run redisrs-py (fix the flag in utils.py) or hiredis.pack_command (flag in connection.py)
2) Extract serialization functionality out of Connection class.
2) Add change log entry. 3) Revert the benchmark changes
Codecov ReportBase: 92.28% // Head: 92.27% // Decreases project coverage by
📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more Additional details and impacted files@@ Coverage Diff @@
## master #2570 +/- ##
==========================================
- Coverage 92.28% 92.27% -0.02%
==========================================
Files 115 115
Lines 29660 29691 +31
==========================================
+ Hits 27373 27396 +23
- Misses 2287 2295 +8
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
| if packer is not None: | ||
| return packer | ||
| elif HIREDIS_PACK_AVAILABLE: | ||
| return HiredisRespSerializer() | ||
| else: | ||
| return PythonRespSerializer(self._buffer_cutoff, self.encoder.encode) |
There was a problem hiding this comment.
seems that elif and else are not needed:
if packer is not None:
return packer
if HIREDIS_PACK_AVAILABLE:
return HiredisRespSerializer()
return PythonRespSerializer(self._buffer_cutoff, self.encoder.encode)
There was a problem hiding this comment.
Technically - yes, we can get rid of else but , my understanding of general sentiment regarding elif in Python is: it's cleaner way to show that conditions intended to be mutually exclusive.
|
@prokazov given that hiredis isn't a dependency, but is an extra (i.e. We need to wrap the pack_command call, with a check to ensure that hiredis is >= 2.2.1, specifically due to this change, otherwise we'll break the community. Similarly @dvora-h this becomes 4.5.0 as discussed. |
|
@chayim Originally the check was checking for a version. It was later changed to check for the existence of the pack_command exposed in the hiredis. So effectively this validates if the hiredis-py contains this new method so it is backwards compatible. |
7 participants
@chayim @dvora-h
Pull Request check-list
Please make sure to review and check all of these items:
[x ] Does
$ toxpass with this change (including linting)?[ x] Do the CI tests pass with this change (enable it first in your forked repo and wait for the github action build to finish)?
[ x] Is the new or changed code fully tested?
[ ?] Is a documentation update included (if this change modifies existing APIs, or introduces new ones)?
[n/a ] Is there an example added to the examples folder (if applicable)?
[ x] Was the change added to CHANGES file?
Historically, hiredis-py (Python extension module written in C and utilizing hiredis library) has been used only for deserialization of the Redis command's replies. With the new hiredis-py 2.2.1 release it's possible to do the opposite operation - serialization of the Python tuples, representing commands in the redis-py into a RESP-encoded bytes buffer. This PR: