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

PubSub: Allow custom encoder #850

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Ododo
Copy link

@Ododo Ododo commented Jan 15, 2022

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

@miguelgrinberg
Copy link
Owner

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.

@Ododo
Copy link
Author

Ododo commented Jan 15, 2022

So there are 2 commits, one In the PubSubManager for decoding and the other for encoding in the RedisManager only (i tested only this).
Do you want me to add encoder parameters for all the manager classes ?

if redis is None:
raise RuntimeError('Redis package is not installed '
'(Run "pip install redis" in your '
'virtualenv).')
self.encoder = encoder
Copy link
Owner

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.

Copy link
Author

Choose a reason for hiding this comment

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

right, thanks

@miguelgrinberg
Copy link
Owner

So there are 2 commits, one In the PubSubManager for decoding and the other for encoding in the RedisManager only (i tested only this).

Sorry, yes, I missed the RedisManager change.

Do you want me to add encoder parameters for all the manager classes ?

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?

@Ododo
Copy link
Author

Ododo commented Jan 17, 2022

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.

@Ododo Ododo force-pushed the allow-custom-encoder branch 2 times, most recently from 568ce70 to af08e53 Compare January 17, 2022 20:03
@Ododo
Copy link
Author

Ododo commented Jan 31, 2022

Hi @miguelgrinberg
I wanted to let you know that this PR is ready to be reviewed.

Kr

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.

None yet

2 participants