From 200bd96c9a6640a0ab6eb8dc292ff0ed78bf839e Mon Sep 17 00:00:00 2001 From: Mats Lindh Date: Fri, 6 Jan 2017 14:41:13 +0100 Subject: [PATCH 1/5] Make methods static --- imboclient/client.py | 28 ++++++++++++++++------------ 1 file changed, 16 insertions(+), 12 deletions(-) diff --git a/imboclient/client.py b/imboclient/client.py index 4bbc6be..183b916 100644 --- a/imboclient/client.py +++ b/imboclient/client.py @@ -204,9 +204,6 @@ def http_user_info(self): return self._wrap_result_json(http_user_info, [200], 'Failed getting user info.') - def _image_file_data(self, path): - return open(path, 'rb').read() - def _parse_urls(self, urls): def should_remove_port(self, url_parts): return parts.port and (parts.scheme == 'http' and parts.port == 80 or (parts.scheme == 'https' and parts.port == 443)) @@ -227,18 +224,9 @@ def should_remove_port(self, url_parts): return parsed_urls - def _current_timestamp(self): - return time.strftime("%Y-%m-%dT%H:%M:%SZ", time.gmtime()) - def _auth_headers(self, method, url): return authenticate.Authenticate(self._public_key, self._private_key, method, url, self._current_timestamp()).headers() - def _validate_local_file(self, path): - return os.path.isfile(path) and os.path.getsize(path) > 0 - - def _generate_image_identifier(self, content): - return hashlib.md5(content).hexdigest() - def _host_for_image_identifier(self, image_identifier): dec = int(image_identifier[0] + image_identifier[1], 16) return self.server_urls[dec % len(self.server_urls)] @@ -263,6 +251,22 @@ def _wrap_result(self, function, success_status_codes, error): else: raise self.ImboInternalError(error + ' Imbo returned HTTP ' + str(response.status_code) + ' and body \'' + response.text + '\'') + @classmethod + def _current_timestamp(cls): + return time.strftime("%Y-%m-%dT%H:%M:%SZ", time.gmtime()) + + @classmethod + def _generate_image_identifier(cls, content): + return hashlib.md5(content).hexdigest() + + @classmethod + def _image_file_data(cls, path): + return open(path, 'rb').read() + + @classmethod + def _validate_local_file(cls, path): + return os.path.isfile(path) and os.path.getsize(path) > 0 + class ImboTransportError(Exception): pass From 68d3d48c91ef0979770aca426c83f6b0ee2e17b8 Mon Sep 17 00:00:00 2001 From: Mats Lindh Date: Fri, 6 Jan 2017 14:41:52 +0100 Subject: [PATCH 2/5] Remove unreachable statement --- imboclient/client.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/imboclient/client.py b/imboclient/client.py index 183b916..03da577 100644 --- a/imboclient/client.py +++ b/imboclient/client.py @@ -142,8 +142,6 @@ def http_num_images(self): except KeyError as error: raise self.ImboInternalError(error + ' Could not extract number of images from Imbo response.') - return 0 - def images(self, query = None): images_url = images.UrlImages(self.server_urls[0], self._public_key, self._private_key) From afb08efc2afb4258e53c9bb42b3dc107948d4f55 Mon Sep 17 00:00:00 2001 From: Mats Lindh Date: Fri, 6 Jan 2017 14:43:18 +0100 Subject: [PATCH 3/5] Clean up exception messages We need to explicitly cast these exceptions to a str before using them - as we're concatenating the messages. The ValueError is thrown by the JSON parsing, so the response line should be moved outside of the try/catch statement, so that we can be sure that the `response` value is actually defined for the catch statement. --- imboclient/client.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/imboclient/client.py b/imboclient/client.py index 03da577..22017be 100644 --- a/imboclient/client.py +++ b/imboclient/client.py @@ -140,7 +140,7 @@ def http_num_images(self): try: return user_data_decoded['numImages'] except KeyError as error: - raise self.ImboInternalError(error + ' Could not extract number of images from Imbo response.') + raise self.ImboInternalError(str(error) + ' Could not extract number of images from Imbo response.') def images(self, query = None): images_url = images.UrlImages(self.server_urls[0], self._public_key, self._private_key) @@ -175,7 +175,7 @@ def image_properties(self, image_identifier): "x-imbo-originalextension": headers["x-imbo-originalextension"] } except KeyError as key_error: - raise self.ImboInternalError(key_error + ' Imbo failed returning image properties.') + raise self.ImboInternalError(str(key_error) + ' Imbo failed returning image properties.') def image_identifier(self, path): if self._validate_local_file(path): @@ -230,8 +230,9 @@ def _host_for_image_identifier(self, image_identifier): return self.server_urls[dec % len(self.server_urls)] def _wrap_result_json(self, function, success_status_codes, error): + response = self._wrap_result(function, success_status_codes, error) + try: - response = self._wrap_result(function, success_status_codes, error) response_json = response.json() except ValueError as value_error: raise self.ImboInternalError(error + ' The response from Imbo could not be parsed as JSON: \'' + response.text + '\'') From f9adf6cedfb19c6b774b3454b5d9c51ed8bfaec4 Mon Sep 17 00:00:00 2001 From: Mats Lindh Date: Fri, 6 Jan 2017 14:45:11 +0100 Subject: [PATCH 4/5] Remove pep8 conflict in query= parameter definition --- imboclient/client.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/imboclient/client.py b/imboclient/client.py index 22017be..0d0ee1b 100644 --- a/imboclient/client.py +++ b/imboclient/client.py @@ -142,7 +142,7 @@ def http_num_images(self): except KeyError as error: raise self.ImboInternalError(str(error) + ' Could not extract number of images from Imbo response.') - def images(self, query = None): + def images(self, query=None): images_url = images.UrlImages(self.server_urls[0], self._public_key, self._private_key) if query: From 89512e653389e8a8ff10eff23e0558a9adf0844d Mon Sep 17 00:00:00 2001 From: Mats Lindh Date: Fri, 6 Jan 2017 14:46:23 +0100 Subject: [PATCH 5/5] Avoid assigning an object property when testing There is no need to assign these client tests to the object itself, as the result is tested within the method. The assignment _could_ have side effects somewhere later (if the teardown for some reason isn't ran as assumed). To avoid any potential side effects we keep the assignments local to the method. --- imboclient/test/unit/test_client.py | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/imboclient/test/unit/test_client.py b/imboclient/test/unit/test_client.py index a1949bd..565a7a5 100644 --- a/imboclient/test/unit/test_client.py +++ b/imboclient/test/unit/test_client.py @@ -24,32 +24,32 @@ def teardown(self): self._client_with_user = None def test_server_urls_generic(self): - self._client = imbo.Client(['imbo.local'], 'public', 'private') - assert self._client.server_urls[0] == 'http://imbo.local' + client = imbo.Client(['imbo.local'], 'public', 'private') + assert client.server_urls[0] == 'http://imbo.local' def test_server_urls_http(self): - self._client = imbo.Client(['http://imbo.local'], 'public', 'private') - assert self._client.server_urls[0] == 'http://imbo.local' + client = imbo.Client(['http://imbo.local'], 'public', 'private') + assert client.server_urls[0] == 'http://imbo.local' def test_server_urls_https(self): - self._client = imbo.Client(['https://imbo.local'], 'public', 'private') - assert self._client.server_urls[0] == 'https://imbo.local' + client = imbo.Client(['https://imbo.local'], 'public', 'private') + assert client.server_urls[0] == 'https://imbo.local' def test_server_urls_port_normal(self): - self._client = imbo.Client(['http://imbo.local'], 'public', 'private') - assert self._client.server_urls[0] == 'http://imbo.local' + client = imbo.Client(['http://imbo.local'], 'public', 'private') + assert client.server_urls[0] == 'http://imbo.local' def test_server_urls_port_normal_explicit(self): - self._client = imbo.Client(['http://imbo.local:80'], 'public', 'private') - assert self._client.server_urls[0] == 'http://imbo.local' + client = imbo.Client(['http://imbo.local:80'], 'public', 'private') + assert client.server_urls[0] == 'http://imbo.local' def test_server_urls_port_ssl(self): - self._client = imbo.Client(['https://imbo.local:443'], 'public', 'private') - assert self._client.server_urls[0] == 'https://imbo.local' + client = imbo.Client(['https://imbo.local:443'], 'public', 'private') + assert client.server_urls[0] == 'https://imbo.local' def test_server_urls_port_explicit_without_protocol(self): - self._client = imbo.Client(['imbo.local:8000'], 'public', 'private') - assert self._client.server_urls[0] == 'http://imbo.local:8000' + client = imbo.Client(['imbo.local:8000'], 'public', 'private') + assert client.server_urls[0] == 'http://imbo.local:8000' @patch('imboclient.url.status.UrlStatus') def test_status_url(self, mocked_url_status):