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

Don't encode sent message if it is of type bytes #6

Merged
merged 1 commit into from
Oct 7, 2020

Conversation

yehonatanz
Copy link
Contributor

No description provided.

@mlasevich
Copy link
Owner

Thanks for the contribution.

I am trying to understand the use-case - under what scenario would it be already encoded as JSON but not be a string?

I mean, if you do not wish to encode, you can just disable the encode altogether - this check is primarily intended to avoid accidentally double encoding - is there case where it is encoded to json but not a string?

@yehonatanz
Copy link
Contributor Author

This is intended to allow users to encode their messages themselves, including non JSON messages.
Non-JSON messages usually encode to raw bytes, which redis supports nicely.

According to the line I modified a bytes message will always satisfy not isinstance(message, str) and thus will always get encoded, which is not what one would expect when supplying raw bytes.
Perhaps I'm missing something, how does one disable encoding altogether?

@mlasevich
Copy link
Owner

Oh, I got it. It makes sense, though my preference would be to change encode parameter to be tristate (True/False/None) instead of true/false and only autodetect if it is None - it would give more control to user - though it would be a breaking change :-/ ...

@yehonatanz
Copy link
Contributor Author

I agree about the parameter, but it seems to me that even in autodetect mode, bytes shouldn't get encoded, so that PR is still relevant.

@mlasevich
Copy link
Owner

I agree about the parameter, but it seems to me that even in autodetect mode, bytes shouldn't get encoded, so that PR is still relevant.

I can live with that

@mlasevich mlasevich merged commit 54b260b into mlasevich:master Oct 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants