Make APIVersion's null check more pythonic

Our current APIVersion object has an is_null method to check when the
version instance is null (major=0 and minor=0).

While this works it is not very pythonic, since you have to write
expressions such as:

 if not min_version and not max_version:
     return True
 elif ((min_version and max_version) and
       max_version.is_null() and min_version.is_null()):
     return True

This patch removes the is_null method and instead implements the truth
value testing to simplify expressions and make code more pythonic.

So previous code would just look like:

 if not min_version and not max_version:
     return True

Because this will work with min_version being None or being an
APIVersion instance with major=0 and minor=0.

Change-Id: I7497c5dc940c1e726507117cadbad232d8c1d80d

cinderclient/api_versions.py

@@ -75,13 +75,14 @@ class APIVersion(object):
                 % (self.ver_major, self.ver_minor))
 
     def __repr__(self):
-        if self.is_null():
+        if self:
-            return "<APIVersion: null>"
-        else:
             return "<APIVersion: %s>" % self.get_string()
+        return "<APIVersion: null>"
 
-    def is_null(self):
+    def __bool__(self):
-        return self.ver_major == 0 and self.ver_minor == 0
+        return self.ver_major != 0 or self.ver_minor != 0
+
+    __nonzero__ = __bool__
 
     def is_latest(self):
         return self.ver_minor == float("inf")
@@ -133,7 +134,7 @@ class APIVersion(object):
         If self is null then raise ValueError
         """
 
-        if self.is_null():
+        if not self:
             raise ValueError("Null APIVersion doesn't support 'matches'.")
 
         if isinstance(min_version, str):
@@ -141,24 +142,21 @@ class APIVersion(object):
         if isinstance(max_version, str):
             max_version = APIVersion(version_str=max_version)
 
+        # This will work when they are None and when they are version 0.0
         if not min_version and not max_version:
             return True
-        elif ((min_version and max_version) and
-              max_version.is_null() and min_version.is_null()):
-            return True
 
-        elif not max_version or max_version.is_null():
+        if not max_version:
             return min_version <= self
-        elif not min_version or min_version.is_null():
+        if not min_version:
             return self <= max_version
-        else:
+        return min_version <= self <= max_version
-            return min_version <= self <= max_version
 
     def get_string(self):
         """Converts object to string representation which if used to create
         an APIVersion object results in the same version.
         """
-        if self.is_null():
+        if not self:
             raise ValueError("Null APIVersion cannot be converted to string.")
         elif self.is_latest():
             return "%s.%s" % (self.ver_major, "latest")
@@ -208,8 +206,7 @@ def check_major_version(api_version):
                                                       supported
     """
     available_versions = get_available_major_versions()
