Repository: python-attrs/attrs

Problem statement:
Poor error message/type for validators.in_ with a string
```python
import attr

@attr.s
class C:
   s = attr.ib(validator=attr.validators.in_('abc'))

C(s=1)  # TypeError: 'in <string>' requires string as left operand, not int
```

`__contains__` behaves a little weirdly for strings, but I still think this error could be improved.

Obvious options would be to catch exceptions and re-raise something with a clearer error message, and/or to deprecate use of strings as the collection here.

Found in attrs=18.1.0 while working on HypothesisWorks/hypothesis#954.


Hints:
What is the actual problem here? I don't think it's fair to expect in_ to also type check?

I think you want

```python
s = attr.ib(validator=[attr.validators.instance_of(str), attr.validators.in_('abc')])
```

?
I think the request is for something like:

```python
    def __call__(self, inst, attr, value):
        if value not in self.options:
            try:
                raise ValueError(
                    "'{name}' must be in {options!r} (got {value!r})"
                    .format(name=attr.name, options=self.options, value=value)
                )
            except Exception as e:
                raise AssertionError(
                    "in_ validator for attribute {attr} raised exception for value {value}: {error}"
                    .format(attr=attr, value=value, error=e)
                )
```

Almost - the idea is that if `value in self.options` raises an exception instead of returning a bool, we should still raise a ValueError (not e.g. TypeError), and preferably one with a useful and consistent message.

I'd write this as follows, and would be happy to open a PR if that would be helpful 😄 

```python
def __call__(self, inst, attr, value):
	try:
		if value not in self.options:
			raise ValueError(
				"'{name}' must be in {options!r} (got {value!r})"
				.format(name=attr.name, options=self.options, value=value)
			)
	except Exception:
		raise ValueError(
			"'{name}' must be in {options!r} "
			"(got {value!r}, which caused an internal error)"
			.format(name=attr.name, options=self.options, value=value)
		)
```
The contract for validators isn't super clear in the docs, but I don't see anything that says that `ValueError` is the only acceptable sort of exception to raise.  That probably should be clarified, but I would think `TypeError` is a valid exception to raise in the case where data of the wrong type is given, as in this example.

I do agree that the a better error message would be useful, but I'd definitely include the original exception text.
Uh yeah there is absolutely no contract about what kind of exceptions are raised by validators and I’d even argue that changing it could be backward incompatible. 🤔
I think it might make some sense for us to advise that certain exceptions are used, but I wouldn't go farther than that.  Otherwise, every validator would have to wrap a try/except around it's body, which could be a best practice, but seems like a lame thing to require.

Which leaves us with: should we do better than the above error message.  I notice I wrapped the try/except in my example around the wrong code (oops).

Lemme try again with a different suggestion:

```python
    def __call__(self, inst, attr, value):
        try:
            in_options = value in self.options
        except TypeError as e:
            in_options = False

        if not in_options:
            raise ValueError(
                "'{name}' must be in {options!r} (got {value!r})"
                .format(name=attr.name, options=self.options, value=value)
            )
```

I don't think trying to catch all exceptions is necessarily more correct, but we can definitely say that a non-string object isn't in the provide bucket (that happens to be a string) of options.  Since that bucket isn't required to contain objects of a homogenous type, I returning `False` is more appropriate (or consistent with, say a `list`) than raising a `TypeError`.

I guess I would accept a PR in this case.

Patch:
diff --git a/src/attr/validators.py b/src/attr/validators.py
--- a/src/attr/validators.py
+++ b/src/attr/validators.py
@@ -135,7 +135,12 @@ class _InValidator(object):
     options = attrib()
 
     def __call__(self, inst, attr, value):
-        if value not in self.options:
+        try:
+            in_options = value in self.options
+        except TypeError as e:  # e.g. `1 in "abc"`
+            in_options = False
+
+        if not in_options:
             raise ValueError(
                 "'{name}' must be in {options!r} (got {value!r})"
                 .format(name=attr.name, options=self.options, value=value)


Test patch:
diff --git a/tests/test_validators.py b/tests/test_validators.py
--- a/tests/test_validators.py
+++ b/tests/test_validators.py
@@ -243,6 +243,19 @@ def test_fail(self):
             "'test' must be in [1, 2, 3] (got None)",
         ) == e.value.args
 
+    def test_fail_with_string(self):
+        """
+        Raise ValueError if the value is outside our options when the
+        options are specified as a string and the value is not a string.
+        """
+        v = in_("abc")
+        a = simple_attr("test")
+        with pytest.raises(ValueError) as e:
+            v(None, a, None)
+        assert (
+            "'test' must be in 'abc' (got None)",
+        ) == e.value.args
+
     def test_repr(self):
         """
         Returned validator has a useful `__repr__`.