Repository: geopy/geopy
Pull request #319 (2018-08-03 09:09:25)

Make OpenMapQuest extend Nominatim
OpenMapQuest API is basically an older version of Nominatim [[1]] [[2]], so the OpenMapQuest class should extend Nominatim as well. This PR does exactly that.

This consequently adds more parameters to OpenMapQuest + adds the reverse geocoding method.

Closes #174

[1]: https://developer.mapquest.com/documentation/open/nominatim-search/
[2]: https://developer.mapquest.com/forum/differences-between-openmaquestcom-and-nominatimopenstreetmaporg


Patch:
diff --git a/docs/index.rst b/docs/index.rst
index 4fccf40..471bcc8 100644
--- a/docs/index.rst
+++ b/docs/index.rst
@@ -179,6 +179,8 @@ OpenMapQuest
 
 .. autoclass:: geopy.geocoders.OpenMapQuest
    :members:
+   :inherited-members:
+   :show-inheritance:
 
    .. automethod:: __init__
 
diff --git a/geopy/geocoders/openmapquest.py b/geopy/geocoders/openmapquest.py
index 33ee9a6..de054cf 100644
--- a/geopy/geocoders/openmapquest.py
+++ b/geopy/geocoders/openmapquest.py
@@ -1,26 +1,34 @@
-from geopy.compat import urlencode
 from geopy.exc import ConfigurationError
-from geopy.geocoders.base import DEFAULT_SENTINEL, Geocoder
-from geopy.location import Location
-from geopy.util import logger
+from geopy.geocoders.base import DEFAULT_SENTINEL
+from geopy.geocoders.osm import Nominatim
 
 __all__ = ("OpenMapQuest", )
 
 
-class OpenMapQuest(Geocoder):
+class OpenMapQuest(Nominatim):
     """Geocoder using MapQuest Open Platform Web Services.
 
     Documentation at:
         https://developer.mapquest.com/documentation/open/
+
+    .. versionchanged:: 1.17.0
+       OpenMapQuest now extends the Nominatim class.
     """
 
+    geocode_path = '/nominatim/v1/search'
+    reverse_path = '/nominatim/v1/reverse'
+
     def __init__(
             self,
             api_key=None,
             format_string=None,
-            scheme=None,
+            view_box=None,
+            bounded=False,
+            country_bias=None,
             timeout=DEFAULT_SENTINEL,
             proxies=DEFAULT_SENTINEL,
+            domain='open.mapquestapi.com',
+            scheme=None,
             user_agent=None,
             ssl_context=DEFAULT_SENTINEL,
     ):
@@ -35,8 +43,22 @@ class OpenMapQuest(Geocoder):
         :param str format_string:
             See :attr:`geopy.geocoders.options.default_format_string`.
 
-        :param str scheme:
-            See :attr:`geopy.geocoders.options.default_scheme`.
+        :param tuple view_box: Coordinates to restrict search within.
+            Accepts instances of the :class:`geopy.point.Point`
+            ``[Point(22, 180), Point(-22, -180)]``,
+            or iterables of numeric and string types ``[180, 22, -180, -22]``,
+            ``["180", "22", "-180", "-22"]``
+
+            .. versionadded:: 1.17.0
+
+        :param bool bounded: Restrict the results to only items contained
+            within the bounding view_box.
+
+            .. versionadded:: 1.17.0
+
+        :param str country_bias: Bias results to this country.
+
+            .. versionadded:: 1.17.0
 
         :param int timeout:
             See :attr:`geopy.geocoders.options.default_timeout`.
@@ -44,6 +66,14 @@ class OpenMapQuest(Geocoder):
         :param dict proxies:
             See :attr:`geopy.geocoders.options.default_proxies`.
 
+        :param str domain: Domain where the target Nominatim service
+            is hosted.
+
+            .. versionadded:: 1.17.0
+
+        :param str scheme:
+            See :attr:`geopy.geocoders.options.default_scheme`.
+
         :param str user_agent:
             See :attr:`geopy.geocoders.options.default_user_agent`.
 
