Simplify the string formatting rules for the Invalid exceptions.

As per review.

Also include the actual failing implementation object in the BrokenMethodImplementation to make it easier to track down what's going on when inheritance is involved.

3 files changed, 221 insertions, 77 deletions

diff --git a/src/zope/interface/exceptions.py b/src/zope/interface/exceptions.py
index 86caf3f..2f3758b 100644
--- a/src/zope/interface/exceptions.py
+++ b/src/zope/interface/exceptions.py
@@ -30,38 +30,94 @@ class Invalid(Exception):
     """A specification is violated
     """
 
-_NotGiven = '<Not Given>'
 
-class _TargetMixin(object):
-    target = _NotGiven
-    interface = None
+class _TargetInvalid(Invalid):
+    # Internal use. Subclass this when you're describing
+    # a particular target object that's invalid according
+    # to a specific interface.
+    #
+    # For backwards compatibility, the *target* and *interface* are
+    # optional, and the signatures are inconsistent in their ordering.
+    #
+    # We deal with the inconsistency in ordering by defining the index
+    # of the two values in ``self.args``. *target* uses a marker object to
+    # distinguish "not given" from "given, but None", because the latter
+    # can be a value that gets passed to validation. For this reason, it must
+    # always be the last argument (we detect absense by the ``IndexError``).
+
+    _IX_INTERFACE = 0
+    _IX_TARGET = 1
+    # The exception to catch when indexing self.args indicating that
+    # an argument was not given. If all arguments are expected,
+    # a subclass should set this to ().
+    _NOT_GIVEN_CATCH = IndexError
+    _NOT_GIVEN = '<Not Given>'
+
+    def _get_arg_or_default(self, ix, default=None):
+        try:
+            return self.args[ix] # pylint:disable=unsubscriptable-object
+        except self._NOT_GIVEN_CATCH:
+            return default
 
     @property
-    def _target_prefix(self):
-        if self.target is _NotGiven:
-            return "An object"
-        return "The object %r" % (self.target,)
+    def interface(self):
+        return self._get_arg_or_default(self._IX_INTERFACE)
 
-    _trailer = '.'
+    @property
+    def target(self):
+        return self._get_arg_or_default(self._IX_TARGET, self._NOT_GIVEN)
+
+    ###
+    # str
+    #
+    # The ``__str__`` of self is implemented by concatenating (%s), in order,
+    # these properties (none of which should have leading or trailing
+    # whitespace):
+    #
+    # - self._str_subject
+    #   Begin the message, including a description of the target.
+    # - self._str_description
+    #   Provide a general description of the type of error, including
+    #   the interface name if possible and relevant.
+    # - self._str_conjunction
+    #   Join the description to the details. Defaults to ": ".
+    # - self._str_details
+    #   Provide details about how this particular instance of the error.
+    # - self._str_trailer
+    #   End the message. Usually just a period.
+    ###
 
     @property
-    def _general_description(self):
-        return "has failed to implement interface %s:" % (
-            self.interface
-        ) if self.interface is not None else ''
+    def _str_subject(self):
+        target = self.target
+        if target is self._NOT_GIVEN:
+            return "An object"
+        return "The object %r" % (target,)
 
+    @property
+    def _str_description(self):
+        return "has failed to implement interface %s" % (
+            self.interface or '<Unknown>'
+        )
+
+    _str_conjunction = ": "
+    _str_details = "<unknown>"
+    _str_trailer = '.'
 
     def __str__(self):
