-
Notifications
You must be signed in to change notification settings - Fork 6
RESP3 parse and encode support #18
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
Conversation
|
I wrote this over a year ago, I believe I completed the implementation but if there was anything left to implement I have now forgotten. |
|
Also, some of the details like the specific way Push ( |
|
Additional note: though the RESP3 spec allows Map and Attribute keys to be any data type, but since they are best represented by hashes in Perl, they will be coerced to and specified as strings, as the specification suggests to fit the language's data types. |
|
The specification suggests using hashes to represent the Set type, but since hash keys can only be strings and Sets can contain any data type, and the protocol itself does not enforce element uniqueness, I chose to represent them as arrays instead so the values are not coerced to strings. The encoder allows specifying the Map type as an array of alternating keys and values, in case the order in which they are encoded matters (though the specification refers to Map as unordered) or to allow encoding non-string Map keys. |
|
I should probably add mention of these implementation details to the documentation. |
|
Hello! Thank you for the PR, it looks good! I have a question if it is possible to subclass version 3 and version 2 so we avoid statements like |
|
Do you mean for example, create a Protocol::Redis::APIv3 subclass that contains a separate implementation of parse and encode methods? That sounds reasonable to me. |
|
Actually I can call it ::RESP2 and ::RESP3, to separate it from the need to provide different API methods in the future. |
|
Do you think it makes sense to use |
|
|
There's one problem with the subclass approach. If sub new returns a different subclass it will no longer be usable by subclasses to bless into their own package. An alternative that doesn't complicate the subclassing is just to have separate parse and encode functions within the main package. |
|
I changed it to be implemented that way. Either way I don't think reusing code between them is useful enough to warrant the performance loss of adding subroutine calls to the encode and parse loops. |
|
I was thinking about two options that a user might want, if it would make sense to add options:
|
|
Both of these options could also be solved by a wrapper or higher level module based on the values returned, though. |
|
I realized that I had missed a factor of the Verbatim String type, which is that the first 4 bytes are required to be a 3 character format followed by a colon. It will now parse that out from the string data and add it to the string when encoding. |
|
I also removed support for "nil" values by specifying -1 as the length of a string or array, since that is fully replaced by the null type and not supported by RESP3. I believe I had left it in just to simplify the combined logic. |
|
Approved. I suppose failing windows tests are not related to the PR. |
RESP3 protocol documented at https://github.com/redis/redis-specifications/blob/master/protocol/RESP3.md . Since the original protocol is now referred to as RESP2, I allowed
api => 2to act asapi => 1, and definedapi => 3as enabling RESP3 support.