@@ -57,71 +87,30 @@ class OpenMapQuest(Geocoder):
         """
         super(OpenMapQuest, self).__init__(
             format_string=format_string,
-            scheme=scheme,
+            view_box=view_box,
+            bounded=bounded,
+            country_bias=country_bias,
             timeout=timeout,
             proxies=proxies,
+            domain=domain,
+            scheme=scheme,
             user_agent=user_agent,
             ssl_context=ssl_context,
         )
         if not api_key:
             raise ConfigurationError('OpenMapQuest requires an API key')
         self.api_key = api_key
-        self.api = ("%s://open.mapquestapi.com/nominatim/v1/search"
-                    "?format=json" % self.scheme)
 
-    def geocode(self, query, exactly_one=True, timeout=DEFAULT_SENTINEL):
+    def _construct_url(self, base_api, params):
         """
-        Return a location point by address.
+        Construct geocoding request url. Overridden.
 
-        :param str query: The address or query you wish to geocode.
+        :param string base_api: Geocoding function base address - self.api
+            or self.reverse_api.
 
-        :param bool exactly_one: Return one result or a list of results, if
-            available.
+        :param dict params: Geocoding params.
 
-        :param int timeout: Time, in seconds, to wait for the geocoding service
-            to respond before raising a :class:`geopy.exc.GeocoderTimedOut`
-            exception. Set this only if you wish to override, on this call
-            only, the value set during the geocoder's initialization.
-
-        :rtype: ``None``, :class:`geopy.location.Location` or a list of them, if
-            ``exactly_one=False``.
-        """
-        params = {
-            'key': self.api_key,
-            'q': self.format_string % query
-        }
-        if exactly_one:
-            params['maxResults'] = 1
-        url = "&".join((self.api, urlencode(params)))
-
-        logger.debug("%s.geocode: %s", self.__class__.__name__, url)
-        return self._parse_json(
-            self._call_geocoder(url, timeout=timeout),
-            exactly_one
-        )
-
-    @classmethod
-    def _parse_json(cls, resources, exactly_one=True):
-        """
-        Parse display name, latitude, and longitude from an JSON response.
+        :return: string URL.
         """
-        if not len(resources):
-            return None
-        if exactly_one:
-            return cls.parse_resource(resources[0])
-        else:
-            return [cls.parse_resource(resource) for resource in resources]
-
-    @classmethod
-    def parse_resource(cls, resource):
-        # TODO make this a private API
-        # Return location and coordinates tuple from dict.
-        location = resource['display_name']
-
-        latitude = resource['lat'] or None
-        longitude = resource['lon'] or None
-        if latitude and longitude:
-            latitude = float(latitude)
-            longitude = float(longitude)
-
-        return Location(location, (latitude, longitude), resource)
+        params['key'] = self.api_key
+        return super(OpenMapQuest, self)._construct_url(base_api, params)
diff --git a/geopy/geocoders/osm.py b/geopy/geocoders/osm.py
index dc5d63e..73e68d6 100644
--- a/geopy/geocoders/osm.py
+++ b/geopy/geocoders/osm.py
@@ -14,7 +14,7 @@ _DEFAULT_NOMINATIM_DOMAIN = 'nominatim.openstreetmap.org'
 
 
 class Nominatim(Geocoder):
-    """Nominatim geocoder for OpenStreetMap servers.
+    """Nominatim geocoder for OpenStreetMap data.
 
     Documentation at:
         https://wiki.openstreetmap.org/wiki/Nominatim
@@ -45,6 +45,9 @@ class Nominatim(Geocoder):
         'postalcode',
     }
 
+    geocode_path = '/search'
+    reverse_path = '/reverse'
+
     def __init__(
             self,
             format_string=None,
@@ -73,22 +76,21 @@ class Nominatim(Geocoder):
             .. versionchanged:: 1.15.0
                Previously only a list of stringified coordinates was supported.
 
-        :param str country_bias: Bias results to this country.
-
         :param bool bounded: Restrict the results to only items contained
             within the bounding view_box.
 
             .. versionadded:: 1.15.0
 
+        :param str country_bias: Bias results to this country.
+
         :param int timeout:
             See :attr:`geopy.geocoders.options.default_timeout`.
 
         :param dict proxies:
             See :attr:`geopy.geocoders.options.default_proxies`.
 
-        :param str domain: Should be the localized Openstreetmap domain to
-            connect to. The default is ``'nominatim.openstreetmap.org'``,
-            but you can change it to a domain of your own.
+        :param str domain: Domain where the target Nominatim service
+            is hosted.
 
             .. versionadded:: 1.8.2
 
@@ -138,8 +140,8 @@ class Nominatim(Geocoder):
                 UserWarning
             )
 
-        self.api = "%s://%s/search" % (self.scheme, self.domain)
-        self.reverse_api = "%s://%s/reverse" % (self.scheme, self.domain)
+        self.api = "%s://%s%s" % (self.scheme, self.domain, self.geocode_path)
+        self.reverse_api = "%s://%s%s" % (self.scheme, self.domain, self.reverse_path)
 
     def _construct_url(self, base_api, params):
         """
