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

Body encoding #259

Closed
wants to merge 6 commits into from
Closed

Body encoding #259

wants to merge 6 commits into from

Conversation

farrjere
Copy link

@farrjere farrjere commented May 6, 2018

This should fix #153. I added a test to validate that it does handle encoding issues, also I tested locally on my windows partition that it was broken and isn't now.

I couldn't see any rules for contributing so please let me know if there is something else I need to do. I did sign the agreement though.

@trestletech
Copy link
Contributor

Thanks for proposing this change! It looks like the CI build now has some failed tests in light of this change. Do you mind taking a look at those?

@farrjere
Copy link
Author

farrjere commented May 8, 2018

I am looking into the failures tonight. I'll post back once I have them fixed.

@farrjere farrjere closed this May 8, 2018
@farrjere farrjere reopened this May 8, 2018
@farrjere
Copy link
Author

farrjere commented May 8, 2018

I accidentally closed and commented. The pull request is still valid, I will post once I have the tests taken care of.

@codecov-io
Copy link

codecov-io commented May 8, 2018

Codecov Report

Merging #259 into master will increase coverage by 0.06%.
The diff coverage is 87.5%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #259      +/-   ##
==========================================
+ Coverage   88.26%   88.32%   +0.06%     
==========================================
  Files          24       24              
  Lines        1227     1242      +15     
==========================================
+ Hits         1083     1097      +14     
- Misses        144      145       +1
Impacted Files Coverage Δ
R/response.R 100% <100%> (ø) ⬆️
R/content-types.R 100% <100%> (ø) ⬆️
R/post-body.R 54.54% <60%> (+4.54%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ddf9e3e...572511e. Read the comment docs.

schloerke added a commit that referenced this pull request Jun 4, 2018
This should fix #153.  I added a test to validate that it does handle encoding issues, also I tested locally on my windows partition that it was broken and isn't now. 

#153: Cannot POST in UTF-8 under windows
#259: Body Encoding

* setting encoding to fix windows problems

* changing where setting encoding to ease testing

* adding test for encoding

* fixing encoding test (windows botched commit)

* fixing bad assumption that everything is UTF-8, now checking if the body is character and looking for charset

* adding a test for postBodyFilter

* natural collate order

* lints

Co-authored-by: Jeremy Farrell @farrjere
@schloerke
Copy link
Collaborator

Closing as merged in #264

@schloerke schloerke closed this Jun 4, 2018
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.

Cannot POST in UTF-8 under windows
4 participants