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

p5k6 commented 3 years ago

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 commented 3 years ago

@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 commented 3 years ago

@p5k6 github config updated you should see checks for you pull request (once you update it) show up like in here: https://github.com/okigan/awscurl/pull/94

okigan commented 3 years ago

@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 commented 3 years ago

Done 😄

okigan commented 3 years ago

Wasn’t sure what breaks for python 2.7 - seems it’s missing encoding marker - made or for your branch.

It progressed more - check it out (python encoding is not fresh in my memory)

On Oct 20, 2020, at 7:13 PM, Josh Stanfield notifications@github.com wrote:

Done 😄

— You are receiving this because you commented. Reply to this email directly, view it on GitHub, or unsubscribe.

p5k6 commented 3 years ago

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

p5k6 commented 3 years ago

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 commented 3 years ago

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

p5k6 commented 3 years ago

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

p5k6 commented 3 years ago

updated

okigan commented 3 years ago

Nice!, checks in github passed (https://github.com/okigan/awscurl/actions/runs/320251739), waiting for https://travis-ci.org/github/okigan/awscurl/builds/737771305 to complete

okigan commented 3 years ago

@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 commented 3 years ago

awesome thank you!

okigan / awscurl

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