-        return "%s %s%s%s" % (
-            self._target_prefix,
-            self._general_description,
-            self._specifics,
-            self._trailer
+        return "%s %s%s%s%s" % (
+            self._str_subject,
+            self._str_description,
+            self._str_conjunction,
+            self._str_details,
+            self._str_trailer
         )
 
 
-class DoesNotImplement(_TargetMixin, Invalid):
+class DoesNotImplement(_TargetInvalid):
     """
+    DoesNotImplement(interface[, target])
+
     The *target* (optional) does not implement the *interface*.
 
     .. versionchanged:: 5.0.0
@@ -69,19 +125,13 @@ class DoesNotImplement(_TargetMixin, Invalid):
        string value of this object accordingly.
     """
 
-    def __init__(self, interface, target=_NotGiven):
-        Invalid.__init__(self, interface, target)
-        self.interface = interface
-        self.target = target
+    _str_details = "Does not declaratively implement the interface"
 
-    _general_description = "does not implement the interface"
 
-    @property
-    def _specifics(self):
-        return ' ' + str(self.interface)
-
-class BrokenImplementation(_TargetMixin, Invalid):
+class BrokenImplementation(_TargetInvalid):
     """
+    BrokenImplementation(interface, name[, target])
+
     The *target* (optional) is missing the attribute *name*.
 
     .. versionchanged:: 5.0.0
@@ -91,48 +141,98 @@ class BrokenImplementation(_TargetMixin, Invalid):
        The *name* can either be a simple string or a ``Attribute`` object.
     """
 
-    def __init__(self, interface, name, target=_NotGiven):
-        Invalid.__init__(self, interface, name, target)
-        self.interface = interface
-        self.name = name
-        self.target = target
+    _IX_NAME = _TargetInvalid._IX_INTERFACE + 1
+    _IX_TARGET = _IX_NAME + 1
 
+    @property
+    def name(self):
+        return self.args[1] # pylint:disable=unsubscriptable-object
 
     @property
-    def _specifics(self):
-        return " The %s attribute was not provided" % (
+    def _str_details(self):
+        return "The %s attribute was not provided" % (
             repr(self.name) if isinstance(self.name, str) else self.name
         )
 
-class BrokenMethodImplementation(_TargetMixin, Invalid):
+
+class BrokenMethodImplementation(_TargetInvalid):
     """
-    The *target* (optional) has a *method* that violates
+    BrokenMethodImplementation(method, message[, implementation, interface, target])
+
+    The *target* (optional) has a *method* in *implementation* that violates
     its contract in a way described by *mess*.
 
     .. versionchanged:: 5.0.0
-       Add the *target* argument and attribute, and change the resulting
-       string value of this object accordingly.
+       Add the *interface* and *target* argument and attribute,
+       and change the resulting string value of this object accordingly.
 
        The *method* can either be a simple string or a ``Method`` object.
+
+    .. versionchanged:: 5.0.0
+       If *implementation* is given, then the *message* will have the
+       string "implementation" replaced with an short but informative
+       representation of *implementation*.
+
     """
 
-    def __init__(self, method, mess, target=_NotGiven):
-        Invalid.__init__(self, method, mess, target)
-        self.method = method
-        self.mess = mess
-        self.target = target
+    _IX_IMPL = 2
+    _IX_INTERFACE = _IX_IMPL + 1
+    _IX_TARGET = _IX_INTERFACE + 1
+
+    @property
+    def method(self):
+        return self.args[0] # pylint:disable=unsubscriptable-object
+
+    @property
+    def mess(self):
+        return self.args[1] # pylint:disable=unsubscriptable-object
+
+    @staticmethod
+    def __implementation_str(impl):
+        # It could be a callable or some arbitrary object, we don't
+        # know yet.
+        import inspect # Inspect is a heavy-weight dependency, lots of imports
+        try:
+            sig = inspect.signature
+            formatsig = str
+        except AttributeError:
+            sig = inspect.getargspec
+            f = inspect.formatargspec
+            formatsig = lambda sig: f(*sig) # pylint:disable=deprecated-method
+
+        try:
+            sig = sig(impl)
+        except (ValueError, TypeError):
+            # Unable to introspect. Darn.
+            # This could be a non-callable, or a particular builtin,
+            # or a bound method that doesn't even accept 'self', e.g.,
+            # ``Class.method = lambda: None; Class().method``
+            return repr(impl)
+
+        try:
+            name = impl.__qualname__
+        except AttributeError:
+            name = impl.__name__
+
+        return name + formatsig(sig)
 
     @property
-    def _specifics(self):
-        return 'violates the contract of %s because %s' % (
+    def _str_details(self):
+        impl = self._get_arg_or_default(self._IX_IMPL, self._NOT_GIVEN)
+        message = self.mess
+        if impl is not self._NOT_GIVEN and 'implementation' in message:
+            message = message.replace("implementation", '%r')
+            message = message % (self.__implementation_str(impl),)
+
+        return 'The contract of %s is violated because %s' % (
             repr(self.method) if isinstance(self.method, str) else self.method,
-            self.mess,
+            message,
         )
 
 
-class MultipleInvalid(_TargetMixin, Invalid):
+class MultipleInvalid(_TargetInvalid):
     """
-    The *target* has failed to implement the *iface* in
+    The *target* has failed to implement the *interface* in
     multiple ways.
 
     The failures are described by *exceptions*, a collection of
@@ -141,23 +241,26 @@ class MultipleInvalid(_TargetMixin, Invalid):
     .. versionadded:: 5.0
     """
 
-    def __init__(self, iface, target, exceptions):
-        exceptions = list(exceptions)
-        Invalid.__init__(self, iface, target, exceptions)
-        self.target = target
-        self.interface = iface
-        self.exceptions = exceptions
+    _NOT_GIVEN_CATCH = ()
+
+    def __init__(self, interface, target, exceptions):
+        super(MultipleInvalid, self).__init__(interface, target, tuple(exceptions))
+
+    @property
+    def exceptions(self):
+        return self.args[2] # pylint:disable=unsubscriptable-object
 
     @property
-    def _specifics(self):
+    def _str_details(self):
         # It would be nice to use tabs here, but that
         # is hard to represent in doctests.
         return '\n    ' + '\n    '.join(
-            x._specifics.strip() if isinstance(x, _TargetMixin) else(str(x))
+            x._str_details.strip() if isinstance(x, _TargetInvalid) else str(x)
             for x in self.exceptions
         )
 
-    _trailer = ''
+    _str_conjunction = ':' # We don't want a trailing space, messes up doctests
+    _str_trailer = ''
 
 
 class InvalidInterface(Exception):
diff --git a/src/zope/interface/tests/test_exceptions.py b/src/zope/interface/tests/test_exceptions.py
index 42ea7eb..a55f522 100644
--- a/src/zope/interface/tests/test_exceptions.py
+++ b/src/zope/interface/tests/test_exceptions.py
@@ -35,15 +35,19 @@ class DoesNotImplementTests(unittest.TestCase):
         dni = self._makeOne()
         self.assertEqual(
             str(dni),
-            'An object does not implement the interface '
-            '<InterfaceClass zope.interface.tests.test_exceptions.IDummy>.')
+            "An object has failed to implement interface "
+            "<InterfaceClass zope.interface.tests.test_exceptions.IDummy>: "
+            "Does not declaratively implement the interface."
+        )
 
     def test___str__w_candidate(self):
         dni = self._makeOne('candidate')
         self.assertEqual(
             str(dni),
-            'The object \'candidate\' does not implement the interface '
-            '<InterfaceClass zope.interface.tests.test_exceptions.IDummy>.')
+            "The object 'candidate' has failed to implement interface "
+            "<InterfaceClass zope.interface.tests.test_exceptions.IDummy>: "
+            "Does not declaratively implement the interface."
+        )
 
 
 class BrokenImplementationTests(unittest.TestCase):
@@ -72,32 +76,68 @@ class BrokenImplementationTests(unittest.TestCase):
             '<InterfaceClass zope.interface.tests.test_exceptions.IDummy>: '
             "The 'missing' attribute was not provided.")
 