@@ -235,7 +237,7 @@ class Nominatim(Geocoder):
 
         if exactly_one:
             params['limit'] = 1
-        elif limit:
+        elif limit is not None:
             limit = int(limit)
             if limit < 1:
                 raise ValueError("Limit cannot be less than 1")
@@ -356,18 +358,16 @@ class Nominatim(Geocoder):
         latitude = place.get('lat', None)
         longitude = place.get('lon', None)
         placename = place.get('display_name', None)
-        if latitude and longitude:
+        if latitude is not None and longitude is not None:
             latitude = float(latitude)
             longitude = float(longitude)
         return Location(placename, (latitude, longitude), place)
 
     def _parse_json(self, places, exactly_one):
-        if places is None:
+        if not places:
             return None
         if not isinstance(places, list):
             places = [places]
-        if not len(places):
-            return None
         if exactly_one:
             return self.parse_code(places[0])
         else:
diff --git a/geopy/geocoders/pickpoint.py b/geopy/geocoders/pickpoint.py
index 5f9c217..197948b 100644
--- a/geopy/geocoders/pickpoint.py
+++ b/geopy/geocoders/pickpoint.py
@@ -14,6 +14,9 @@ class PickPoint(Nominatim):
 
     """
 
+    geocode_path = '/v1/forward'
+    reverse_path = '/v1/reverse'
+
     def __init__(
             self,
             api_key,
@@ -49,9 +52,8 @@ class PickPoint(Nominatim):
         :param dict proxies:
             See :attr:`geopy.geocoders.options.default_proxies`.
 
-        :param str domain: Should be the localized Openstreetmap domain to
-            connect to. The default is ``'api.pickpoint.io'``, but you
-            can change it to a domain of your own.
+        :param str domain: Domain where the target Nominatim service
+            is hosted.
 
         :param str scheme:
             See :attr:`geopy.geocoders.options.default_scheme`.
@@ -79,12 +81,10 @@ class PickPoint(Nominatim):
             ssl_context=ssl_context,
         )
         self.api_key = api_key
-        self.api = "%s://%s/v1/forward" % (self.scheme, self.domain)
-        self.reverse_api = "%s://%s/v1/reverse" % (self.scheme, self.domain)
 
     def _construct_url(self, base_api, params):
         """
-        Construct geocoding request url. Overriden.
+        Construct geocoding request url. Overridden.
 
         :param string base_api: Geocoding function base address - self.api
             or self.reverse_api.


