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

Fix incorrect JsonProvider numeric inference #1222

Conversation

panesofglass
Copy link
Contributor

@panesofglass panesofglass commented Oct 23, 2018

See #1221

Hypothesis is that commit 8d05e4b is not quite correct.

@panesofglass
Copy link
Contributor Author

Apologies for the formatting changes. I didn't do anything to change the formatting. Should I try to revert that?


[<Test>]
let ``Getting field with numeric values should infer decimal``() =
let json = JsonProvider<""" { "a":[1.0, 1.0] } """>.GetSample()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this is supposed to infer decimal, but I could be wrong.

@panesofglass
Copy link
Contributor Author

panesofglass commented Oct 23, 2018

Appears these new lines are the issue: https://github.com/fsharp/FSharp.Data/blob/8d05e4bf32fe750cb0f1bad977cbd5b97b39ab17/src/Json/JsonInference.fs#L34-L35

What's wanted here? I would expect the format of the number to override the use of Math.Round(v) = v as a means of inference. If not, what is required to make this work correctly?

@panesofglass
Copy link
Contributor Author

@ovatsus @baronfel I don't know the right way to fix this. Help is appreciated.

@baronfel
Copy link
Contributor

Personally I think the single flag for inferTypesFromValues is a bit wide for coercing both strings and numeric types. I'd be in favor of expanding the number of static/configuration parameters for this (maybe just a split for string-inference and number-inference) as well as expanding the number of JsonValue cases to include Integer and LongInteger or something semantically similar, since the only time where we have enough information is at JsonValue.Parse time. Armed with JsonValue.Integer and JsonValue.LongInteger we could easily handle type mapping to int/int64 for json provider even without inferTypesFromValues set.

@dsyme
Copy link
Contributor

dsyme commented Mar 21, 2019

Close/open to re-run CI

@dsyme dsyme closed this Mar 21, 2019
@dsyme dsyme reopened this Mar 21, 2019
@dsyme
Copy link
Contributor

dsyme commented Apr 14, 2021

Closing this out as the repo has diverged a long way since the proposal was made

@dsyme dsyme closed this Apr 14, 2021
@panesofglass
Copy link
Contributor Author

panesofglass commented Apr 14, 2021

We ultimately never upgraded and are stuck on FSharp.Data 3.3.3. Has this been fixed in a different PR by any chance?

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.

4 participants