Replace homegrown converter with osm2geojson

[dericke]

Replaces the built-in method to convert Overpass JSON to geojson with the osm2geojson library. The motivation is to reduce the scope of this library and offload some maintenance burden.

Important differences:

• osm2geojson uses 7-digit coordinate precision, same as the Overpass JSON. This lib has been producing 6-digit geojson up until now
• osm2geojson nests the id attribute under properties, this lib has been putting id as a top-level attribute

Setting as a draft PR until:

• it is decided either that these differences are acceptable, or either osm2geojson or this PR is modified to produce identical output
• it is confirmed that this will not regress recent PRs such as Include nodes, user, uid, timestamp, and version in GeoJSON properties #130

Closes #135 and possibly some others?

[mvexel]

Hey cool! Sorry it took me so long to respond.
Since this would be a breaking change, with the id moving to properties, this would mean a 1.0 release. I would like to shore up tests before we do that, but in general I like not relying on bespoke code when a well-established library is available.

What work do you think needs to be done to the unit tests? Last I checked test coverage is still pretty poor...

[dericke]

Testing is not my greatest strength, but I think #141 would be a good start in improving the tests. I'll try to do some more comprehensive analysis to see what can be improved beyond that.

[mvexel]

@dericke what is left to do before we can merge this? Should there be a 1.x branch since this would be a breaking API change? @russbiggs can you comment on whether this will break #130 ?

[dericke]

I was just waiting until the PR related to tests was merged, which I see you've done (thanks!), and for feedback on the API change. I don't feel confident on judging whether the API change is significant enough to warrant a major version increment or not.

[russbiggs]

@mvexel it looks like this would remove all the changes I made in #130 since it removes the whole _as_geojson method. I'm not sure if osm2geojson would do the same as what my PR does.

[dericke]

It does appear that this PR currently regresses #130 . I'll look into doing a PR against osm2geojson to fix this.

[dericke]

The PR for osm2geojson was accepted, so when using version >= 0.1.30 of that library, #130 is not regressed.

[mvexel]

Hey, see my comment here: #135 (comment)
Do you agree it makes sense to adopt __geo_interface__? /cc @sgillies

[dericke]

I like the idea of supporting __geo_interface__, but wouldn't that still require a converter—provided either by this lib or an external dependency—to go from overpass json output to __geo_interface__-compatible output?

[mvexel]

I like the idea of supporting __geo_interface__, but wouldn't that still require a converter—provided either by this lib or an external dependency—to go from overpass json output to __geo_interface__-compatible output?

Yes, it would. But then any library that adopts __geo_interface__ would be able to handle the output.

[t-vi]

Just a quick shout: It seems that the example for MapQuery hits a very similar "polygons only as references" problem as #122 (which #129 apparently partially fixed).
So it would seem that there would be more work to do on the geojson conversion unless it is dropped in favour of an external solution (but I don't know what level of activity to expect for that, either).

[JanBeelte]

I like the idea of supporting __geo_interface__, but wouldn't that still require a converter—provided either by this lib or an external dependency—to go from overpass json output to __geo_interface__-compatible output?

Yes, it would. But then any library that adopts __geo_interface__ would be able to handle the output.

I agree this would probably be the best option, but given nothing is happening on that end wouldnt it be better to merge this one than the status quo?

[tumluliu]

This PR's code can resolve the "corrupted multipolygon" issue from my end. I hope it can be merged soon if no other blockers. Thanks a lot for the nice job @dericke !

[mvexel]

I don't see any other blockers here. I think we're good to finally (sorry..) merge this /cc @metazool

[mvexel]

okay I royally messed up the pr review process and committed everything to main instead. I'm going to close this now for that reason. Everything should be there but I'll double check