Test patch:
diff --git a/test/geocoders/nominatim.py b/test/geocoders/nominatim.py
index 3f31814..98ccb4a 100644
--- a/test/geocoders/nominatim.py
+++ b/test/geocoders/nominatim.py
@@ -12,6 +12,7 @@ from test.geocoders.util import GeocoderTestBase
 
 class BaseNominatimTestCase(with_metaclass(ABCMeta, object)):
     # Common test cases for Nominatim-based geocoders.
+    # Assumes that Nominatim uses the OSM data.
 
     delta = 0.04
 
@@ -36,6 +37,26 @@ class BaseNominatimTestCase(with_metaclass(ABCMeta, object)):
             {"latitude": 39.916, "longitude": 116.390},
         )
 
+    def test_geocode_empty_result(self):
+        self.geocode_run(
+            {"query": "dsadjkasdjasd"},
+            {},
+            expect_failure=True,
+        )
+
+    def test_limit(self):
+        with self.assertRaises(ValueError):  # non-positive limit
+            self.geocode_run(
+                {"query": "does not matter", "limit": 0, "exactly_one": False},
+                {}
+            )
+
+        result = self.geocode_run(
+            {"query": "second street", "limit": 4, "exactly_one": False},
+            {}
+        )
+        self.assertEqual(4, len(result))
+
     @patch.object(geopy.geocoders.options, 'default_user_agent',
                   'mocked_user_agent/0.0.0')
     def test_user_agent_default(self):
@@ -63,6 +84,35 @@ class BaseNominatimTestCase(with_metaclass(ABCMeta, object)):
         )
         self.assertIn("New York", location.address)
 
+    def test_country_bias(self):
+        self.geocoder = self.make_geocoder(country_bias='RU')
+        self.geocode_run(
+            {"query": "moscow"},
+            {"latitude": 55.7507178, "longitude": 37.6176606,
+             "delta": 0.3},
+        )
+
+        self.geocoder = self.make_geocoder(country_bias='US')
+        location = self.geocode_run(
+            {"query": "moscow"},
+            # There are two possible results:
+            # Moscow Idaho: 46.7323875,-117.0001651
+            # Moscow Penn: 41.3367497,-75.5185191
+            {},
+        )
+        # We don't care which Moscow is returned, unless it's
+        # the Russian one. We can sort this out by asserting
+        # the longitudes. The Russian Moscow has positive longitudes.
+        self.assertLess(-119, location.longitude)
+        self.assertLess(location.longitude, -70)
+
+    def test_structured_query(self):
+        self.geocode_run(
+            {"query": {"country": "us", "city": "moscow",
+                       "state": "idaho"}},
+            {"latitude": 46.7323875, "longitude": -117.0001651},
+        )
+
     def test_city_district_with_dict_query(self):
         self.geocoder = self.make_geocoder(country_bias='DE')
         query = {'postalcode': 10117}
@@ -70,7 +120,14 @@ class BaseNominatimTestCase(with_metaclass(ABCMeta, object)):
             {"query": query, "addressdetails": True},
             {},
         )
-        self.assertEqual(result.raw['address']['city_district'], 'Mitte')
+        try:
+            # For some queries `city_district` might be missing in the response.
+            # For this specific query on OpenMapQuest the key is also missing.
+            city_district = result.raw['address']['city_district']
+        except KeyError:
+            # MapQuest
+            city_district = result.raw['address']['suburb']
+        self.assertEqual(city_district, 'Mitte')
 
     def test_geocode_language_parameter(self):
         query = "Mohrenstrasse Berlin"
@@ -144,9 +201,6 @@ class BaseNominatimTestCase(with_metaclass(ABCMeta, object)):
         )
 
     def test_geocode_geometry_geojson(self):
