-
Notifications
You must be signed in to change notification settings - Fork 36
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
Support RESP3 #50
base: master
Are you sure you want to change the base?
Support RESP3 #50
Conversation
The hiredis parser does not support RESP3 nor will it support it any time soon (at least unlikely). Therefore it is best to remove it.
Also remove some comments.
@Salakar you assigned this to yourself, do you still want to have a look at it? |
I did sorry, but my time has been 💩 if you'd like to continue without my review that's fine ofc 👍 |
Could we not keep the RESP 2 & 3 parsers separate files and export each for clients to use, that way logic for RESP2 doesn't need to interfere with RESP3 and each can be optimised for their specific protocol version. My understanding from this was that to switch to RESP3 the client needs to send a This also has the added benefit of keeping RESP2 unchanged and stable whilst also future proofing the parser should any new protocol versions need adding in future. |
has anyone taken a look at this anytime recently? happy to help in any way I can, but don't wanna redo someone else's work or step on anyone's toes. |
has anyone taken a look at this anytime recently? |
This adds full support for RESP3 while keeping backwards compatibility. Most things should stay on par performance wise.
My intention here is to first fully support the spec and to improve the performance later on for the new data types. Especially as the logic would become increasingly difficult otherwise due to the new attribute type (data that should be ignored while delivering it to the user in an unspecified way).
I have no strong opinion about the way
push data
andattributes
is implemented and I am happy to get some input about those.@luin @Salakar PTAL