-
Notifications
You must be signed in to change notification settings - Fork 644
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.
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
add extratags parameter for nominatim #320
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I left some notes below, could you please address them?
Ignore the failed Travis build for py3.7 – that looks like a Travis fault; it's definitely not caused by your code.
geopy/geocoders/osm.py
Outdated
@@ -166,7 +166,8 @@ def geocode( | |||
limit=None, | |||
addressdetails=False, | |||
language=False, | |||
geometry=None | |||
geometry=None, | |||
extratags=True, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please keep the old behavior (i.e. set the default value to False
)? The extra info is rarely needed, so defaulting that to True
means some pointlessly wasted cpu and network resources.
@@ -216,6 +217,9 @@ def geocode( | |||
|
|||
.. versionadded:: 1.3.0 | |||
|
|||
:param bool extratags: Include additional information in the result if available, | |||
e.g. wikipedia link, opening hours. | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please add the following line:
.. versionadded:: 1.17.0
test/geocoders/nominatim.py
Outdated
@@ -258,6 +258,26 @@ def test_bounded(self): | |||
{"latitude": 56.4803224, "longitude": 85.0060457653324}, | |||
) | |||
|
|||
def test_extratags(self): | |||
""" | |||
Nominatim.geocode using `extratags` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This docstring is not needed as it restates the test method name.
test/geocoders/nominatim.py
Outdated
""" | ||
Nominatim.geocode using `extratags` | ||
""" | ||
geocoder = Nominatim(user_agent='my_user_agent/1.0') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tests above use self.make_geocoder()
for creating an instance of geocoder. This is important, because this test class is an abstract class which tests not just Nominatim, but also other Nominatim-based geocoders (e.g. Pickpoint and OpenMapQuest), which instances are returned by the overridden make_geocoder
methods in the specific test cases.
_make_request
usage is also discouraged. The same could be achieved with the following (which also doesn't require creating a new geocoder instance):
query = "175 5th Avenue NYC"
location = self.geocode_run(
{"query": query},
{},
)
self.assertIsNone(result.raw.get('extratags'))
location = self.geocode_run(
{"query": query, "extratags": True},
{},
)
self.assertEqual(result.raw['extratags']['wikidata'], 'Q220728')
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perfect, thank you!
this commit fixes #157