Skip to content
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

Jump to bottom

Relation and multipolygon support #115

Merged
merged 8 commits into from
Dec 10, 2019
Merged

Relation and multipolygon support #115

merged 8 commits into from
Dec 10, 2019

Conversation

lukasmu
Copy link
Contributor

@lukasmu lukasmu commented Sep 10, 2019

This pull request can be considered a follow-up to #76.

I initially started to resolve the merge conflict referenced in the pull request mentioned above. But then I decided to rewrite and optimize the geojson parsing method completely.

Finally I added a test based on real data to make sure that the parsing works correctly.Please note that the example.json file in the tests folder can be opened with any geojson viewer.

@lukasmu lukasmu force-pushed the relation_and_multipolygon_support branch from f2ccb10 to f6ba57a Compare September 10, 2019 22:07
ThomDietrich
ThomDietrich reviewed Sep 11, 2019
View reviewed changes
Copy link
Contributor

@ThomDietrich ThomDietrich left a comment

Choose a reason for hiding this comment

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

Hey @lukasmu,
great work! Looks really good. I added a few minor comments for discussion. Best!

overpass/api.py Outdated Show resolved Hide resolved
overpass/api.py Outdated Show resolved Hide resolved
overpass/api.py Show resolved Hide resolved
overpass/api.py Show resolved Hide resolved
overpass/api.py Outdated Show resolved Hide resolved
@lukasmu
Copy link
Contributor Author

lukasmu commented Sep 13, 2019

A note on the Travis checks:
For Python 2.7 the error occurs due to differences in pickle functionalities that are solely used in the tests. I don't think this is worth fixing anymore (see https://www.python.org/doc/sunset-python-2)
For Python 3.3 there seems to be something wrong with the Travis config

@ThomDietrich
Copy link
Contributor

ThomDietrich commented Sep 16, 2019 •
edited
Loading

Thanks for the latest commit. All my comments are addressed 👍💯

@ThomDietrich
Copy link
Contributor

@mvexel @tbolender @t-g-williams What do you think? Anything holding back a merge soon?
Best!

tbolender
tbolender requested changes Oct 17, 2019
View reviewed changes
Copy link
Collaborator

@tbolender tbolender left a comment

Choose a reason for hiding this comment

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

Everything looks okay, I just added a small clarification comment.

@mvexel We should remove support for 2.7 since it's going to retire anyway soon..

tests/test_api.py Show resolved Hide resolved
@lukasmu
Copy link
Contributor Author

lukasmu commented Nov 11, 2019

@tbolender @mvexel Is there still anything that I can improve on this pull request? Or did you just not yet have the time to look into it?

@tbolender
Copy link
Collaborator

From my side everything is okay. I waited since I wanted @mvexel to have a look at it.

@mvexel
Copy link
Owner

mvexel commented Dec 10, 2019

Sorry guys I've been unavailable.. I'll have a look.

@mvexel mvexel merged commit 0db1a7e into mvexel:master Dec 10, 2019
mvexel added a commit that referenced this pull request Dec 10, 2019
@mvexel
Merge pull request #115 from lukasmu/relation_and_multipolygon_support
7988b3f 
Relation and multipolygon support
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ThomDietrich ThomDietrich ThomDietrich left review comments

@tbolender tbolender tbolender approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@lukasmu @ThomDietrich @tbolender @mvexel @t-g-williams