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

Support JSON stringify replacer argument. #2009

Merged
merged 1 commit into from Mar 26, 2016
Merged

Support JSON stringify replacer argument. #2009

merged 1 commit into from Mar 26, 2016

Conversation

elyobo
Copy link
Contributor

@elyobo elyobo commented Jan 13, 2016

Adds support for the replacer argument of JSON.stringify, in the same
way that request already supports the revivier argument for JSON.parse.

@elyobo
Copy link
Contributor Author

elyobo commented Jan 14, 2016

Updated to remove extra semicolon.

@elyobo
Copy link
Contributor Author

elyobo commented Mar 25, 2016

@mikeal Anything I can do to move this one forward? It seems like a simple change and complements the existing use of the reviver when parsing JSON, but I haven't heard a yeah, a nay or a please-change-this from anyone.

@mikeal
Copy link
Member

mikeal commented Mar 25, 2016

So, I'm +1 on adding this for now but it should be named something else, like "jsonStringify".

@elyobo
Copy link
Contributor Author

elyobo commented Mar 26, 2016

Thanks @mikeal.

Just to explain my reasoning, I went with jsonReplacer as, like the existing jsonReviver argument, it reflects the name of the argument parsed to the underlying JSON.stringify call (as reviver reflects the argument to JSON.parse) and I thought that those names would be more obvious to users because of that.

That said, if you're sure that something like jsonStringifier instead I'll make that change and repush.

@elyobo
Copy link
Contributor Author

elyobo commented Mar 26, 2016

Possibly something like jsonParseReviver and jsonStringifyReplacer would be more descriptive for both of them.

@elyobo
Copy link
Contributor Author

elyobo commented Mar 26, 2016

Test failures appear unrelated...

@@ -749,6 +749,7 @@ The first argument can be either a `url` or an `options` object. The only requir
- `postambleCRLF` - append a newline/CRLF at the end of the boundary of your `multipart/form-data` request.
- `json` - sets `body` to JSON representation of value and adds `Content-type: application/json` header. Additionally, parses the response body as JSON.
- `jsonReviver` - a [reviver function](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/JSON/parse) that will be passed to `JSON.parse()` when parsing a JSON response body.
- `jsonReplacer` - a [replacer function](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/JSON/stringify) that will be passed to `JSON.string()` when stringifying a JSON request body.
Copy link
Member

Choose a reason for hiding this comment

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

The function is called JSON.stringify() not JSON.string()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ta, fixed. Got it right in the URL, not sure what I was thinking in the comment.

Adds support for the replacer argument of JSON.stringify, in the same
way that request already supports the revivier argument for JSON.parse.

Change jsonReplacer arg to jsonStringifier.

Revert "Change jsonReplacer arg to jsonStringifier."

This reverts commit 40c03af.
@simov
Copy link
Member

simov commented Mar 26, 2016

@elyobo don't squash commits before the final review, now I can't see what your first commit looked like.

jsonReplacer is fine, that's how we add those options so far. Ideally we should be using an object for these settings like this

@elyobo
Copy link
Contributor Author

elyobo commented Mar 26, 2016

Apologies on the squashing, I can probably reconstruct it out of the reflog if you'd like to see it how it was. The only change in the last one was to correct the JSON.string() to JSON.stringify() in the readme. Before that the change was to update the docs in the readme, which I'd missed before that. Before that was a change to switch to jsonStringifier along the lines of mikeal's suggestion which I pushed accidentally, then reverted and squashed. No functional changes have been made at any time.

@simov simov merged commit a7ef602 into request:master Mar 26, 2016
@simov
Copy link
Member

simov commented Mar 26, 2016

Ok 👍 do you want this change published soon? It's probably a good time for new release anyway.

@elyobo
Copy link
Contributor Author

elyobo commented Mar 26, 2016

Thanks for the merge, whenever suits you to release is fine by me.

On Sat, 26 Mar 2016 18:50 simo notifications@github.com wrote:

Ok [image: 👍] do you want this change published soon? It's probably a
good time for new release anyway.


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#2009 (comment)

@elyobo elyobo deleted the stringify-replacer branch March 5, 2017 21:48
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

3 participants