Skip to content

parse all numbers as Float64 when reading JSON #54

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Nov 14, 2022
Merged

parse all numbers as Float64 when reading JSON #54

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Project.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ Extents = "0.1"
GeoFormatTypes = "0.4"
GeoInterface = "1"
GeoInterfaceRecipes = "1"
JSON3 = "1"
JSON3 = "1.12"
Copy link
Member

Choose a reason for hiding this comment

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

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)

Copy link
Member Author

Choose a reason for hiding this comment

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

This is the JSON3 version, not GeoJSON. I didn't bump the GeoJSON version yet.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

Lol I never read toml files correctly in PRs. But yes, bugfix it is.

Tables = "1"
julia = "1.6"

Expand Down
2 changes: 1 addition & 1 deletion src/json.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Member

Choose a reason for hiding this comment

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

Should we allow passing the numbertype keyword through read?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

if object === nothing
error("JSON string is empty")
end
Expand Down
4 changes: 3 additions & 1 deletion test/geojson_samples.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -487,6 +487,8 @@ geom_collection = """{
}]
}"""

geometries = [multi, bbox, bbox_z, bermuda_triangle, geom_collection]
point_int = """{"type":"Point","coordinates":[1,2]}"""

geometries = [multi, bbox, bbox_z, bermuda_triangle, geom_collection, point_int]

end # module test
13 changes: 12 additions & 1 deletion test/runtests.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -234,7 +234,7 @@ include("geojson_samples.jl")
@test t[1] isa GeoJSON.Feature
@test t.geometry isa Vector{Union{T,Missing}} where {T<:GeoJSON.Point}
@test ismissing(t.geometry[3])
@test t.a isa Vector{Union{Int,Missing}}
@test t.a isa Vector{Union{Float64,Missing}}
@test isequal(t.a, [1, missing, 3])
@test t.b isa Vector{Missing}
@test Tables.columntable(t) isa NamedTuple
Expand Down Expand Up @@ -280,6 +280,17 @@ include("geojson_samples.jl")
@test p isa GeoJSON.Point
end

@testset "numbertype" begin
# all numbers are Float64 since we use numbertype=Float64
p = GeoJSON.read(T.point_int)
@test p isa GeoJSON.Point
coords = GeoJSON.coordinates(p)
@test coords isa JSON3.Array
@test eltype(coords) == Float64
@test coords == [1, 2]
@test copy(coords) isa Vector{Float64}
end

Aqua.test_all(GeoJSON)

end # testset "GeoJSON"