Allow converter.optional to take a Converter such as converter.pipe a…

…s its argument (#1372)

* Allow converter.optional to take a converter such as converter.pipe as its argument

* Only turn optional into a Converter if needed

* Move call to Converter constructor to the end of optional()

The constructor consumes __annotations__, so move the constructor call to after those have been set on the optional_converter function

* Update tests/test_converters.py

* Update tests/test_converters.py

---------

Co-authored-by: Hynek Schlawack <[email protected]>

changelog.d/1372.change.md

@@ -0,0 +1 @@
+`attrs.converters.optional()` works again when taking `attrs.converters.pipe()` or another Converter as its argument.

src/attr/converters.py

@@ -7,7 +7,7 @@
 import typing
 
 from ._compat import _AnnotationExtractor
-from ._make import NOTHING, Factory, pipe
+from ._make import NOTHING, Converter, Factory, pipe
 
 
 __all__ = [
@@ -33,10 +33,19 @@ def optional(converter):
     .. versionadded:: 17.1.0
     """
 
-    def optional_converter(val):
-        if val is None:
-            return None
-        return converter(val)
+    if isinstance(converter, Converter):
+
+        def optional_converter(val, inst, field):
+            if val is None:
+                return None
+            return converter(val, inst, field)
+
+    else:
+
+        def optional_converter(val):
+            if val is None:
+                return None
+            return converter(val)
 
     xtr = _AnnotationExtractor(converter)
 
@@ -48,6 +57,9 @@ def optional_converter(val):
     if rt:
         optional_converter.__annotations__["return"] = typing.Optional[rt]
 
+    if isinstance(converter, Converter):
+        return Converter(optional_converter, takes_self=True, takes_field=True)
+
     return optional_converter
 
 

tests/test_converters.py

@@ -143,6 +143,14 @@ def test_fail(self):
         with pytest.raises(ValueError):
             c("not_an_int")
 
+    def test_converter_instance(self):
+        """
+        Works when passed a Converter instance as argument.
+        """
+        c = optional(Converter(to_bool))
+
+        assert True is c("yes", None, None)
+
 
 class TestDefaultIfNone:
     def test_missing_default(self):
@@ -272,6 +280,48 @@ class C:
         )
 
 
+class TestOptionalPipe:
+    def test_optional(self):
+        """
+        Nothing happens if None.
+        """
+        c = optional(pipe(str, Converter(to_bool), bool))
+
+        assert None is c.converter(None, None, None)
+
+    def test_pipe(self):
+        """
+        A value is given, run it through all wrapped converters.
+        """
+        c = optional(pipe(str, Converter(to_bool), bool))
+
+        assert (
+            True
+            is c.converter("True", None, None)
+            is c.converter(True, None, None)
+        )
+
+    def test_instance(self):
+        """
+        Should work when set as an attrib.
+        """
+
+        @attr.s
+        class C:
+            x = attrib(
+                converter=optional(pipe(str, Converter(to_bool), bool)),
+                default=None,
+            )
+
+        c1 = C()
+
+        assert None is c1.x
+
+        c2 = C("True")
+
+        assert True is c2.x
+
+
 class TestToBool:
     def test_unhashable(self):
         """