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

change optionFromJson to enable it to handle undefined values #80

Merged
merged 3 commits into from
Jun 17, 2022

Conversation

adnelson
Copy link
Contributor

@adnelson adnelson commented Feb 6, 2022

Changes decco's optionFromJson so that it treats both null and undefined as None. This makes it much easier to deal with objects coming in from JS code which might use undefined or have the field missing entirely.

@adnelson
Copy link
Contributor Author

I'm not sure why the CI failed, it looks like some of the jobs weren't run. If there's anything further you need from me on this PR please let me know.

@ryb73
Copy link
Member

ryb73 commented Feb 15, 2022

Hi @adnelson. Thanks for the PR.

Not sure why CI failed, I'll have to take a look at that.

This change seems reasonable, I'm curious though if the [@decco.default] annotation would meet your use case?

@adnelson
Copy link
Contributor Author

adnelson commented Feb 23, 2022

@ryb73 thanks for the response, I tried it but unfortunately I get an error if the field is set to undefined. It finds the key set on the object, then tries to parse it as null, which fails, since null !== undefined

@adnelson
Copy link
Contributor Author

Just wondering is this ok to merge? Does the reasoning make sense?

@adnelson
Copy link
Contributor Author

adnelson commented Apr 7, 2022

@ryb73 ryb73 merged commit 2051c37 into rescript-labs:master Jun 17, 2022
ryb73 added a commit that referenced this pull request Jun 17, 2022
@ryb73
Copy link
Member

ryb73 commented Jun 17, 2022

Hey @adnelson, sorry for the delay. I merged the PR but am unable to publish. You can see #82 for more info.

@ryb73
Copy link
Member

ryb73 commented Jul 14, 2022

This change has been released in v1.6.0.

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.

2 participants