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

feat(serde-json-impl): handle bool for serde_json::Value #358

Conversation

GuillaumeDecMeetsMore
Copy link
Contributor

Goal

  • Adds boolean to the TS type for serde_json::Value

Notes

  • As far as I know, boolean is a correct type in JSONs, and is one of the possible variants of serde_json::Value, so I think it makes sense to add it to the generated TS type

Changes

  • Added new variant to the enum TsJsonValue => Boolean(bool)
  • Added | boolean in the unit tests for serde-json-impl feature

Checklist

  • I have followed the steps listed in the Contributing guide.
  • If necessary, I have added documentation related to the changes made.
  • I have added or updated the tests related to the changes made.

Copy link
Collaborator

@gustavo-shigueo gustavo-shigueo left a comment

Choose a reason for hiding this comment

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

Hi @GuillaumeDecMeetsMore, thanks for the PR! Nice catch, I'll just ask @NyxCode to take a look, but it all LGTM!

@NyxCode
Copy link
Collaborator

NyxCode commented Oct 7, 2024

Thanks!
What's even worse: null is missing as well.
Which idiot originally wrote this? Oh, wait.. 🙁

Actually no idea how this slipped through, though.

@NyxCode NyxCode merged commit 51a7a87 into Aleph-Alpha:main Oct 7, 2024
9 checks passed
@NyxCode NyxCode mentioned this pull request Oct 7, 2024
1 task
@GuillaumeDecMeetsMore
Copy link
Contributor Author

Thank you for the fast review and merge both of you 👍

I also completely missed that null was absent. I see you’re already handling it in #359. Thanks!

@gustavo-shigueo gustavo-shigueo added the bug Something isn't working label Nov 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants