Repository: python-attrs/attrs
Pull request #536 (2019-05-30 00:09:32)

Improve the error message informing the value that it got it
I would like to propose a change on the error message returned by the validator `is_callable()`.

Currently, only a message "TypeError: 'x' must be callable" it's informed to the user, I would like to add the value that was passed to it within the message.

This approach besides given more information to the user resembles the errors messages returned by others validators that also include the value informed =)

# Pull Request Check List

This is just a reminder about the most common mistakes.  Please make sure that you tick all *appropriate* boxes.  But please read our [contribution guide](https://www.attrs.org/en/latest/contributing.html) at least once, it will save you unnecessary review cycles!

If an item doesn't apply to your pull request, **check it anyway** to make it apparent that there's nothing to do.

- [X] Added **tests** for changed code.
- [X] New features have been added to our [Hypothesis testing strategy](https://github.com/python-attrs/attrs/blob/master/tests/strategies.py).
- [X] Changes or additions to public APIs are reflected in our type stubs (files ending in ``.pyi``).
    - [X] ...and used in the stub test file `tests/typing_example.py`.
- [X] Updated **documentation** for changed code.
    - [X] New functions/classes have to be added to `docs/api.rst` by hand.
    - [X] Changes to the signature of `@attr.s()` have to be added by hand too.
    - [X] Changed/added classes/methods/functions have appropriate `versionadded`, `versionchanged`, or `deprecated` [directives](http://www.sphinx-doc.org/en/stable/markup/para.html#directive-versionadded).
- [X] Documentation in `.rst` files is written using [semantic newlines](https://rhodesmill.org/brandon/2012/one-sentence-per-line/).
- [X] Changes (and possible deprecations) have news fragments in [`changelog.d`](https://github.com/python-attrs/attrs/blob/master/changelog.d).

If you have *any* questions to *any* of the points above, just **submit and ask**!  This checklist is here to *help* you, not to deter you from contributing!


hynek: Hm I wish it would work like that but I'm afraid that this is a breaking change. :(  Maybe make it an option?
williamjamir: Hi hynek, thanks for your time review it :)

> Hm I wish it would work like that but I'm afraid that this is a breaking change. :(
 
Oh sorry, I didn't consider that users would be relying/depending on the errors message raised by the validators.

> Maybe make it an option?

Sure, do you have anything in mind? I mean, where would I introduce this option?
wsanchez: If we want people to distinguish this `TypeError` from others without looking at string values, and the value of the thing-that-isn't-callable is useful, then that should all be made available without having to parse a string.

So perhaps a new exception type `NotCallableError`, subclassing `TypeError`, which has a `message` attribute, a `value` attribute, and make sure that it's `__str__` produces the same not-informative-enough message as before?
hynek: What @wsanchez said SGTM!
williamjamir: Hi @wsanchez, I not sure if I got all right.

> If we want people to distinguish this `TypeError` from others without looking at string values, and the value of the thing-that-isn't-callable is useful, then that should all be made available without having to parse a string.

Yep, I totally agree =) 

> So perhaps a new exception type `NotCallableError`, subclassing `TypeError`, which has a `message` attribute, a `value` attribute, and make sure that it's `__str__` produces the same not-informative-enough message as before?

Right, I agree with adding this new exception (and I can add in the next commit without any problem), but the "original issue" that I brought was not solved, the message still not-informative and doesn't resemble the errors messages returned by others validators.

So just to make sure that I got right, we are going to keep with the not-informative-enough message in order to avoid breaking changes right?

wsanchez: Yeah, I think @hynek is saying that the current string can't change for compatibility, so adding a new type that at least can get you the data you need is an improvement.

I agree that the default string could be better, and I'm less sympathetic to compatibility with code that's inspecting the string, but `attrs` has a high compatibility bar.  (I'm also not sure where an option to enable an improved message would go.)
hynek: Firstly, having a dedicated class with a `got` field (do have have precedents on how to call fields like that?) allows you to write tests that don't break on changes. I guess I can live with changing the string in the end however if we do it, we should also add a good way to introspect the error.
williamjamir: Is there any further change in order to merge this pull request? 

Patch:
diff --git a/changelog.d/536.change.rst b/changelog.d/536.change.rst
new file mode 100644
index 0000000..71f96a3
--- /dev/null
+++ b/changelog.d/536.change.rst
@@ -0,0 +1,2 @@
+``attr.validators.is_callable()`` validator now raises an exception ``attr.exceptions.NotCallableError``, a subclass of ``TypeError``, informing the received value.
+`#536 <https://github.com/python-attrs/attrs/pull/536>`_.
diff --git a/docs/api.rst b/docs/api.rst
index c6cd105..1e102a8 100644
--- a/docs/api.rst
+++ b/docs/api.rst
@@ -397,7 +397,7 @@ Validators
         >>> C("not a callable")
         Traceback (most recent call last):
             ...
-        TypeError: 'x' must be callable
+        attr.exceptions.NotCallableError: 'x' must be callable (got 'not a callable' that is a <class 'str'>).
 
 
 .. autofunction:: attr.validators.deep_iterable
diff --git a/src/attr/exceptions.py b/src/attr/exceptions.py
index b12e41e..1fa4e47 100644
--- a/src/attr/exceptions.py
+++ b/src/attr/exceptions.py
@@ -55,3 +55,20 @@ class PythonTooOldError(RuntimeError):
 
     .. versionadded:: 18.2.0
     """
+
+
+class NotCallableError(TypeError):
+    """
+    A ``attr.ib()`` requiring a callable has been set with a value
+    that is not callable.
+
+    .. versionadded:: 19.2.0
+    """
+
+    def __init__(self, msg, value):
+        super(TypeError, self).__init__(msg, value)
+        self.msg = msg
+        self.value = value
+
+    def __str__(self):
+        return str(self.msg)
diff --git a/src/attr/exceptions.pyi b/src/attr/exceptions.pyi
index 48fffcc..8d6bd39 100644
--- a/src/attr/exceptions.pyi
+++ b/src/attr/exceptions.pyi
@@ -1,3 +1,5 @@
+from typing import Any
+
 class FrozenInstanceError(AttributeError):
     msg: str = ...
 
@@ -5,3 +7,8 @@ class AttrsAttributeNotFoundError(ValueError): ...
 class NotAnAttrsClassError(ValueError): ...
 class DefaultAlreadySetError(RuntimeError): ...
 class UnannotatedAttributeError(RuntimeError): ...
+
+class NotCallableError(TypeError):
+    msg: str = ...
+    value: Any = ...
+    def __init__(self, msg: str, value: Any) -> None: ...
diff --git a/src/attr/validators.py b/src/attr/validators.py
index 9c39dd2..ed3f22a 100644
--- a/src/attr/validators.py
+++ b/src/attr/validators.py
@@ -5,6 +5,7 @@ Commonly useful validators.
 from __future__ import absolute_import, division, print_function
 
 from ._make import _AndValidator, and_, attrib, attrs
+from .exceptions import NotCallableError
 
 
 __all__ = [
@@ -186,7 +187,16 @@ class _IsCallableValidator(object):
         We use a callable class to be able to change the ``__repr__``.
         """
         if not callable(value):
-            raise TypeError("'{name}' must be callable".format(name=attr.name))
+            message = (
+                "'{name}' must be callable "
+                "(got {value!r} that is a {actual!r})."
+            )
+            raise NotCallableError(
+                msg=message.format(
+                    name=attr.name, value=value, actual=value.__class__
+                ),
+                value=value,
+            )
 
     def __repr__(self):
         return "<is_callable validator>"
@@ -194,13 +204,15 @@ class _IsCallableValidator(object):
 
 def is_callable():
     """
-    A validator that raises a :class:`TypeError` if the initializer is called
-    with a value for this particular attribute that is not callable.
+    A validator that raises a :class:`attr.exceptions.NotCallableError`
+    if the initializer is called with a value for this particular attribute
+    that is not callable.
 
     .. versionadded:: 19.1.0
 
-    :raises TypeError: With a human readable error message containing the
-        attribute (of type :class:`attr.Attribute`) name.
+    :raises `attr.exceptions.NotCallableError`: With a human readable error
+        message containing the attribute (:class:`attr.Attribute`) name,
+        and the value it got.
     """
     return _IsCallableValidator()
 


Test patch:
diff --git a/tests/test_validators.py b/tests/test_validators.py
index 30bf672..eb87867 100644
--- a/tests/test_validators.py
+++ b/tests/test_validators.py
@@ -354,8 +354,15 @@ class TestDeepIterable(object):
         """
         with pytest.raises(TypeError) as e:
             deep_iterable(member_validator, iterable_validator)
+        value = 42
+        message = "must be callable (got {value} that is a {type_}).".format(
+            value=value, type_=value.__class__
+        )
 
-        e.match(r"\w* must be callable")
+        assert message in e.value.args[0]
+        assert value == e.value.args[1]
+        assert message in e.value.msg
+        assert value == e.value.value
 
     def test_fail_invalid_member(self):
         """
@@ -466,7 +473,15 @@ class TestDeepMapping(object):
         with pytest.raises(TypeError) as e:
             deep_mapping(key_validator, value_validator, mapping_validator)
 
-        e.match(r"\w* must be callable")
+        value = 42
+        message = "must be callable (got {value} that is a {type_}).".format(
+            value=value, type_=value.__class__
+        )
+
+        assert message in e.value.args[0]
+        assert value == e.value.args[1]
+        assert message in e.value.msg
+        assert value == e.value.value
 
     def test_fail_invalid_mapping(self):
         """
@@ -550,7 +565,14 @@ class TestIsCallable(object):
         with pytest.raises(TypeError) as e:
             v(None, a, None)
 
-        e.match("'test' must be callable")
+        value = None
+        message = "'test' must be callable (got {value} that is a {type_})."
+        expected_message = message.format(value=value, type_=value.__class__)
+
+        assert expected_message == e.value.args[0]
+        assert value == e.value.args[1]
+        assert expected_message == e.value.msg
+        assert value == e.value.value
 
     def test_repr(self):
         """
@@ -559,6 +581,15 @@ class TestIsCallable(object):
         v = is_callable()
         assert "<is_callable validator>" == repr(v)
 
+    def test_exception_repr(self):
+        """
+        Verify that NotCallableError exception has a useful `__str__`.
+        """
+        from attr.exceptions import NotCallableError
+
+        instance = NotCallableError(msg="Some Message", value=42)
+        assert "Some Message" == str(instance)
+
 
 def test_hashability():
     """