parse all numbers as Float64 when reading JSON #54
Conversation
This will avoid problems with coordinates like 1 or 1.0 being converted to Int. This also affects the properties, so integer columns will now be Float64, and will need to be manually converted to integer types.
| @@ -17,7 +17,7 @@ Extents = "0.1" | |||
| GeoFormatTypes = "0.4" | |||
| GeoInterface = "1" | |||
| GeoInterfaceRecipes = "1" | |||
| JSON3 = "1" | |||
| JSON3 = "1.12" | |||
There was a problem hiding this comment.
Should this be 1.1? Note that real breaks need v2. But, I'm not sure if this is really breaking, it seems more like fixing a bug to me.
(And I generally find it better not to bump versions in PRs)
There was a problem hiding this comment.
This is the JSON3 version, not GeoJSON. I didn't bump the GeoJSON version yet.
There was a problem hiding this comment.
About breaking/not breaking, I agree for coordinates it's definitely bugfix. For integer properties it could be a minor change, I'm also fine with releasing as non-breaking.
There was a problem hiding this comment.
Lol I never read toml files correctly in PRs. But yes, bugfix it is.
| @@ -5,7 +5,7 @@ | |||
| Read a GeoJSON string to a GeoInterface.jl compatible feature or geometry object. | |||
| """ | |||
| function read(source) | |||
| object = JSON3.read(source) | |||
| object = JSON3.read(source; numbertype = Float64) | |||
There was a problem hiding this comment.
Should we allow passing the numbertype keyword through read?
There was a problem hiding this comment.
I think not using this setting is a real footgun, not sure we should offer that option. I doubt it's ever useful in practice.
There was a problem hiding this comment.
Yeah, makes sense. I just saw the tests had used Int previously and wondered if we should facilitate numvertype=Int in case anyone else was using Int. But I guess that's not really a real scenario.
This will avoid problems with coordinates like 1 or 1.0 being converted to Int. This also affects the properties, so integer columns will now be Float64, and will need to be manually converted to integer types.
This is now possible in the latest JSON3 release with PR quinnj/JSON3.jl#240.
Fixes #52, also cc @jeremiahpslewis who ran into issues with this for GeoMakie.
Due to the effect on properties this should probably be released as a breaking change.