Skip to content
This repository was archived by the owner on Dec 17, 2025. It is now read-only.

Add Js.JSON.parseExnWithReviver and parseToAnyExnWithReviver#14

Open
MoOx wants to merge 4 commits into
bloodyowl:mainfrom
MoOx:patch-2
Open

Add Js.JSON.parseExnWithReviver and parseToAnyExnWithReviver#14
MoOx wants to merge 4 commits into
bloodyowl:mainfrom
MoOx:patch-2

Conversation

@MoOx

@MoOx MoOx commented May 28, 2022

Copy link
Copy Markdown
Contributor

@bloodyowl

Copy link
Copy Markdown
Owner

Could you add a asJsonReviver identity external for convenience?

(and possibly add the same logic with replacer for stringify?)

@MoOx

MoOx commented May 28, 2022

Copy link
Copy Markdown
Contributor Author

@bloodyowl is this what you had in mind?

Comment thread src/Js__JSON.res Outdated
@bloodyowl

Copy link
Copy Markdown
Owner

yup! just added a small comment on a typo

@MoOx

MoOx commented May 28, 2022

Copy link
Copy Markdown
Contributor Author

Fixed !

Comment thread src/Js__JSON.res Outdated
Comment thread src/Js__JSON.res
Comment on lines 10 to +13
@val external stringify: t => string = "JSON.stringify"
@val external stringifyWithIndent: (t, @as(json`null`) _, int) => string = "JSON.stringify"
@val external stringifyWithReplacer: (t, jsonReplacer) => string = "JSON.stringify"
@val external stringifyWithReplacerAndIndent: (t, jsonReplacer, int) => string = "JSON.stringify"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Maybe it's better to have one method instead of 4:

@val external stringify: (t, ~jsonReplacer: jsonReplacer=?, ~indent: int=?, ()) => string = "JSON.stringify"

@MoOx @bloodyowl what do you think?

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I'd like to avoid having an extra unit argument at the end for the general use-case, it'd make the API a feel bit weird for beginners.

@DZakh DZakh May 28, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Makes sense, also it will generate bigger js output, passing three additional undefineds when we call it with only one argument

@MoOx

MoOx commented May 28, 2022

Copy link
Copy Markdown
Contributor Author

I guess should be good now.

zth added a commit to zth/rescript-js that referenced this pull request Oct 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants