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

Always use the original json primitive type when serializing values #1477

Closed
wants to merge 5 commits into from

Conversation

mlaily
Copy link
Contributor

@mlaily mlaily commented Mar 5, 2023

Before this PR, serializing a value inferred by the json type provider would always write it using its inferred type, regardless of its original primitive json type in the json sample.

Example:

type PT = JsonProvider<""" { "a": "42" } """> // int value in a string primitive type
PT.Root(42).JsonValue.ToString() // correctly inferred as an int

// incorrectly serialized to a json number
val it: string = "{
  "a": 42
}"

After this PR:

val it: string = "{
  "a": "42"
}"

This partially fixes #1271


Note: This PR is currently on top of the fix-json-dictionaries branch from #1476, itself on top of the rearrange-project-files branch from #1475 — I will rebase it on master when the other ones are merged.

EDIT: rebased and ready!

@mlaily mlaily force-pushed the roundtrip-json-primitive-types branch from 864a017 to 2abe008 Compare March 5, 2023 22:15
@cartermp
Copy link
Collaborator

cartermp commented Mar 7, 2023

I need to take a closer look but so far this seems great

mlaily added 5 commits March 7, 2023 23:16
Both functions are identical, and inferPrimitiveType makes more sense,
being symmetric with inferCollectionType...

getInferedTypeFromString should be removed at some point.
@mlaily mlaily force-pushed the roundtrip-json-primitive-types branch from 2abe008 to beaeeb2 Compare March 7, 2023 22:21
@mlaily
Copy link
Contributor Author

mlaily commented Mar 7, 2023

Thanks for taking a first look!

One thing to note: I struggled a little with the code quotations, and didn't manage to pass a DU case value directly to the runtime code, so I decided to pass it as an int...

I'm still unsure whether it's possible or not, but I started having doubts after I realized that the existing implementation of the InferedTypeTag does a similar thing, though it serializes its values as strings.

Feel free to ask questions if something is unclear.

@mlaily
Copy link
Contributor Author

mlaily commented Apr 4, 2023

Hello,

Since I started working on a JsonProvider2 (#1478) and it seems to be going somewhere, would you prefer it if I closed this PR and only did the changes it contains in the new provider, keeping the original JsonProvider's behaviour untouched?

I think the changes are still valuable, but they are not enough on their own to make the original JsonProvider trustworthy.
The missing part is the serialization of null values (as null or as missing properties), but I'll only be doing that in JsonProvider2 because it's going to change the generated code somewhat more...

Let me know what you think.

@cartermp
Copy link
Collaborator

cartermp commented Apr 8, 2023

Hmm, yeah, let's close this. AFter second thought, this is a breaking change (even if it's the right one), and I'd rather be a little more whole-hog in doing a breaking change than just have a minor behavior difference.

@cartermp cartermp closed this Apr 8, 2023
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.

JSON constructors create values that don't correspond to the original schema
2 participants