Repository: ansible/tower-cli
Pull request #336 (2017-08-09 02:46:25)

allow for multiple inheritances with click methods
This has caused bugs in the past. I hope the test is expressive enough to show what is being solved.

Basically, we previously couldn't use mixins, because the click decoration would not be respected across multiple parent classes (see the naive `superclass = bases[0]`), which means that almost any attempt at using multiple classes would result in missing or incomplete commands.

Note that python functionality was never impacted. That was never really a problem. This deals with `@command(**kwargs)` in parent classes, mixins, and subclasses - how to choose which kwargs and other args to use and such.

coveralls: 
[![Coverage Status](https://coveralls.io/builds/12754451/badge)](https://coveralls.io/builds/12754451)

Coverage increased (+0.01%) to 92.326% when pulling **64f42a97c137fa4ee7241f7de3389ff2e65ae3f0 on AlanCoding:fix_bases** into **6d99a154db316b77793267c73f7fd17462d3cf1e on ansible:v1**.

AlanCoding: One option that might be a good alternative for how to manage this: if `_cli_command_attrs` is not present, raise a descriptive error message that methods across all classes in the inheritance chain must be consistently decorated with the command decorator.

For instance, if the subclass defines `list`, and they don't decorate it, they'll just get a message telling them to add that.

I don't _think_ there's any good argument one way or the other.

Later, I want to come back to test combining over many classes, and assure that arguments are not duplicated.
jangsutsr: I'm good with this PR as long as arguments are verified to have no duplicates (which is the case from code review). Please don't forget to duplicate and transplant this branch onto master (in our internal repo for now) and open a linked PR there before merging this PR (That would be our normal devel workflow from now on)
coveralls: 
[![Coverage Status](https://coveralls.io/builds/12957611/badge)](https://coveralls.io/builds/12957611)

Coverage increased (+0.01%) to 92.326% when pulling **aad75e2f89a9185f47719c0bf0aa1fb50c1e4b9e on AlanCoding:fix_bases** into **6d99a154db316b77793267c73f7fd17462d3cf1e on ansible:v1**.

coveralls: 
[![Coverage Status](https://coveralls.io/builds/12959697/badge)](https://coveralls.io/builds/12959697)

Coverage increased (+0.02%) to 92.331% when pulling **95caae4defd0b87da4ead9745c03ebb3028a5bc0 on AlanCoding:fix_bases** into **6d99a154db316b77793267c73f7fd17462d3cf1e on ansible:v1**.

coveralls: 
[![Coverage Status](https://coveralls.io/builds/12959697/badge)](https://coveralls.io/builds/12959697)

Coverage increased (+0.02%) to 92.331% when pulling **95caae4defd0b87da4ead9745c03ebb3028a5bc0 on AlanCoding:fix_bases** into **6d99a154db316b77793267c73f7fd17462d3cf1e on ansible:v1**.

coveralls: 
[![Coverage Status](https://coveralls.io/builds/12959697/badge)](https://coveralls.io/builds/12959697)

Coverage increased (+0.02%) to 92.331% when pulling **95caae4defd0b87da4ead9745c03ebb3028a5bc0 on AlanCoding:fix_bases** into **6d99a154db316b77793267c73f7fd17462d3cf1e on ansible:v1**.

AlanCoding: @jangsutsr good catch on the duplicate options. Added a test case for that and fixed it.

Patch:
diff --git a/HISTORY.rst b/HISTORY.rst
index 72ee1ff..2f26888 100644
--- a/HISTORY.rst
+++ b/HISTORY.rst
@@ -1,11 +1,6 @@
 Release History
 ===============
 
-3.1.7 (2017-08-07)
-------------------
-
-- Follow up 3.1.6 by duplicating exceptions.py to support `import tower_cli.utils.exceptions` syntax.
-
 3.1.6 (2017-07-18)
 ------------------
 
diff --git a/VERSION b/VERSION
index 23887f6..9cec716 100644
--- a/VERSION
+++ b/VERSION
@@ -1,1 +1,1 @@
-3.1.7
+3.1.6
diff --git a/packaging/rpm/ansible-tower-cli.spec b/packaging/rpm/ansible-tower-cli.spec
index 425f4b1..6ee4376 100644
--- a/packaging/rpm/ansible-tower-cli.spec
+++ b/packaging/rpm/ansible-tower-cli.spec
@@ -2,7 +2,7 @@
 %global srcname tower-cli
 
 Name:           ansible-%{srcname}
-Version:        3.1.7
+Version:        3.1.6
 Release:        2%{?dist}
 Summary:        Commandline interface for Ansible Tower
 Group:          Development/Tools
diff --git a/tower_cli/models/base.py b/tower_cli/models/base.py
index 45a0212..7ebe6e7 100644
--- a/tower_cli/models/base.py
+++ b/tower_cli/models/base.py
@@ -63,18 +63,34 @@ class ResourceMeta(type):
             # down to the subclass implementation.
             if not len(bases):
                 continue
-            superclass = bases[0]
-            super_method = getattr(superclass, key, None)
-            if super_method and getattr(super_method, '_cli_command', False):
-                # Copy the click parameters from the parent method to the
-                # child.
-                cp = getattr(value, '__click_params__', [])
-                cp = getattr(super_method, '__click_params__', []) + cp
-                value.__click_params__ = cp
-
-                # Copy the command attributes from the parent to the child,
-                # if the child has not overridden them.
-                for attkey, attval in super_method._cli_command_attrs.items():
+            cp = []
+            baseattrs = {}
+            for superclass in bases:
+                super_method = getattr(superclass, key, None)
+                if super_method and getattr(super_method, '_cli_command', False):
+                    # Copy the click parameters from the parent method to the
+                    # child.
+                    for param in getattr(super_method, '__click_params__', []):
+                        if param not in cp:
+                            cp.append(param)
+
+                    # Copy the command attributes from the parent to the child,
+                    # if the child has not overridden them.
+                    for attkey, attval in getattr(super_method, '_cli_command_attrs', {}).items():
+                        baseattrs.setdefault(attkey, attval)
+            if cp:
+                # If subclass method is not decorated as command, but parent
+                # classes do, then make it into a command here
+                if not hasattr(value, '__click_params__'):
+                    value.__click_params__ = []
+                if not hasattr(value, '_cli_command_attrs'):
+                    value._cli_command_attrs = {}
+
+                # Copy all parent click parameters to subclass method here
+                for param in cp:
+                    if param not in value.__click_params__:
+                        value.__click_params__.append(param)
+                for attkey, attval in baseattrs.items():
                     value._cli_command_attrs.setdefault(attkey, attval)
         attrs['commands'] = sorted(commands)
 
diff --git a/tower_cli/utils/exceptions.py b/tower_cli/utils/exceptions.py
deleted file mode 100644
index 21001f6..0000000
--- a/tower_cli/utils/exceptions.py
+++ /dev/null
@@ -1,151 +0,0 @@
-# Copyright 2015, Ansible, Inc.
-# Luke Sneeringer <lsneeringer@ansible.com>
-#
-# Licensed under the Apache License, Version 2.0 (the "License");
-# you may not use this file except in compliance with the License.
-# You may obtain a copy of the License at
-#
-#     http://www.apache.org/licenses/LICENSE-2.0
-#
-# Unless required by applicable law or agreed to in writing, software
-# distributed under the License is distributed on an "AS IS" BASIS,
-# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-# See the License for the specific language governing permissions and
-# limitations under the License.
-#
-# NOTE: this is a duplicate of tower_cli/exceptions.py for backward compatibility
-# with Ansible Tower modules.
-
-import click
-from click._compat import get_text_stderr
-
-
-class TowerCLIError(click.ClickException):
-    """Base exception class for problems raised within Tower CLI.
-    This class adds coloring to exceptions.
-    """
-    fg = 'red'
-    bg = None
-    bold = True
-
-    def show(self, file=None):
-        if file is None:
-            file = get_text_stderr()
-        click.secho('Error: %s' % self.format_message(), file=file,
-                    fg=self.fg, bg=self.bg, bold=self.bold)
-
-
-class UsageError(TowerCLIError):
-    """An exception class for reporting usage errors.
-
-    This uses an exit code of 2 in order to match click (which matters more
-    than following the erstwhile "standard" of using 64).
-    """
-    exit_code = 2
-
-
-class BadRequest(TowerCLIError):
-    """An exception class for reporting unexpected error codes from Ansible
-    Tower such that 400 <= code < 500.
-
-    In theory, we should never, ever get these.
-    """
-    exit_code = 40
-
-
-class AuthError(TowerCLIError):
-    """An exception class for reporting when a request failed due to an
-    authorization failure.
-    """
-    exit_code = 41
-
-
-class Forbidden(TowerCLIError):
-    """An exception class for reporting when a user doesn't have permission
-    to do something.
-    """
-    exit_code = 43
-
-
-class NotFound(TowerCLIError):
-    """An exception class for reporting when a request went through without
-    incident, but the requested content could not be found.
-    """
-    exit_code = 44
-
-
-class MethodNotAllowed(BadRequest):
-    """An exception class for sending a request to a URL where the URL doesn't
-    accept that method at all.
-    """
-    exit_code = 45
-
-
-class MultipleResults(TowerCLIError):
-    """An exception class for reporting when a request that expected one
-    and exactly one result got more than that.
-    """
-    exit_code = 49
-
-
-class ServerError(TowerCLIError):
-    """An exception class for reporting server-side errors which are expected
-    to be ephemeral.
-    """
-    exit_code = 50
-
-
-class Found(TowerCLIError):
-    """An exception class for when a record already exists, and we were
-    explicitly told that it shouldn't.
-    """
-    exit_code = 60
-
-
-class RelatedError(TowerCLIError):
-    """An exception class for errors where we can't find related objects
-    that we expect to find.
-    """
-    exit_code = 61
-
-
-class MultipleRelatedError(RelatedError):
-    """An exception class for errors where we try to find a single related
-    object, and get more than one.
-    """
-    exit_code = 62
-
-
-class ValidationError(TowerCLIError):
-    """An exception class for invalid values being sent as option
-    switches to Tower CLI.
-    """
-    exit_code = 64
-
-
-class CannotStartJob(TowerCLIError):
-    """An exception class for jobs that cannot be started within Tower
-    for whatever reason.
-    """
-    exit_code = 97
-
-
-class Timeout(TowerCLIError):
-    """An exception class for timeouts encountered within Tower CLI,
-    usually for monitoring.
-    """
-    exit_code = 98
-
-
-class JobFailure(TowerCLIError):
-    """An exception class for job failures that require error codes within
-    the Tower CLI.
-    """
-    exit_code = 99
-
-
-class ConnectionError(TowerCLIError):
-    """An exception class to bubble requests errors more nicely,
-    and communicate connection issues to the user.
-    """
-    exit_code = 120


Test patch:
diff --git a/tests/test_models_base.py b/tests/test_models_base.py
index 6f0e151..fef068a 100644
--- a/tests/test_models_base.py
+++ b/tests/test_models_base.py
@@ -90,8 +90,46 @@ class ResourceMetaTests(unittest.TestCase):
 
         # Establish that it has one of the options added to the
         # superclass list command.
-        self.assertTrue(any([i.name == 'query'
-                            for i in MyResource.list.__click_params__]))
+        self.assertEqual(MyResource.list.__click_params__, models.Resource.list.__click_params__)
+
+    def test_multiple_inheritance(self):
+        """
+        Establish that click decoration from all parent class chains are
+        preserved in a subclass.
+        """
+        class MyMixin(models.Resource):
+            endpoint = '/bogus/'
+
+            def list(self, **kwargs):
+                return super(MyMixin, self).list(**kwargs)
+
+        class MyResource(MyMixin, models.Resource):
+            endpoint = '/bogus/'
+
+            def list(self, **kwargs):
+                return super(MyResource, self).list(**kwargs)
+
+        self.assertTrue(hasattr(MyResource.list, '__click_params__'))
+        self.assertEqual(MyResource.list.__click_params__, models.Resource.list.__click_params__)
+
+    def test_no_duplicate_options_from_inheritance(self):
+        """
+        Test that metaclass does not duplicate options from multiple parents
+        """
+        class MyMixin1(models.Resource):
+            endpoint = '/bogus/'
+
+        class MyMixin2(models.Resource):
+            endpoint = '/boguser/'
+
+        class MyResource(MyMixin1, MyMixin2):
+            endpoint = '/boguser/'
+
+            def list(self, **kwargs):
+                return super(MyResource, self).list(**kwargs)
+
+        self.assertTrue(hasattr(MyResource.list, '__click_params__'))
+        self.assertEqual(MyResource.list.__click_params__, models.Resource.list.__click_params__)
 
     def test_fields(self):
         """Establish that fields are appropriately classified within