-    if (not api_version.is_null() and
+    if (api_version and str(api_version.ver_major) not in available_versions):
-            str(api_version.ver_major) not in available_versions):
         if len(available_versions) == 1:
             msg = ("Invalid client version '%(version)s'. "
                    "Major part should be '%(major)s'") % {
@@ -260,9 +257,10 @@ def discover_version(client, requested_version):
     server_start_version, server_end_version = _get_server_version_range(
         client)
 
+    both_versions_null = not (server_start_version or server_end_version)
     if (not requested_version.is_latest() and
             requested_version != APIVersion('2.0')):
-        if server_start_version.is_null() and server_end_version.is_null():
+        if both_versions_null:
             raise exceptions.UnsupportedVersion(
                 _("Server doesn't support microversions"))
         if not requested_version.matches(server_start_version,
@@ -275,18 +273,15 @@ def discover_version(client, requested_version):
         return requested_version
 
     if requested_version == APIVersion('2.0'):
-        if (server_start_version == APIVersion('2.1') or
+        if server_start_version == APIVersion('2.1') or both_versions_null:
-                (server_start_version.is_null() and
-                 server_end_version.is_null())):
             return APIVersion('2.0')
-        else:
+        raise exceptions.UnsupportedVersion(
-            raise exceptions.UnsupportedVersion(
+            _("The server isn't backward compatible with Cinder V2 REST "
-                _("The server isn't backward compatible with Cinder V2 REST "
+              "API"))
-                  "API"))
 
-    if server_start_version.is_null() and server_end_version.is_null():
+    if both_versions_null:
         return APIVersion('2.0')
-    elif cinderclient.API_MIN_VERSION > server_end_version:
+    if cinderclient.API_MIN_VERSION > server_end_version:
         raise exceptions.UnsupportedVersion(
             _("Server version is too old. The client valid version range is "
               "'%(client_min)s' to '%(client_max)s'. The server valid version "
@@ -315,7 +310,7 @@ def update_headers(headers, api_version):
        null
     """
 
-    if not api_version.is_null() and api_version.ver_minor != 0:
+    if api_version and api_version.ver_minor != 0:
         headers["OpenStack-API-Version"] = "volume " + api_version.get_string()
 
 
@@ -326,7 +321,7 @@ def add_substitution(versioned_method):
 
 def get_substitutions(func_name, api_version=None):
     substitutions = _SUBSTITUTIONS.get(func_name, [])
-    if api_version and not api_version.is_null():
+    if api_version:
         return [m for m in substitutions
                 if api_version.matches(m.start_version, m.end_version)]
     return substitutions

cinderclient/exceptions.py

@@ -34,19 +34,19 @@ class UnsupportedAttribute(AttributeError):
     """
 
     def __init__(self, argument_name, start_version, end_version):
-        if not start_version.is_null() and not end_version.is_null():
+        if start_version and end_version:
             self.message = (
                 "'%(name)s' argument is only allowed for microversions "
                 "%(start)s - %(end)s." % {"name": argument_name,
                                           "start": start_version.get_string(),
                                           "end": end_version.get_string()})
-        elif not start_version.is_null():
+        elif start_version:
             self.message = (
                 "'%(name)s' argument is only allowed since microversion "
                 "%(start)s." % {"name": argument_name,
                                 "start": start_version.get_string()})
 
-        elif not end_version.is_null():
+        elif end_version:
             self.message = (
                 "'%(name)s' argument is not allowed after microversion "
                 "%(end)s." % {"name": argument_name,

cinderclient/shell.py

@@ -433,11 +433,11 @@ class OpenStackCinderShell(object):
         subparser.set_defaults(func=self.do_bash_completion)
 
     def _build_versioned_help_message(self, start_version, end_version):
-        if not start_version.is_null() and not end_version.is_null():
+        if start_version and end_version:
             msg = (_(" (Supported by API versions %(start)s - %(end)s)")
                 % {"start": start_version.get_string(),
                    "end": end_version.get_string()})
-        elif not start_version.is_null():
+        elif start_version:
             msg = (_(" (Supported by API version %(start)s and later)")
                 % {"start": start_version.get_string()})
         else:
@@ -504,8 +504,7 @@ class OpenStackCinderShell(object):
                 start_version = api_versions.APIVersion(start_version)
                 end_version = kwargs.get('end_version', None)
                 end_version = api_versions.APIVersion(end_version)
-                if do_help and not (start_version.is_null()
+                if do_help and (start_version or end_version):
-                                    and end_version.is_null()):
                     kwargs["help"] = kwargs.get("help", "") + (
                         self._build_versioned_help_message(start_version,
                                                            end_version))

cinderclient/tests/unit/test_api_versions.py

@@ -40,7 +40,11 @@ class APIVersionTestCase(utils.TestCase):
 
     def test_null_version(self):
         v = api_versions.APIVersion()
-        self.assertTrue(v.is_null())
+        self.assertFalse(v)
+
+    def test_not_null_version(self):
+        v = api_versions.APIVersion('1.1')
+        self.assertTrue(v)
 
     @ddt.data("2", "200", "2.1.4", "200.23.66.3", "5 .3", "5. 3", "5.03",
               "02.1", "2.001", "", " 2.1", "2.1 ")
@@ -159,16 +163,24 @@ class GetAPIVersionTestCase(utils.TestCase):
         self.assertRaises(exceptions.UnsupportedVersion,
                           api_versions.get_api_version, "4")
 
+    @mock.patch("cinderclient.api_versions.get_available_major_versions")
     @mock.patch("cinderclient.api_versions.APIVersion")
-    def test_only_major_part_is_presented(self, mock_apiversion):
+    def test_only_major_part_is_presented(self, mock_apiversion,
+                                          mock_get_majors):
+        mock_get_majors.return_value = [
+            str(mock_apiversion.return_value.ver_major)]
         version = 7
         self.assertEqual(mock_apiversion.return_value,
                          api_versions.get_api_version(version))
         mock_apiversion.assert_called_once_with("%s.0" % str(version))
 
+    @mock.patch("cinderclient.api_versions.get_available_major_versions")
     @mock.patch("cinderclient.api_versions.APIVersion")
-    def test_major_and_minor_parts_is_presented(self, mock_apiversion):
+    def test_major_and_minor_parts_is_presented(self, mock_apiversion,
+                                                mock_get_majors):
         version = "2.7"
+        mock_get_majors.return_value = [
+            str(mock_apiversion.return_value.ver_major)]
         self.assertEqual(mock_apiversion.return_value,
                          api_versions.get_api_version(version))
         mock_apiversion.assert_called_once_with(version)