-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
web: #2532 - Decode bytes values to strings #2542
Conversation
This commit fixes issue beetbox#2532 by filtering out any byte values added by any other extensions and decoding them to strings for the JSON serializer. It does not remove any of the keys, instead opting to just convert them. Signed-off-by: Mark Stenglein <mark@stengle.in>
@ocelotsloth, thanks for your PR! By analyzing the history of the files in this pull request, we identified @sampsyo, @geigerzaehler and @maxammann to be potential reviewers. |
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.
Hey; thanks for addressing this! I added a couple of suggestions above.
beetsplug/web/__init__.py
Outdated
@@ -42,6 +42,10 @@ def _rep(obj, expand=False): | |||
else: | |||
del out['path'] | |||
|
|||
# Filter all bytes attributes and convert them to strings | |||
for key in filter(lambda key: isinstance(out[key], bytes), out): |
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.
A somewhat more "idiomatic Python" way of doing this would be to use a plain if
. For example:
for key, value in out.items():
if isinstance(value, bytes):
out[key] = ...
beetsplug/web/__init__.py
Outdated
@@ -42,6 +42,10 @@ def _rep(obj, expand=False): | |||
else: | |||
del out['path'] | |||
|
|||
# Filter all bytes attributes and convert them to strings | |||
for key in filter(lambda key: isinstance(out[key], bytes), out): | |||
out[key] = out[key].decode('utf-8') |
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.
Using .decode('utf-8')
will assume that the binary data stored on the object happens to be a UTF-8-encoded string. That's not necessarily the case: the acoustid fingerprint, for example, is really a raw binary blob, not any representation of text at all.
Fortunately, there's a standard library function that can encode bytes as ASCII-safe bytes. Then you can use .decode('ascii')
to get a text string to pass to the JSON encoder.
As suggested, this commit adds base64 encoding for the bytes encoding. Signed-off-by: Mark Stenglein <mark@stengle.in>
As suggested, changes around the for loop to make it a bit more pythonic by using an if loop inside a normal for loop. Signed-off-by: Mark Stenglein <mark@stengle.in>
Thanks for the comments @sampsyo! I went ahead and made the modifications you suggested, and it definitely looks a bit simpler. |
web: #2532 - Decode bytes values to strings
Thank you for your help with this! ✨ I've merged with a changelog entry. |
Looks to solve #2532 by decoding any bytes objects to strings.
I'm not as familiar with the iterable constructs in Python, but I think that I came up with a reasonable solution. I'm open to learning a better way to do this if there is one.