mocnik-science / osm-python-tools

A library to access OpenStreetMap related services
GNU General Public License v3.0
441 stars 48 forks source link

Update overpassQueryBuilder's area parameter to handle way or relation ids #34

Closed tyhranac closed 3 years ago

tyhranac commented 3 years ago

This would make querying within features with known ids/without names simpler by including logic to calculate the feature's corresponding area id.

Example usage:

mocnik-science commented 3 years ago

Dear tyhranac, thank you very much for your request. I see your intention to make the overpassQueryBuilder handle more types of queries. I agree to making adaptions with this part of the code, but there are yet some details to figure out. Most important, your code would (if I understand the situation correctly) break current applications, which is not a good idea. If the area ID is requested like in Example 3 of the readme, the number provided is already incremented by 2400000000 or 3600000000, if I am correct. In this case, the number should be accepted as is. Only if a way or relation prefix is provided, I would start adapting the area ID in the way you have proposed. Does this make sense to you?

PS: Sorry for my late reply, and thank you for your engagement!

tyhranac commented 3 years ago

Thank you for the in-depth reply, @mocnik-science! Adding 2400000000 or 3600000000 to already incremented area id's was also a concern of mine. To avoid this, I used if isinstance(area, str):, which should return False for integers returned by the areaId() method used in Example 3 of the readme and thus not additionally increment the area value. I believe the code in this pull request will only add 2400000000 or 3600000000 if the area value is of type str and the string is a properly formated way or relation id. Thanks again! I really appreciate your feedback on my first pull request 😃

mocnik-science commented 3 years ago

Thank you for your contribution! I almost guested that this would be your answer. How does the iD Editor format work? For a way, it seems to be w000000000. How are areas represented? Are they in the format a000000000 or something alike? In that case, one could distinguish between area[0] == 'w', area[0] == 'a', and all other cases. In the latter, one could handle them like before and thus not increment by 3600000000. I would like to hear your thoughts on this.

(I hope for your understanding that adaptions need to be executed thoroughly, and I highly appreciate collaborating with you on this issue.)

tyhranac commented 3 years ago

Thanks for getting back to me! I may be confused by area IDs.

Based on the Overpass API documentation, areas aren't native OSM elements like nodes, ways, or relations, so they don't have OSM IDs like nodes, ways, and relations do. Instead, they have "virtual" ID's generated by the Overpass server. Because of this, there is no iD Editor format for areas to filter for.

Thank you for working with me and responding to all my comments! Also, if this is not a good time to work on an OSM ID -> Overpass API area ID feature for the overpassQueryBuilder function, I'm happy to close this pull request.

mocnik-science commented 3 years ago

Dear @tyhranac, Thanks again for your involvement! Still, there are some questions about the code. I will incorporate it and modify accordingly. Also, I will document it. Thanks again for your contribution!

mocnik-science commented 3 years ago

I have now even added corresponding tests. The code will be made available as part of Version 0.3.0 in some hours.

tyhranac commented 3 years ago

@mocnik-science Thank you very much for reviewing my code! I appreciate all your help.

I'm also happy to write documentation in the README if that is helpful.

mocnik-science commented 3 years ago

@tyhranac, thanks so much for your appreciated help and feedback. Actually, one of the reasons why my reactions here are slow sometimes is that I want to deliver an easy-to-use, stable, well-documented library. This is why I review merge requests thoroughly and why I do not accept any new feature without having documented and tested it. I am even aware of some more code cleanup I could do, but not introducing too many changes seems to be important to not introduce new errors.

You find the documentation of ‘your’ new feature in the file overpass.md (just search for way/ in this document). If you have further suggestions regarding the documentation, please let me know. I am aware of the fact that this feature could be documented in a more detailed way, but it seems important to also be short and precise. In the ideal case, the example should read as a nice narrative and yet document all features well in a concise and logical way.

tyhranac commented 3 years ago

@mocnik-science Absolutely--I understand and appreciate your work on OSMPythonTools. I think the documentation you wrote is great and agree it's best not to clutter the README with examples for minor features like this one 👍 Thanks again for reviewing my code 👨‍💻