-        """
-        Nominatim.geocode with full geometry (response in geojson format)
-        """
         result_geocode = self.geocode_run(
             {"query": "Halensee,Berlin", "geometry": 'geojson'},
             {},
diff --git a/test/geocoders/openmapquest.py b/test/geocoders/openmapquest.py
index 65dafeb..cfe0c9d 100644
--- a/test/geocoders/openmapquest.py
+++ b/test/geocoders/openmapquest.py
@@ -1,6 +1,6 @@
-
-from geopy.compat import u
 from geopy.geocoders import OpenMapQuest
+from geopy.exc import ConfigurationError
+from test.geocoders.nominatim import BaseNominatimTestCase
 from test.geocoders.util import GeocoderTestBase, env
 import unittest
 
@@ -14,37 +14,18 @@ class OpenMapQuestNoNetTestCase(GeocoderTestBase):
         )
         self.assertEqual(geocoder.headers['User-Agent'], 'my_user_agent/1.0')
 
+    def test_raises_without_apikey(self):
+        with self.assertRaises(ConfigurationError):
+            OpenMapQuest()
+
 
 @unittest.skipUnless(
     bool(env.get('OPENMAPQUEST_APIKEY')),
     "No OPENMAPQUEST_APIKEY env variable set"
 )
-class OpenMapQuestTestCase(GeocoderTestBase):
+class OpenMapQuestTestCase(BaseNominatimTestCase, GeocoderTestBase):
 
     @classmethod
-    def setUpClass(cls):
-        # setUpClass is still called even if test is skipped.
-        # OpenMapQuest raises ConfigurationError when api_key is empty,
-        # so don't try to create the instance when api_key is empty.
-        if env.get('OPENMAPQUEST_APIKEY'):
-            cls.geocoder = OpenMapQuest(scheme='http', timeout=3,
-                                        api_key=env['OPENMAPQUEST_APIKEY'])
-        cls.delta = 0.04
-
-    def test_geocode(self):
-        """
-        OpenMapQuest.geocode
-        """
-        self.geocode_run(
-            {"query": "435 north michigan ave, chicago il 60611 usa"},
-            {"latitude": 41.890, "longitude": -87.624},
-        )
-
-    def test_unicode_name(self):
-        """
-        OpenMapQuest.geocode unicode
-        """
-        self.geocode_run(
-            {"query": u("\u6545\u5bab")},
-            {"latitude": 39.916, "longitude": 116.390},
-        )
+    def make_geocoder(cls, **kwargs):
+        return OpenMapQuest(api_key=env['OPENMAPQUEST_APIKEY'],
+                            timeout=3, **kwargs)
diff --git a/test/geocoders/util.py b/test/geocoders/util.py
index bd789c2..fe8a77b 100644
--- a/test/geocoders/util.py
+++ b/test/geocoders/util.py
@@ -125,26 +125,40 @@ class GeocoderTestBase(unittest.TestCase):
     def _verify_request(
             self,
             result,
-            raw=EMPTY,
             latitude=EMPTY,
             longitude=EMPTY,
             address=EMPTY,
             exactly_one=True,
+            delta=None,
     ):
         """
-        Verifies that a a result matches the kwargs given.
+        Verifies that result matches the kwargs given.
         """
         item = result if exactly_one else result[0]
+        delta = delta or self.delta
+        exceptions = []
 
-        if raw is not EMPTY:
-            self.assertEqual(item.raw, raw)
         if latitude is not EMPTY:
-            self.assertAlmostEqual(
-                item.latitude, latitude, delta=self.delta
-            )
+            try:
+                self.assertAlmostEqual(
+                    item.latitude, latitude, delta=delta,
+                    msg="latitude differs",
+                )
+            except AssertionError as e:
+                exceptions.append(e)
         if longitude is not EMPTY:
-            self.assertAlmostEqual(
-                item.longitude, longitude, delta=self.delta
-            )
+            try:
+                self.assertAlmostEqual(
+                    item.longitude, longitude, delta=delta,
+                    msg="longitude differs",
+                )
+            except AssertionError as e:
+                exceptions.append(e)
         if address is not EMPTY:
-            self.assertEqual(item.address, address)
+            try:
+                self.assertEqual(item.address, address,
+                                 msg="address differs")
+            except AssertionError as e:
+                exceptions.append(e)
+
+        self.assertFalse(exceptions)