Merge pull request #8 from gadventures/refactor-request

Refactor request

gapipy/query.py

@@ -94,7 +94,7 @@ def all(self, limit=None):
         requestor = APIRequestor(
             self._client,
             self.resource._resource_name,
-            options=self.filters,
+            params=self.filters,
             parent=self.parent
         )
         generator = requestor.list()
@@ -126,7 +126,7 @@ def count(self):
         requestor = APIRequestor(
             self._client,
             self.resource._resource_name,
-            options=self.filters,
+            params=self.filters,
             parent=self.parent
         )
         response = requestor.list_raw()

gapipy/request.py

@@ -9,19 +9,28 @@
 
 JSON_CONTENT_TYPE = 'application/json'
 
+
 class APIRequestor(object):
 
-    def __init__(self, client, resource, options=None, parent=None):
+    def __init__(self, client, resource, params=None, parent=None):
         self.client = client
         self.resource = resource
-        self.options = options
+        self.params = params
         self.parent = parent
 
-    def _request(self, uri, method, data=None, options=None, additional_headers=None):
+    def _request(self, uri, method, data=None, params=None, additional_headers=None):
         """Make an HTTP request to a target API method with proper headers."""
 
         assert method in ['GET', 'POST', 'PUT', 'PATCH'], "Only 'GET', 'POST', 'PUT', and 'PATCH' are allowed."
 
+        url = self._get_url(uri)
+        headers = self._get_headers(method, additional_headers)
+        response = self._make_call(method, url, headers, data, params)
+        return response
+
+    def _get_url(self, uri):
+        """Return the full URL to make a request to for the given `uri`"""
+
         # Support supplying a full url
         if '://' in uri:
             url = uri
@@ -34,6 +43,11 @@ def _request(self, uri, method, data=None, options=None, additional_headers=None
         if api_proxy:
             url = url.replace(api_proxy, '')
 
+        return url
+
+    def _get_headers(self, method, additional_headers):
+        """Return a dictionary of HTTP headers to set on the request to the API."""
+
         headers = {
             'User-Agent': '{0}/{1}'.format(__title__, __version__),
             'X-Application-Key': self.client.application_key,
@@ -50,13 +64,20 @@ def _request(self, uri, method, data=None, options=None, additional_headers=None
         if additional_headers:
             headers.update(additional_headers)
 
-        if api_proxy:
-            headers.update({'X-Api-Proxy': api_proxy})
+        if self.client.api_proxy:
+            headers.update({'X-Api-Proxy': self.client.api_proxy})
+
+        return headers
 
+    def _make_call(self, method, url, headers, data, params):
+        """Make the actual request to the API, using the given URL, headers,
+        data and extra parameters.
+        """
         requests_call = getattr(requests, method.lower())
 
         self.client.logger.debug('Making a {0} request to {1}'.format(method, url))
-        response = requests_call(url, headers=headers, data=data, params=options)
+
+        response = requests_call(url, headers=headers, data=data, params=params)
 
         if response.status_code in ACCEPTABLE_RESPONSE_STATUS_CODES:
             return response.json()
@@ -109,7 +130,7 @@ def list_raw(self, uri=None):
             else:
                 uri = '/{0}'.format(self.resource)
 
-        return self._request(uri, 'GET', options=self.options)
+        return self._request(uri, 'GET', params=self.params)
 
     def list(self, uri=None):
         """Generator for listing resources"""

tests/test_query.py

@@ -68,12 +68,12 @@ def test_filtered_query(self, mock_request):
         query = Query(self.client, Tour).filter(tour_dossier_code='PPP')
         list(query)  # force query evaluation
         mock_request.assert_called_once_with(
-            '/tours', 'GET', options={'tour_dossier_code': 'PPP'})
+            '/tours', 'GET', params={'tour_dossier_code': 'PPP'})
 
         # Check that filters can be chained
         list(query.filter(departures_start_date='2014-01-01'))
         mock_request.assert_called_with(
-            '/tours', 'GET', options={
+            '/tours', 'GET', params={
                 'tour_dossier_code': 'PPP',
                 'departures_start_date': '2014-01-01'
             })
@@ -118,7 +118,7 @@ def test_fetch_all(self, mock_request):
             self.assertIsInstance(dossier, TourDossier)
 
         mock_request.assert_called_once_with(
-            '/tour_dossiers', 'GET', options={})
+            '/tour_dossiers', 'GET', params={})
 
     @patch('gapipy.request.APIRequestor._request', return_value=TOUR_DOSSIER_LIST_DATA)
     def test_fetch_all_with_limit(self, mock_request):
@@ -131,7 +131,7 @@ def test_fetch_all_with_limit(self, mock_request):
             self.assertIsInstance(dossier, TourDossier)
 
         mock_request.assert_called_once_with(
-            '/tour_dossiers', 'GET', options={})
+            '/tour_dossiers', 'GET', params={})
 
     def test_fetch_all_with_wrong_argument_for_limit(self):
         message = '`limit` must be a positive integer'

tests/test_request.py

@@ -22,14 +22,14 @@ def test_get_resource_by_id(self, mock_request):
     def test_list_resource(self, mock_request):
         requestor = APIRequestor(self.client, 'resources')
         requestor.list_raw()
-        mock_request.assert_called_once_with('/resources', 'GET', options=None)
+        mock_request.assert_called_once_with('/resources', 'GET', params=None)
 
     def test_list_resource_with_parent(self, mock_request):
         parent = ('parent', '1234')
         requestor = APIRequestor(self.client, 'child', parent=parent)
         requestor.list_raw()
         mock_request.assert_called_once_with(
-            '/parent/1234/child', 'GET', options=None)
+            '/parent/1234/child', 'GET', params=None)
 
     @patch('gapipy.request.APIRequestor.list_raw')
     def test_list_generator(self, mock_list, mock_request):