search

JuliaGeo / GeoJSON.jl

Utilities for working with GeoJSON data in Julia
https://juliageo.org/GeoJSON.jl/stable/
MIT License
68 stars 10 forks source link

fix write multilinestring #68

Closed ErickChacon closed 1 year ago

ErickChacon commented 1 year ago

Adress #67 and JuliaEarth/GeoTables.jl#4. I have no experienve with GeoJSON.jl but it fixes the issue of saving MultiLineString as Polygon.

ErickChacon commented 1 year ago

Thanks. Would be good to have a test for this too

Great! I will add a test.

ErickChacon commented 1 year ago

@rafaqz. I added a test that checks writing when using _lower which will be the case when other packages uses GeoJSON.jl to write to a file.

rafaqz commented 1 year ago

I don't totally understand why we would test _lower and not just write ?

ErickChacon commented 1 year ago

I don't totally understand why we would test _lower and not just write ?

Because once we read the objects defined in the test, they are a subtype of GeoJSONT, so the function _lower does not get called and it is a reason why the error was not catched before.

The use case of _lower will happen when trying to save geometries defined from another package (e.g. Geotables.jl, Shapefile.jl, ...). So, in this case I am trying to exemplify what would happen when writing using geomtries from other packages.

That is what I understand from the lines:

https://github.com/JuliaGeo/GeoJSON.jl/blob/b9825e924eaed4856bcaa7cdc55138e185a7a477/src/io.jl#L45-L50

rafaqz commented 1 year ago

Ok makes sense. Could you instead do GeoInterface.convert(GeoInterface, geom) to use regular write on a different type? Wouldnt that catch it too?

juliohm commented 1 year ago

@rafaqz I think the PR is ready for a second review.

ErickChacon commented 1 year ago

I modified the tests to use GeoInterface.convert. This method can be used only for non-null geometries, so I defined featuresgeom for features with non-null geometries.

juliohm commented 1 year ago

@rafaqz I am merging this as a hot-fix since you gave me permission to take more action in packages of the org. This is blocking some advances downstream. @ErickChacon has updated the tests following your suggestions.

rafaqz commented 1 year ago

Thanks, looks good.

But do note that expecting one day turn around response for JuliaGeo reviews might be a little ambitious ;)

(Also note that I cant give you total permission to do anything in JuliaGeo, this is a collective social process with a number of people involved, and its good to keep that in mind, along with the fact we all have lives and full time jobs. Patience is often required)