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

update response to no longer encode to utf-8; include tests indicating why #92

Merged
merged 3 commits into from
Oct 21, 2020

Conversation

p5k6
Copy link
Contributor

@p5k6 p5k6 commented Oct 20, 2020

So - I (a long time ago) pinned my version to 0.17, and never got around to looking into this. Hence why i haven't really discussed much in #85. Apologies there, life just got in the way.

fwiw - my producer setup is the same as Matthew's noted here.

I dug in pretty deep today. Some notes:

  • Content-Type charset was set correctly in my app, returning application/json;charset=UTF-8. This ensures the response object from requests has the correct encoding (though interestingly, apparent_encoding(i.e. chardet.detect did not do a great job of detecting the encoding, but that's spurious really).
  • setting the _content object directly to the bytes value mimicked what I saw when I worked with our staging app (the mocked data for resp._content was copied over from there, using an api gateway/lambda serverless app)
  • I could not get awscurl to work with python 2.7 anymore (too many broken packages), so I have not tested this fix on python 2. However, I'd argue that this is okay, given python 2's retirement
  • The test case probably isn't the cleanest (the params are all ignored, I was just being lazy). The only thing that really matters is the response in the new test case really.

Let me know if you have any questions/comments/concerns

@okigan
Copy link
Owner

okigan commented Oct 21, 2020

@p5k6 thanks for pull request!!, seems it failed python 3.5 https://travis-ci.org/github/okigan/awscurl/jobs/737570548

(i'll check why the failure is not shown in github)

@okigan
Copy link
Owner

okigan commented Oct 21, 2020

@p5k6 github config updated you should see checks for you pull request (once you update it) show up like in here: #94

@okigan
Copy link
Owner

okigan commented Oct 21, 2020

@p5k6 hmm, python 3.5 may have dependencies issue for the test library which is unrelated -- please rebase your pull request onto master and re-push

@p5k6
Copy link
Contributor Author

p5k6 commented Oct 21, 2020

Done 😄

@okigan
Copy link
Owner

okigan commented Oct 21, 2020 via email

@p5k6
Copy link
Contributor Author

p5k6 commented Oct 21, 2020

I'll have to take a look tomorrow. Spending some time with my family for a bit. Appreciate your help!

@p5k6
Copy link
Contributor Author

p5k6 commented Oct 21, 2020

I've updated the test to account for python 2 behavior. In Py2, encoding the unicode into an 8-bit string (.encode('utf-8')), and a unicode object (expected) cannot be directly compared to the encoded string. In py3, this does not raise an exception, and the two can be compared directly, hence why there are differing tests based upon python version number.

@okigan
Copy link
Owner

okigan commented Oct 21, 2020

@p5k6 I believe file encoding marker still needs to be added like shown here: https://github.com/p5k6/awscurl/pull/1/files

@p5k6
Copy link
Contributor Author

p5k6 commented Oct 21, 2020

Ah yes - my fault there. Running the tests one more time, will push once they've passed 😀

@p5k6
Copy link
Contributor Author

p5k6 commented Oct 21, 2020

updated

@okigan
Copy link
Owner

okigan commented Oct 21, 2020

@okigan okigan merged commit 86f5c1e into okigan:master Oct 21, 2020
@okigan
Copy link
Owner

okigan commented Oct 21, 2020

@p5k6 new release is on pypi: https://pypi.org/project/awscurl/0.21/

macos/homebrew build would need to be updated (as a separate thread)

@p5k6
Copy link
Contributor Author

p5k6 commented Oct 21, 2020

awesome thank you!

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.

None yet

2 participants