+
+def broken_function():
+    """
+    This is a global function with a simple argument list.
+
+    It exists to be able to report the same information when
+    formatting signatures under Python 2 and Python 3.
+    """
+
+
 class BrokenMethodImplementationTests(unittest.TestCase):
 
     def _getTargetClass(self):
         from zope.interface.exceptions import BrokenMethodImplementation
         return BrokenMethodImplementation
 
+    message = 'I said so'
+
     def _makeOne(self, *args):
-        return self._getTargetClass()('aMethod', 'I said so', *args)
+        return self._getTargetClass()('aMethod', self.message, *args)
 
     def test___str__(self):
         dni = self._makeOne()
         self.assertEqual(
             str(dni),
-            "An object violates the contract of 'aMethod' because I said so.")
+            "An object has failed to implement interface <Unknown>: "
+            "The contract of 'aMethod' is violated because I said so."
+        )
 
-    def test___str__w_candidate(self):
-        dni = self._makeOne('candidate')
+    def test___str__w_candidate_no_implementation(self):
+        dni = self._makeOne('some_function', '<IFoo>', 'candidate')
         self.assertEqual(
             str(dni),
-            "The object 'candidate' violates the contract of 'aMethod' because I said so.")
+            "The object 'candidate' has failed to implement interface <IFoo>: "
+            "The contract of 'aMethod' is violated because I said so."
+        )
+
+    def test___str__w_candidate_w_implementation(self):
+        self.message = 'implementation is wonky'
+        dni = self._makeOne(broken_function, '<IFoo>', 'candidate')
+        self.assertEqual(
+            str(dni),
+            "The object 'candidate' has failed to implement interface <IFoo>: "
+            "The contract of 'aMethod' is violated because "
+            "'broken_function()' is wonky."
+        )
+
+    def test___str__w_candidate_w_implementation_not_callable(self):
+        self.message = 'implementation is not callable'
+        dni = self._makeOne(42, '<IFoo>', 'candidate')
+        self.assertEqual(
+            str(dni),
+            "The object 'candidate' has failed to implement interface <IFoo>: "
+            "The contract of 'aMethod' is violated because "
+            "'42' is not callable."
+        )
 
     def test___repr__w_candidate(self):
