final push

[visr]

Before we can put this code on top of GeoJSON.jl, ref JuliaGeo/GeoJSON.jl#36.

[rafaqz]

Are there any blocking issues here? I would love to get this merged into GeoJSON.jl tonight.

+1 for using Aqua we should really add it everywhere.

[visr]

Yeah I have some commits pending that fix several issues, like the geometry being excluded from the tables interface, and non JSON3 but NamedTuple backed objects not working well.

[visr]

Sorry for the large commit, but after 69aec66 I'm happy with this for now. The remaining points I have are non breaking, possibly except for one question that you can probably answer. In the GeoInterface docs for extent, I don't see whether it is expected to try to compute the extent, if it is not stored already. I guess not, since that is what is implemented here now, and the fallback in GeoInterface also returns nothing. Should there then be a separate function in GeoInterface that (re-)computes the extent? Or even a function that only computes it if it is not stored?

Anyway I might as well dump my remaining points below here. I can create issues later.

Create issues

• JSON3 has jsonlines support; support GeoJSONSeq
• Support pretty output via JSON3.pretty
• Look into write performace, 3 minutes for basinatlas-6-rfc.geojson
  • now it is written on GeoInterface, but perhaps we can dump GeoJSONObject directly. This will also preserve any extra fields that are allowed in the JSON spec, but lost over the GeoInterface.
• Add support for out-of-spec CRS parsing
• GI.isclosed TODOs
• numbertype=Float64 possibly in the future of JSON3, see Generated types not working for floats quinnj/JSON3.jl#198 (comment)
• GI.extent returns nothing if no bbox is present, what if we want to calculate it?
• Add convert methods to create GeoJSON geometries from GeoInterface

Document

• Document how to upgrade from the old GeoJSON
• That we implement the standard
• Document that object can be something like a NamedTuple

[rafaqz]

Ok that looks like a bit more thought might be needed merging this into GeoJSON.jl

I've also been wondering about computing the extent. It's really cheap precalculated but a little expensive to calculate (especially for large datasets) so knowing if it's precalculated can be useful.

[visr]

Ok that looks like a bit more thought might be needed merging this into GeoJSON.jl

Thought about what? I think the list of issues I posted are not blocking to me. One IMO important thing is to write a short bit on how to port code from the old GeoJSON to the new. I won't have that much time to push on this on the short term however.

computing the extent. It's really cheap precalculated but a little expensive to calculate

I we decide (and document) that GeoInterface.extent returns nothing unless the extent has been precalculated, this would not change anything here. But I haven't checked if SF says anything about this, and not sure what is common. On first glance it looks like for instance GEOS just computes it. But I've also worked with point clouds, where you really want to have it in the metadata.

[rafaqz]

Looks good, cant see much that needs fixing If you think theyre not blocking we can merge.

I think we should always be able to return nothing for the extent, for the point cloud example. We could possible add a compute_extent method to geointerface that calls extent and otherwise does the computation over all points.

But I don't think thats required immediately. Rasters.jl is just doing something like that internally for now.