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.

Sign up for GitHub

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

Jump to bottom

Default body to querystring, null encoding #892

Merged
merged 5 commits into from
Mar 17, 2016
Merged

Default body to querystring, null encoding #892

merged 5 commits into from
Mar 17, 2016

Conversation

flovilmart
Copy link
Contributor

fixes #727

encoding - Encoding to be used on setEncoding of response data. If null, the body is returned as a Buffer. Anything else (including the default value of undefined) will be passed as the encoding parameter to toString() (meaning this is effectively utf8 by default). (Note: if you expect binary data, you should set encoding: null.)

gfosco
gfosco reviewed Mar 7, 2016
View reviewed changes
spec/HTTPRequest.spec.js
it("should encode a JSON body", (done) => {
it("should encode a JSON body by default", (done) => {

let options = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we need 'use strict' at the top if you want to use let.

@flovilmart flovilmart force-pushed the flovilmart.httpRequestDefaultContentType branch from 8dec808 to 647532f Compare March 7, 2016 17:56
@facebook-github-bot
Copy link

@flovilmart updated the pull request.

@codecov-io
Copy link

Current coverage is 91.03%

Merging #892 into master will increase coverage by +0.01% as of ebb0d86

@@            master    #892   diff @@
======================================
  Files           72      73     +1
  Stmts         4165    4171     +6
  Branches       862     863     +1
  Methods          0       0       
======================================
+ Hit           3791    3797     +6
  Partial         10      10       
  Missed         364     364

Review entire Coverage Diff as of ebb0d86

Powered by Codecov. Updated on successful CI builds.

@drew-gross
Copy link
Contributor

Looks good to me but since this is a fairly big API change I would prefer having a 2nd reviewer.

@flovilmart flovilmart changed the title Encodes the body by default to application/json if is object Default body to JSON, null encoding Mar 8, 2016
@facebook-github-bot
Copy link

@flovilmart updated the pull request.

nlutsenko
nlutsenko reviewed Mar 8, 2016
View reviewed changes
spec/HTTPRequest.spec.js
@@ -154,31 +156,50 @@ describe("httpRequest", () => {
done();
})
});
it("should encode a JSON body", (done) => {
it("should encode a JSON body by default", (done) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Space before.

@nlutsenko nlutsenko assigned flovilmart and unassigned nlutsenko Mar 8, 2016
@nlutsenko
Copy link
Contributor

Put few thoughts here and there, nothing very bad, but few pieces that feel weird...
Also, if we are using ES6 - can we use it on every line you changed? This is nitpicking... I know...

@facebook-github-bot
Copy link

@flovilmart updated the pull request.

@flovilmart flovilmart force-pushed the flovilmart.httpRequestDefaultContentType branch from 2c30e38 to 66fefa5 Compare March 8, 2016 05:16
@facebook-github-bot
Copy link

@flovilmart updated the pull request.

@flovilmart flovilmart force-pushed the flovilmart.httpRequestDefaultContentType branch from 66fefa5 to 6acb5ce Compare March 8, 2016 13:04
@facebook-github-bot
Copy link

@flovilmart updated the pull request.

1 similar comment
@facebook-github-bot
Copy link

@flovilmart updated the pull request.

@flovilmart flovilmart force-pushed the flovilmart.httpRequestDefaultContentType branch from 2c183f8 to bd7d951 Compare March 8, 2016 13:12
@flovilmart flovilmart changed the title Default body to JSON, null encoding Default body to querystring, null encoding Mar 8, 2016
@facebook-github-bot
Copy link

@flovilmart updated the pull request.

@flovilmart flovilmart assigned nlutsenko and unassigned flovilmart Mar 8, 2016
@flovilmart
Copy link
Contributor Author

@nlutsenko what's your thoughts on that? The default encoding will be QS, to match with original implementation of httpRequest

@nlutsenko nlutsenko assigned flovilmart and unassigned nlutsenko Mar 17, 2016
@nlutsenko
Copy link
Contributor

Looks good, sorry took a while to review.
Matching original encoding of httpRequest sounds good as well.

nlutsenko
nlutsenko reviewed Mar 17, 2016
View reviewed changes
src/cloud-code/HTTPResponse.js
this.cookies = response.headers["set-cookie"];
}

get text() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Love the lazy-loading piece here.

flovilmart added a commit that referenced this pull request Mar 17, 2016
@flovilmart
Merge pull request #892 from ParsePlatform/flovilmart.httpRequestDefa…
0f7335b 
…ultContentType

Default body to querystring, null encoding
@flovilmart flovilmart merged commit 0f7335b into master Mar 17, 2016
@flovilmart flovilmart deleted the flovilmart.httpRequestDefaultContentType branch March 17, 2016 02:43
@cleever cleever mentioned this pull request Jun 24, 2016
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@flovilmart flovilmart

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Parse.Cloud.httpRequest: provide default string encoding for body option
6 participants
@flovilmart @facebook-github-bot @codecov-io @drew-gross @nlutsenko @gfosco