Repository: python-attrs/attrs
Pull request #414 (2018-07-28 13:19:23)

Implement attr.converters.default_if_none
So this is a stab at #400 which I find a reasonable usage of converters.

Typing tests fail tho and they also fail for `attr.validators.optional`. Am I right to suspect that this has to be fixed in mypy's plugin? Gives us a bit of a Mexican stand-off, doesn't it? It’s completely possible that I additionally got the stubs wrong so feel free to advice.

cc @euresti @chadrik

euresti: Yeah this is something the plugin currently doesn't support:

`error: Unsupported converter, only named functions and types are currently supported`

The underlying problem is that it's hard to extract the type of the function call in the mypy pipeline.

A workaround could be to write:
```
optional_int = optional(int)

@attr.s
class ConvCOptional:
    x: Optional[int] = attr.ib(converter=optional_int)
```

But that's broken also, but I think easier to fix.  Now I remember, in our work code, when we started using mypy I removed all our uses of optional and just wrote our own converters.
hynek: I guess until mypy opens up it's plugin API (I talked to Jukka here at EuroPython and it's legit on their roadmap) we’ll have to comment stuff out, wait for mypy to release the support? Having hard-coded support is “fine” for now…

***

Does the signature look correct to you tho? If someone rubber-stamps it, I’ll just comment it out and we open an issue on mypy.
euresti: I can review the signature.  As for commenting it out I'm not sure since I'm not sure when we'll fix the plugin.  It might be a while.  
hynek: Just to be clear: by special casing I mean that mypy only supports our shipped converters for now. Would that be a significant amount of work too?
euresti: Hmm.  Maybe I could detect the converters by name and handle them specially.  That might actually work.
hynek: > Hmm. Maybe I could detect the converters by name and handle them specially. That might actually work.

Yeah, that’s what I was thinking.

Patch:
diff --git a/.github/CONTRIBUTING.rst b/.github/CONTRIBUTING.rst
index 132c6d1..1927f8a 100644
--- a/.github/CONTRIBUTING.rst
+++ b/.github/CONTRIBUTING.rst
@@ -23,6 +23,8 @@ Workflow
 - No contribution is too small!
   Please submit as many fixes for typos and grammar bloopers as you can!
 - Try to limit each pull request to *one* change only.
+- Since we squash on merge, it's up to you how you handle updates to the master branch.
+  Whether you prefer to rebase on master or merge master into your branch, do whatever is more comfortable for you.
 - *Always* add tests and docs for your code.
   This is a hard rule; patches with missing tests or documentation can't be merged.
 - Make sure your changes pass our CI_.
diff --git a/changelog.d/400.change.rst b/changelog.d/400.change.rst
new file mode 100644
index 0000000..86dfbb7
--- /dev/null
+++ b/changelog.d/400.change.rst
@@ -0,0 +1,2 @@
+Added ``attr.converters.default_if_none()`` that allows to replace ``None`` values in attributes.
+For example ``attr.ib(converter=default_if_none(""))`` replaces ``None`` by empty strings.
diff --git a/changelog.d/414.change.rst b/changelog.d/414.change.rst
new file mode 100644
index 0000000..86dfbb7
--- /dev/null
+++ b/changelog.d/414.change.rst
@@ -0,0 +1,2 @@
+Added ``attr.converters.default_if_none()`` that allows to replace ``None`` values in attributes.
+For example ``attr.ib(converter=default_if_none(""))`` replaces ``None`` by empty strings.
diff --git a/docs/api.rst b/docs/api.rst
index 3ba2360..665fe9f 100644
--- a/docs/api.rst
+++ b/docs/api.rst
@@ -387,6 +387,21 @@ Converters
       C(x=42)
 
 
+.. autofunction:: attr.converters.default_if_none
+
+   For example:
+
+   .. doctest::
+
+      >>> @attr.s
+      ... class C(object):
+      ...     x = attr.ib(
+      ...         converter=attr.converters.default_if_none("")
+      ...     )
+      >>> C(None)
+      C(x='')
+
+
 Deprecated APIs
 ---------------
 
diff --git a/src/attr/converters.py b/src/attr/converters.py
index 3b3bac9..37c4a07 100644
--- a/src/attr/converters.py
+++ b/src/attr/converters.py
@@ -4,6 +4,8 @@ Commonly useful converters.
 
 from __future__ import absolute_import, division, print_function
 
+from ._make import NOTHING, Factory
+
 
 def optional(converter):
     """
@@ -13,7 +15,7 @@ def optional(converter):
     :param callable converter: the converter that is used for non-``None``
         values.
 
-    ..  versionadded:: 17.1.0
+    .. versionadded:: 17.1.0
     """
 
     def optional_converter(val):
@@ -22,3 +24,55 @@ def optional(converter):
         return converter(val)
 
     return optional_converter
