search

mvexel / overpass-api-python-wrapper

Python bindings for the OpenStreetMap Overpass API
Apache License 2.0
359 stars 89 forks source link

Replace homegrown converter with osm2geojson #140

Closed dericke closed 4 months ago

dericke commented 3 years ago

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:

Setting as a draft PR until:

Closes #135 and possibly some others?

mvexel commented 3 years ago

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 commented 3 years ago

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 commented 2 years ago

@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 commented 2 years ago

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 commented 2 years ago

@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 commented 2 years ago

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

dericke commented 2 years ago

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

mvexel commented 2 years ago

Hey, see my comment here: https://github.com/mvexel/overpass-api-python-wrapper/issues/135#issuecomment-1075451525 Do you agree it makes sense to adopt __geo_interface__? /cc @sgillies

dericke commented 2 years ago

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 commented 2 years ago

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 commented 1 year ago

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 commented 8 months ago

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 commented 8 months ago

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 commented 6 months ago

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

mvexel commented 4 months ago

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