Repository: python-attrs/attrs
Pull request #951 (2022-04-11 19:16:15)

Complete error signature of _InValidator
# Summary

This makes the _InValidator produce the error signature it is proclaimed to have in the docstring of _in(). Similarly to the other validators, the attribute, conflicting factor (in this case self.options) and the value are now included when a ValueError is raised.


# Pull Request Check List

- [X] Added **tests** for changed code.
  Our CI fails if coverage is not 100%.
- [X] New features have been added to our [Hypothesis testing strategy](https://github.com/python-attrs/attrs/blob/main/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] If they've been added to `attr/__init__.pyi`, they've *also* been re-imported in `attrs/__init__.pyi`.
- [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).
          Find the appropriate next version in our [``__init__.py``](https://github.com/python-attrs/attrs/blob/main/src/attr/__init__.py) file.
- [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/main/changelog.d).
- [X] Consider granting [push permissions to the PR branch](https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/allowing-changes-to-a-pull-request-branch-created-from-a-fork), so maintainers can fix minor issues themselves without pestering you.

I hope it's ok that I checked all the boxes that don't apply as well (e.g. sufficient documentation is already there).

# Notes 

It may be advisable to only include a gist of the options because otherwise this change could negatively impact performance if _in() is called on a large collection (the error message would have to use that gist as well of course). 

My initial goal though was to make the ValueError do what the docstring says it does. One may want to catch a ValueError and present the user with a more "polished" (or in my case also translated) version of it. A standardized error signature can help with that, that's why _in() would now produce the same signature as e.g. matches_re() would. I think this was also the intention of the person that wrote the _in() docstring. Hope this is helpful, it's my first PR ever!


iron3oxide: Looks like an InternalServerError prevented the 3.9 tests from succeeding.. 
hynek: I’m afraid that would be a backwards-incompatible change. I also suspect the docs was a copy/paste mistake and the docs should be adapted to the behavior and not vice versa?
iron3oxide: In theory, adapting the docs to the actual behavior is the way to go, no question about it. In this case, I thought the docstring expressed the intended behavior and the method just never got "completed". 

What about adding a default argument like `verbose_value_error=False` to _in() and passing it to _InValidator? That way, the change should be backwards-compatible but people that need a more verbose error message could obtain it.
hynek: Agh ugh…that seems a lot of complexity for something that almost nobody would notice.

You know what? Let's treat it as a bug and fix it to conform to the documented behavior.

Make the newsfragment breaking and please use [semantic newlines](https://rhodesmill.org/brandon/2012/one-sentence-per-line/).
hynek: Agh ugh…that seems a lot of complexity for something that almost nobody would notice.

You know what? Let's treat it as a bug and fix it to conform to the documented behavior.

Make the newsfragment breaking and please use [semantic newlines](https://rhodesmill.org/brandon/2012/one-sentence-per-line/).
iron3oxide: Semantic newlines seem pretty neat, thanks for making me discover them! I actually realized that one can also obtain the values in question from the caught error, so I won't insist on merging this if the complexity it adds outweighs the benefits in your eyes.  
hynek: What I meant is that I'm willing to accept your original approach of adapting the behavior to the documentation since I don't expect a lot of breakage to make this complex/special case.

JFTR you've gone a bit overboard with semantic newlines…I'd call that "Poetic Newlines" 😅. We usually just add newlines after periods, colons and sometimes semicolons. :) (comma is OK, but middle of the sentence is not)
iron3oxide: That's probably a sane strategy for semantic newlines, I can see why breaks anywhere else might seem arbitrary. I tried to follow the style used in the article you linked, found [here](https://raw.githubusercontent.com/brandon-rhodes/blog/master/texts/brandon/2012/one-sentence-per-line.rst), where breaks in the middle of a sentence can occur at "breaks between clauses", but finding these breaks can of course be cumbersome, and deciding if a break occurs may even be subjective in some cases. 

Either way, I didn't quite understand the imperative of your reply, or to put it bluntly: what should I do next? 
iron3oxide: Lessons learned the hard way: git reset does not work how I thought it would (never needed to use it before) and manually reverting changes is in most cases better than using git revert. Sorry for the mess.

Patch:
diff --git a/changelog.d/951.change.rst b/changelog.d/951.change.rst
new file mode 100644
index 0000000..24303f6
--- /dev/null
+++ b/changelog.d/951.change.rst
@@ -0,0 +1,3 @@
+The error signature of _InValidator has been completed,
+it now includes the attribute,
+options and value as proclaimed in the docstring of _in().
diff --git a/src/attr/validators.py b/src/attr/validators.py
index 6a1e198..3a4e728 100644
--- a/src/attr/validators.py
+++ b/src/attr/validators.py
@@ -299,7 +299,10 @@ class _InValidator:
             raise ValueError(
                 "'{name}' must be in {options!r} (got {value!r})".format(
                     name=attr.name, options=self.options, value=value
-                )
+                ),
+                attr,
+                self.options,
+                value,
             )
 
     def __repr__(self):


Test patch:
diff --git a/tests/test_validators.py b/tests/test_validators.py
index f3fe69c..3bec62b 100644
--- a/tests/test_validators.py
+++ b/tests/test_validators.py
@@ -471,7 +471,12 @@ class TestIn_:
         a = simple_attr("test")
         with pytest.raises(ValueError) as e:
             v(None, a, None)
-        assert ("'test' must be in [1, 2, 3] (got None)",) == e.value.args
+        assert (
+            "'test' must be in [1, 2, 3] (got None)",
+            a,
+            [1, 2, 3],
+            None,
+        ) == e.value.args
 
     def test_fail_with_string(self):
         """
@@ -482,7 +487,12 @@ class TestIn_:
         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
+        assert (
+            "'test' must be in 'abc' (got None)",
+            a,
+            "abc",
+            None,
+        ) == e.value.args
 
     def test_repr(self):
         """