+
+
+def default_if_none(default=NOTHING, factory=None):
+    """
+    A converter that allows to replace ``None`` values by *default* or the
+    result of *factory*.
+
+    :param default: Value to be used if ``None`` is passed. Passing an instance
+       of :class:`attr.Factory` is supported, however the ``takes_self`` option
+       is *not*.
+    :param callable factory: A callable that takes not parameters whose result
+       is used if ``None`` is passed.
+
+    :raises TypeError: If **neither** *default* or *factory* is passed.
+    :raises TypeError: If **both** *default* and *factory* are passed.
+    :raises ValueError: If an instance of :class:`attr.Factory` is passed with
+       ``takes_self=True``.
+
+    .. versionadded:: 18.2.0
+    """
+    if default is NOTHING and factory is None:
+        raise TypeError("Must pass either `default` or `factory`.")
+
+    if default is not NOTHING and factory is not None:
+        raise TypeError(
+            "Must pass either `default` or `factory` but not both."
+        )
+
+    if factory is not None:
+        default = Factory(factory)
+
+    if isinstance(default, Factory):
+        if default.takes_self:
+            raise ValueError(
+                "`takes_self` is not supported by default_if_none."
+            )
+
+        def default_if_none_converter(val):
+            if val is not None:
+                return val
+
+            return default.factory()
+
+    else:
+
+        def default_if_none_converter(val):
+            if val is not None:
+                return val
+
+            return default
+
+    return default_if_none_converter
diff --git a/src/attr/converters.pyi b/src/attr/converters.pyi
index bdf7a0c..4c5cac4 100644
--- a/src/attr/converters.pyi
+++ b/src/attr/converters.pyi
@@ -1,4 +1,4 @@
-from typing import TypeVar, Optional
+from typing import TypeVar, Optional, Callable
 from . import _ConverterType
 
 _T = TypeVar("_T")
@@ -6,3 +6,6 @@ _T = TypeVar("_T")
 def optional(
     converter: _ConverterType[_T]
 ) -> _ConverterType[Optional[_T]]: ...
+def default_if_none(
+    default: _T = ..., factory: Callable[[], _T] = ...
+) -> _ConverterType[_T]: ...


Test patch:
diff --git a/tests/test_converters.py b/tests/test_converters.py
index 0f92bc3..73e180a 100644
--- a/tests/test_converters.py
+++ b/tests/test_converters.py
@@ -6,7 +6,8 @@ from __future__ import absolute_import
 
 import pytest
 
-from attr.converters import optional
+from attr import Factory
+from attr.converters import default_if_none, optional
 
 
 class TestOptional(object):
@@ -19,6 +20,7 @@ class TestOptional(object):
         Wrapped converter is used as usual if value is not None.
         """
         c = optional(int)
+
         assert c("42") == 42
 
     def test_success_with_none(self):
@@ -26,6 +28,7 @@ class TestOptional(object):
         Nothing happens if None.
         """
         c = optional(int)
+
         assert c(None) is None
 
     def test_fail(self):
@@ -33,5 +36,62 @@ class TestOptional(object):
         Propagates the underlying conversion error when conversion fails.
         """
         c = optional(int)
+
         with pytest.raises(ValueError):
             c("not_an_int")
+
+
+class TestDefaultIfNone(object):
+    def test_missing_default(self):
+        """
+        Raises TypeError if neither default nor factory have been passed.
+        """
+        with pytest.raises(TypeError, match="Must pass either"):
+            default_if_none()
+
+    def test_too_many_defaults(self):
+        """
+        Raises TypeError if both default and factory are passed.
+        """
+        with pytest.raises(TypeError, match="but not both"):
+            default_if_none(True, lambda: 42)
+
+    def test_factory_takes_self(self):
+        """
+        Raises ValueError if passed Factory has takes_self=True.
+        """
+        with pytest.raises(ValueError, match="takes_self"):
+            default_if_none(Factory(list, takes_self=True))
+
+    @pytest.mark.parametrize("val", [1, 0, True, False, "foo", "", object()])
+    def test_not_none(self, val):
+        """
+        If a non-None value is passed, it's handed down.
+        """
+        c = default_if_none("nope")
+
+        assert val == c(val)
+
+        c = default_if_none(factory=list)
+
+        assert val == c(val)
+
+    def test_none_value(self):
+        """
+        Default values are returned when a None is passed.
+        """
+        c = default_if_none(42)
+
+        assert 42 == c(None)
+
+    def test_none_factory(self):
+        """
+        Factories are used if None is passed.
+        """
+        c = default_if_none(factory=list)
+
+        assert [] == c(None)
+
+        c = default_if_none(default=Factory(list))
+
+        assert [] == c(None)
diff --git a/tests/typing_example.py b/tests/typing_example.py
index 300ac87..527216f 100644
--- a/tests/typing_example.py
+++ b/tests/typing_example.py
@@ -78,3 +78,25 @@ HH(x=[1], y=[], z=1.1)
 
 # same class
 c == cc
+
+
+# Converters
+# XXX: Currently converters can only be functions so none of this works
+# although the stubs should be correct.
+
+# @attr.s
+# class ConvCOptional:
+#     x: Optional[int] = attr.ib(converter=attr.converters.optional(int))
+
+
+# ConvCOptional(1)
+# ConvCOptional(None)
+
+
+# @attr.s
+# class ConvCDefaultIfNone:
+#     x: int = attr.ib(converter=attr.converters.default_if_none(42))
+
+
+# ConvCDefaultIfNone(1)
+# ConvCDefaultIfNone(None)