-        dni = self._makeOne('candidate')
+        dni = self._makeOne(None, 'candidate')
         self.assertEqual(
             repr(dni),
-            "BrokenMethodImplementation('aMethod', 'I said so', 'candidate')"
+            "BrokenMethodImplementation('aMethod', 'I said so', None, 'candidate')"
         )
 
 
@@ -122,7 +162,7 @@ class MultipleInvalidTests(unittest.TestCase):
             str(dni),
             "The object 'target' has failed to implement interface "
             "<InterfaceClass zope.interface.tests.test_exceptions.IDummy>:\n"
-            "    violates the contract of 'aMethod' because I said so\n"
+            "    The contract of 'aMethod' is violated because I said so\n"
             "    Regular exception"
         )
 
@@ -139,6 +179,6 @@ class MultipleInvalidTests(unittest.TestCase):
             repr(dni),
             "MultipleInvalid(<InterfaceClass zope.interface.tests.test_exceptions.IDummy>,"
             " 'target',"
-            " [BrokenMethodImplementation('aMethod', 'I said so', '<Not Given>'),"
-            " Exception('Regular', 'exception')])"
+            " (BrokenMethodImplementation('aMethod', 'I said so'),"
+            " Exception('Regular', 'exception')))"
         )
diff --git a/src/zope/interface/verify.py b/src/zope/interface/verify.py
index a9b80a7..0a64aeb 100644
--- a/src/zope/interface/verify.py
+++ b/src/zope/interface/verify.py
@@ -78,7 +78,7 @@ def _verify(iface, candidate, tentative=False, vtype=None):
 
     excs = []
     if not tentative and not tester(candidate):
-        excs.append(DoesNotImplement(iface))
+        excs.append(DoesNotImplement(iface, candidate))
 
     for name, desc in iface.namesAndDescriptions(all=True):
         try:
@@ -102,7 +102,7 @@ def _verify_element(iface, name, desc, candidate, vtype):
             # We can't verify non-methods on classes, since the
             # class may provide attrs in it's __init__.
             return
-
+        # TODO: On Python 3, this should use ``raise...from``
         raise BrokenImplementation(iface, desc, candidate)
 
     if not isinstance(desc, Method):
@@ -146,7 +146,8 @@ def _verify_element(iface, name, desc, candidate, vtype):
 
     else:
         if not callable(attr):
-            raise BrokenMethodImplementation(desc, "implementation is not a method", candidate)
+            raise BrokenMethodImplementation(desc, "implementation is not a method",
+                                             attr, iface, candidate)
         # sigh, it's callable, but we don't know how to introspect it, so
         # we have to give it a pass.
         return
@@ -157,7 +158,7 @@ def _verify_element(iface, name, desc, candidate, vtype):
     if mess:
         if PYPY2 and _pypy2_false_positive(mess, candidate, vtype):
             return
-        raise BrokenMethodImplementation(desc, mess, candidate)
+        raise BrokenMethodImplementation(desc, mess, attr, iface, candidate)