-
-
Notifications
You must be signed in to change notification settings - Fork 597
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
PubSub: Allow custom encoder #850
base: main
Are you sure you want to change the base?
Conversation
As far as I can see this PR is incomplete. You are decoding messages using the provided encoder, but you are not encoding with it. |
So there are 2 commits, one In the PubSubManager for decoding and the other for encoding in the RedisManager only (i tested only this). |
src/socketio/redis_manager.py
Outdated
if redis is None: | ||
raise RuntimeError('Redis package is not installed ' | ||
'(Run "pip install redis" in your ' | ||
'virtualenv).') | ||
self.encoder = encoder |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't necessary, right? The parent class also stores this attribute.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
right, thanks
Sorry, yes, I missed the RedisManager change.
Yes, I want to have parity across these classes. The build results also show that now there is a portion of the code that isn't covered by tests. Would you be able to address that as well? |
Yes, will do. |
568ce70
to
af08e53
Compare
b069fc5
to
ed5679a
Compare
af08e53
to
4239331
Compare
Hi @miguelgrinberg Kr |
ab33cb7
to
8e3460c
Compare
Hello.
I wanted to introduce this functionality because i needed the RedisManager to use Json instead of pickle for encoding data, so that other non-python applications listening to the redis channel can decode the data more easily.
I extended this to allow any encoder object to be used basically.
Thanks,
Olivier