improve code quality and conventions

Elias Nygren · Elias Nygren · commit ed9ee63ad25b · 2016-08-30T15:20:59.000+03:00
diff --git a/setup.py b/setup.py
@@ -4,15 +4,15 @@
 
 setup(
     name='upcloud-api',
-    version='0.3.8',
+    version='0.4.0',
     description='UpCloud API Client',
     author='Elias Nygren',
     author_email='elias.nygren@upcloud.com',
     maintainer='Elias Nygren',
     maintainer_email='elias.nygren@upcloud.com',
     url='https://github.com/UpCloudLtd/upcloud-python-api',
     packages=['upcloud_api', 'upcloud_api.cloud_manager'],
-    download='https://github.com/UpCloudLtd/upcloud-python-api/tarball/v0.3.8',
+    download='https://github.com/UpCloudLtd/upcloud-python-api/tarball/v0.4.0',
     license='MIT',
     install_requires=[
         'requests>=2.6.0',
diff --git a/test/test_tags.py b/test/test_tags.py
@@ -22,7 +22,8 @@ def tag_post_callback(request):
     if 'servers' in request_body['tag']:
         assert isinstance(request_body['tag']['servers'], dict)
         assert isinstance(request_body['tag']['servers']['server'], list)
-        assert isinstance(request_body['tag']['servers']['server'][0], six.string_types)
+        if len(request_body['tag']['servers']['server']) > 0:
+            assert isinstance(request_body['tag']['servers']['server'][0], six.string_types)
 
     if 'description' in request_body['tag']:
         assert isinstance(request_body['tag']['description'], six.string_types)
diff --git a/upcloud_api/__init__.py b/upcloud_api/__init__.py
@@ -7,18 +7,18 @@
 from __future__ import unicode_literals
 from __future__ import absolute_import
 
-__version__ = '0.3.8'
+__version__ = '0.4.0'
 __author__ = 'Elias Nygren'
 __author_email__ = 'elias.nygren@upcloud.com'
 __license__ = 'MIT'
 __copyright__ = 'Copyright (c) 2015 Elias Nygren'
 
 from upcloud_api.upcloud_resource import UpCloudResource
-from upcloud_api.errors import UpCloudAPIError
+from upcloud_api.errors import UpCloudClientError, UpCloudAPIError
+from upcloud_api.constants import OperatingSystems, ZONE
 from upcloud_api.storage import Storage
 from upcloud_api.ip_address import IPAddress
 from upcloud_api.server import Server, login_user_block
 from upcloud_api.firewall import FirewallRule
-from upcloud_api.tools import OperatingSystems, ZONE
 from upcloud_api.tag import Tag
 from upcloud_api.cloud_manager.cloud_manager import CloudManager
diff --git a/upcloud_api/cloud_manager/__init__.py b/upcloud_api/cloud_manager/__init__.py
@@ -3,6 +3,7 @@
 from __future__ import division
 from __future__ import absolute_import
 
+from upcloud_api.cloud_manager.base import BaseAPI
 from upcloud_api.cloud_manager.firewall_mixin import FirewallManager
 from upcloud_api.cloud_manager.ip_address_mixin import IPManager
 from upcloud_api.cloud_manager.server_mixin import ServerManager
diff --git a/upcloud_api/cloud_manager/base.py b/upcloud_api/cloud_manager/base.py
diff --git a/upcloud_api/cloud_manager/cloud_manager.py b/upcloud_api/cloud_manager/cloud_manager.py
@@ -2,8 +2,8 @@
 from __future__ import absolute_import
 import base64
 
-from upcloud_api.base import BaseAPI
 from upcloud_api.cloud_manager import (
+    BaseAPI,
     ServerManager,
     IPManager,
     StorageManager,
diff --git a/upcloud_api/cloud_manager/firewall_mixin.py b/upcloud_api/cloud_manager/firewall_mixin.py
@@ -7,8 +7,7 @@ def uuid_and_instance(server):
     """server => uuid, instance"""
     if isinstance(server, Server):
         return server.uuid, server
-    else:
-        return server, None
+    return server, None
 
 class FirewallManager(object):
     """
diff --git a/upcloud_api/cloud_manager/ip_address_mixin.py b/upcloud_api/cloud_manager/ip_address_mixin.py
@@ -34,12 +34,9 @@ def attach_ip(self, server, family='IPv4'):
         """
         Attach a new (random) IPAddress to the given server (object or UUID).
         """
-        if not isinstance(server, six.string_types):
-            server = server.uuid
-
         body = {
             'ip_address': {
-                'server': server,
+                'server': str(server),
                 'family': family
             }
         }
@@ -53,16 +50,13 @@ def modify_ip(self, ip_addr, ptr_record):
 
         Accepts an IPAddress instance (object) or its address (string).
         """
-        if not isinstance(ip_addr, six.string_types):
-            ip_addr = ip_addr.address
-
         body = {
             'ip_address': {
                 'ptr_record': ptr_record
             }
         }
 
-        res = self.request('PUT', '/ip_address/' + ip_addr, body)
+        res = self.request('PUT', '/ip_address/' + str(ip_addr), body)
         return IPAddress(cloud_manager=self, **res['ip_address'])
 
     def release_ip(self, ip_addr):
@@ -71,7 +65,4 @@ def release_ip(self, ip_addr):
 
         Accepts an IPAddress instance (object) or its address (string).
         """
-        if not isinstance(ip_addr, six.string_types):
-            ip_addr = ip_addr.address
-
-        return self.request('DELETE', '/ip_address/' + ip_addr)
+        return self.request('DELETE', '/ip_address/' + str(ip_addr))
diff --git a/upcloud_api/cloud_manager/server_mixin.py b/upcloud_api/cloud_manager/server_mixin.py
@@ -4,7 +4,7 @@
 from __future__ import absolute_import
 
 from upcloud_api import IPAddress, Server, Storage
-from upcloud_api.tools import assignIfExists
+from upcloud_api.utils import assignIfExists
 
 
 class ServerManager(object):
diff --git a/upcloud_api/cloud_manager/tag_mixin.py b/upcloud_api/cloud_manager/tag_mixin.py
@@ -32,7 +32,7 @@ def create_tag(self, name, description=None, servers=[]):
         Returns the created Tag object.
         """
         servers = [str(server) for server in servers]
-        body = Tag(name, description, servers).to_dict()
+        body = {'tag': Tag(name, description, servers).to_dict()}
         res = self.request('POST', '/tag', body)
 
         return Tag(cloud_manager=self, **res['tag'])
@@ -43,7 +43,7 @@ def _modify_tag(self, name, description, servers, new_name):
 
         Private method used by the Tag class and TagManager.modify_tag.
         """
-        body = Tag(new_name, description, servers).to_dict()
+        body = {'tag': Tag(new_name, description, servers).to_dict()}
         res = self.request('PUT', '/tag/' + name, body)
         return res['tag']
 
@@ -82,7 +82,4 @@ def remove_tags(self, server, tags):
 
     def delete_tag(self, tag):
         """Delete the Tag. Returns and empty object."""
-        if not isinstance(tag, six.string_types):
-            tag = tag.name
-
-        return self.request('DELETE', '/tag/' + tag)
+        return self.request('DELETE', '/tag/' + str(tag))
diff --git a/upcloud_api/constants.py b/upcloud_api/constants.py
@@ -3,17 +3,6 @@
 
 import re
 
-
-def assignIfExists(opts, default=None, **kwargs):
-    """
-    Helper for assigning object attributes from API responses.
-    """
-    for opt in opts:
-        if(opt in kwargs):
-            return kwargs[opt]
-    return default
-
-
 class ZONE(object):
     """
     Enums for UpCloud's Zones.
diff --git a/upcloud_api/errors.py b/upcloud_api/errors.py
@@ -1,5 +1,8 @@
 
-class UpCloudAPIError(Exception):
+class UpCloudClientError(Exception):
+    pass
+
+class UpCloudAPIError(UpCloudClientError):
     """Custom Error class for UpCloud API error responses.
 
     Each API call returns an `error_code` and `error_message` that
diff --git a/upcloud_api/firewall.py b/upcloud_api/firewall.py
@@ -3,7 +3,7 @@
 
 class FirewallRule(UpCloudResource):
     """
-    Object representation of the FirewallRule in UpCloud's API.
+    Class representation of the API's firewall rule. Extends UpCloudResource.
     """
 
     ATTRIBUTES = {
diff --git a/upcloud_api/ip_address.py b/upcloud_api/ip_address.py
@@ -3,7 +3,7 @@
 
 class IPAddress(UpCloudResource):
     """
-    Object representation of the IP-address.
+    Class representation of the API's IP address. Extends UpCloudResource.
 
     Attributes:
     access -- "public" or "private"
@@ -36,12 +36,19 @@ def destroy(self):
         """
         self.cloud_manager.release_ip(self.address)
 
-    def __str__(self):  # noqa
-        return 'IP-address: ' + self.address
+    def __str__(self):
+        """
+        String representation of IPAddress.
+        Can be used to add tags into API requests: str(ip_addr).
+        """
+        return self.address
 
     @staticmethod
     def _create_ip_address_objs(ip_addresses, cloud_manager):
-
+        """
+        Create IPAddress objects from API response data.
+        Also associates CloudManager with the objects.
+        """
         # ip-addresses might be provided as a flat array or as a following dict:
         # {'ip_addresses': {'ip_address': [...]}} || {'ip_address': [...]}
 
diff --git a/upcloud_api/storage.py b/upcloud_api/storage.py
@@ -3,7 +3,8 @@
 from __future__ import division
 from __future__ import absolute_import
 
-from upcloud_api.tools import assignIfExists, OperatingSystems
+from upcloud_api import OperatingSystems
+from upcloud_api.utils import assignIfExists
 
 
 class Storage(object):
diff --git a/upcloud_api/tag.py b/upcloud_api/tag.py
@@ -8,13 +8,13 @@
 
 class Tag(UpCloudResource):
     """
-    Class representation of UpCloud Tag.
+    Class representation of the API's tags. Extends UpCloudResource.
 
-    - name: unique name for the tag.
-    - description: optional description
-    - servers:
-        list of Server objects (with only uuid populated).
-        Can be instantiated with UUID strings or Server objects.
+    Attributes:
+    name -- unique name for the tag
+    description -- optional description
+    servers -- list of Server objects (with only uuid populated)
+               can be instantiated with UUID strings or Server objects
     """
 
     ATTRIBUTES = {
@@ -46,6 +46,14 @@ def _reset(self, **kwargs):
         if self.servers and isinstance(self.servers[0], six.string_types):
             self.servers = [Server(uuid=server, populated=False) for server in self.servers]
 
+    @property
+    def server_uuids(self):
+        """
+        Return the tag's servers as UUIDs.
+        Useful for forming API requests.
+        """
+        return [server.uuid for server in self.servers]
+
     def save(self):
         tag_dict = self.cloud_manager._modify_tag(self._api_name,
                                                   self.description,
@@ -54,27 +62,26 @@ def save(self):
         self._reset(**tag_dict)
 
     def destroy(self):
+        """
+        Destroy the tag at the API.
+        """
         self.cloud_manager.delete_tag(self.name)
 
-    @property
-    def server_uuids(self):
-        """return the tag's servers as UUIDs."""
-        return [server.uuid for server in self.servers]
+    def to_dict(self):
+        """
+        Return a dict that can be serialised to JSON and sent to UpCloud's API.
+        """
+        return {
+            'name': self.name,
+            'description': self.description or '',
+            'servers': {
+                'server': self.server_uuids
+            }
+        }
 
     def __str__(self):
+        """
+        String representation of Tag.
+        Can be used to add tags into API requests: str(tag).
+        """
         return self.name
-
-    def to_dict(self):
-        body = dict()
-        body['tag'] = dict()
-
-        if self.name:
-            body['tag']['name'] = self.name
-
-        if self.description:
-            body['tag']['description'] = self.description
-
-        if self.servers:
-            body['tag']['servers'] = dict()
-            body['tag']['servers']['server'] = [str(server) for server in self.servers]
-        return body
diff --git a/upcloud_api/upcloud_resource.py b/upcloud_api/upcloud_resource.py
@@ -1,5 +1,18 @@
 
 class UpCloudResource(object):
+    """
+    Base class for all API resources.
+
+    ATTRIBUTES is used to define serialization (see: to_dict)
+    and defaults (see: __init__ and _reset).
+
+    All UpCloudResources:
+    - must define ATTRIBUTES accordingly with https://www.upcloud.com/api/ (doc)
+    - must have `to_dict` for JSON serialization
+    - must have `_reset` for initializing and refreshing the instance with updated data
+    - must call `UpCloudResource.__init__` (that uses `_reset`)
+    - optionally implement `sync` for refreshing the instance with new data from API
+    """
 
     ATTRIBUTES = {}  # subclass should define this
 
@@ -23,9 +36,8 @@ def _reset(self, **kwargs):
         for attr in self.ATTRIBUTES:
             if not hasattr(self, attr) and self.ATTRIBUTES[attr] is not None:
                 setattr(self, attr, self.ATTRIBUTES[attr])
-        print('muh', self.__dict__)
 
-    def sync_from_api(self):
+    def sync(self):
         """
         Sync the object from the API and use the internal resource._reset to
         update fields.
diff --git a/upcloud_api/utils.py b/upcloud_api/utils.py
@@ -1,7 +1,17 @@
 import itertools
 from time import sleep
 
-from upcloud_api import UpCloudAPIError
+from upcloud_api import UpCloudClientError
+
+
+def assignIfExists(opts, default=None, **kwargs):
+    """
+    Helper for assigning object attributes from API responses.
+    """
+    for opt in opts:
+        if(opt in kwargs):
+            return kwargs[opt]
+    return default
 
 
 def try_it_n_times(operation, expected_error_codes, custom_error='operation failed', n=5):
@@ -21,4 +31,4 @@ def try_it_n_times(operation, expected_error_codes, custom_error='operation fail
                 raise e
             sleep(3)
         if i >= n - 1:
-            raise Exception(custom_error)
+            raise UpCloudClientError(custom_error)
