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

base64 encoded responses break unit tests for Python3 #98

Closed
avelis opened this issue Jan 20, 2016 · 3 comments
Closed

base64 encoded responses break unit tests for Python3 #98

avelis opened this issue Jan 20, 2016 · 3 comments

Comments

@avelis
Copy link
Collaborator

avelis commented Jan 20, 2016

Visible under the last build run on TravisCI(https://travis-ci.org/django-silk/silk/jobs/103436926), the enhancement merged #94 isn't quite Python3 compliant.

This has to due with base64 encoder expecting a byte string and not a string.

Potential solution for https://github.com/django-silk/silk/blob/master/silk/model_factory.py#L240:

# Python3 requires a byte-sting
if isinstance(content, str):
    content = content.encode('utf-8')
silky_response.raw_body = base64.b64encode(content)
@avelis
Copy link
Collaborator Author

avelis commented Jan 20, 2016

@trik I know you worked on #94 for issues #85 & #1. Would you mind weighing in on if my potential solution (above) will still keep your enhancement intact?

I want to be certain before I unintentionally break the core part of the feature.

avelis added a commit that referenced this issue Jan 20, 2016
Addresses #98

Change set should make all unit tests pass under TravisCI
@trik
Copy link
Contributor

trik commented Jan 20, 2016

@avelis sorry, i never use python3.. i made some tests and everything seems fine with your patch, requests are correctly decoded also in p3
anyway, in model_factory.py lines 240-244, i would have done:

try
    silky_response.raw_body = base64.b64encode(content)
except TypeError:
    silky_response.raw_body = base64.b64encode(content.encode('utf-8'))

@avelis
Copy link
Collaborator Author

avelis commented Jan 20, 2016

@trik Thanks for your input and suggestions! This will help me move forward with some confidence.

avelis added a commit that referenced this issue Jan 20, 2016
Addresses #98

Change set should make all unit tests pass under TravisCI
@avelis avelis closed this as completed Jan 20, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants