Repository: python-attrs/attrs
Pull request #1380 (2024-12-09 15:24:15)

Make attrs.converters.pipe only return a Converter instance if one is passed
# Summary

Make `pipe()` return a regular callable if only passed callables, only return a `Converter` instance if one is strictly required (by having a `Converter` instance within the pipeline.)

Some tests are affected by this change (mostly in how `__annotations__` is accessed), this PR reverts most of them to their state before #1267.

<!-- Please tell us what your pull request is about here. -->

# Pull Request Check List

<!--
This is just a friendly reminder about the most common mistakes.
Please make sure that you tick all boxes.
But please read our [contribution guide](https://github.com/python-attrs/attrs/blob/main/.github/CONTRIBUTING.md) 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 left to do.
If your pull request is a documentation fix or a trivial typo, feel free to delete the whole thing.
-->

- [x] Do **not** open pull requests from your `main` branch – **use a separate branch**!
  - There's a ton of footguns waiting if you don't heed this warning. You can still go back to your project, create a branch from your main branch, push it, and open the pull request from the new branch.
  - This is not a pre-requisite for your pull request to be accepted, but **you have been warned**.
- [x] Added **tests** for changed code.
  Our CI fails if coverage is not 100%.
- (N/A) New features have been added to our [Hypothesis testing strategy](https://github.com/python-attrs/attrs/blob/main/tests/strategies.py).
- (N/A) Changes or additions to public APIs are reflected in our type stubs (files ending in ``.pyi``).
- (N/A) Updated **documentation** for changed code.
- (N/A) Documentation in `.rst` and `.md` files is written using [semantic newlines](https://rhodesmill.org/brandon/2012/one-sentence-per-line/).
- [ ] 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.

<!--
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!
-->


filbranden: @hynek @Tinche please take a look, let me know if you have any questions or would like to see any changes in this PR.

Also, I'd appreciate it if this and my previous couple of PRs could make it into an official release soon, is there a chance you'd consider cutting a 24.3.0 after this PR gets merged? Thanks!

hynek: > @hynek @Tinche please take a look, let me know if you have any questions or would like to see any changes in this PR.

Would you mind sharing the reasoning? It was kinda on purpose to normalize how the converters look like but I'm not married to it.

> Also, I'd appreciate it if this and my previous couple of PRs could make it into an official release soon, is there a chance you'd consider cutting a 24.3.0 after this PR gets merged? Thanks!

I will do my best to release a 24.3 before NYE. We need to fix #1347 first, hopefully merge #1358, and ideally implement #1313 (but that's more of a hope than a requirement).
filbranden: > Would you mind sharing the reasoning? It was kinda on purpose to normalize how the converters look like but I'm not married to it.

Sure. In trying to upgrade to 24.2.0, I found some situations where code was calling one of the converter functions and trying to use that directly as a callable (I believe it was with `optional(...)` rather than `pipe(...)`, but I'm not 100% sure there weren't cases of using `pipe()` that way. When I pushed #1372 I made sure the updated `optional(...)` would return a callable if passed a callable.) 

The other issue I've encountered is that `Converter` instances trigger a bug in `inspect.signature()` in Python 3.10. This has been fixed in https://github.com/python/cpython/issues/101293 but this was never backported to Python 3.10. I found other workarounds that involve adding a `__call__()` to Converter, or setting `__signature__` explicitly if detecting Python 3.10 and under, but it turns out returning a callable when passed a callable seems to address my use cases of that as well, so it doesn't require any other workarounds anymore.

> I will do my best to release a 24.3 before NYE.

Thank you! 
filbranden: Updated to add type signatures with overloads, that specify passing callables will return a callable for both `optional()` (already the case) and `pipe()` (changed in this PR.) Plus a few other minor cleanups.

Patch:
diff --git a/src/attr/_make.py b/src/attr/_make.py
index 239ea2c..530ce39 100644
--- a/src/attr/_make.py
+++ b/src/attr/_make.py
@@ -2932,11 +2932,25 @@ def pipe(*converters):
     .. versionadded:: 20.1.0
     """
 
-    def pipe_converter(val, inst, field):
-        for c in converters:
-            val = c(val, inst, field) if isinstance(c, Converter) else c(val)
+    return_instance = any(isinstance(c, Converter) for c in converters)
 
-        return val
+    if return_instance:
+
+        def pipe_converter(val, inst, field):
+            for c in converters:
+                val = (
+                    c(val, inst, field) if isinstance(c, Converter) else c(val)
+                )
+
+            return val
+
+    else:
+
+        def pipe_converter(val):
+            for c in converters:
+                val = c(val)
+
+            return val
 
     if not converters:
         # If the converter list is empty, pipe_converter is the identity.
@@ -2957,4 +2971,6 @@ def pipe(*converters):
         if rt:
             pipe_converter.__annotations__["return"] = rt
 
-    return Converter(pipe_converter, takes_self=True, takes_field=True)
+    if return_instance:
+        return Converter(pipe_converter, takes_self=True, takes_field=True)
+    return pipe_converter
diff --git a/src/attr/converters.pyi b/src/attr/converters.pyi
index f902e18..12bd0c4 100644
--- a/src/attr/converters.pyi
+++ b/src/attr/converters.pyi
@@ -1,13 +1,19 @@
-from typing import Callable, TypeVar, overload
+from typing import Callable, Any, overload
 
-from attrs import _ConverterType
-
-_T = TypeVar("_T")
+from attrs import _ConverterType, _CallableConverterType
 
+@overload
+def pipe(*validators: _CallableConverterType) -> _CallableConverterType: ...
+@overload
 def pipe(*validators: _ConverterType) -> _ConverterType: ...
+@overload
+def optional(converter: _CallableConverterType) -> _CallableConverterType: ...
+@overload
 def optional(converter: _ConverterType) -> _ConverterType: ...
 @overload
-def default_if_none(default: _T) -> _ConverterType: ...
+def default_if_none(default: Any) -> _CallableConverterType: ...
 @overload
-def default_if_none(*, factory: Callable[[], _T]) -> _ConverterType: ...
+def default_if_none(
+    *, factory: Callable[[], Any]
+) -> _CallableConverterType: ...
 def to_bool(val: str | int | bool) -> bool: ...
diff --git a/src/attrs/__init__.pyi b/src/attrs/__init__.pyi
index c6902bd..05e5a0c 100644
--- a/src/attrs/__init__.pyi
+++ b/src/attrs/__init__.pyi
@@ -51,7 +51,8 @@ _C = TypeVar("_C", bound=type)
 
 _EqOrderType = bool | Callable[[Any], Any]
 _ValidatorType = Callable[[Any, "Attribute[_T]", _T], Any]
-_ConverterType = Callable[[Any], Any] | Converter[Any, _T]
+_CallableConverterType = Callable[[Any], Any]
+_ConverterType = _CallableConverterType | Converter[Any, Any]
 _ReprType = Callable[[Any], str]
 _ReprArgType = bool | _ReprType
 _OnSetAttrType = Callable[[Any, "Attribute[Any]", Any], Any]


Test patch:
diff --git a/tests/test_annotations.py b/tests/test_annotations.py
index cd09a8c..6fbae3c 100644
--- a/tests/test_annotations.py
+++ b/tests/test_annotations.py
@@ -278,27 +278,25 @@ class TestAnnotations:
         def identity(z):
             return z
 
-        assert attr.converters.pipe(int2str).converter.__annotations__ == {
+        assert attr.converters.pipe(int2str).__annotations__ == {
             "val": int,
             "return": str,
         }
-        assert attr.converters.pipe(
-            int2str, strlen
-        ).converter.__annotations__ == {
+        assert attr.converters.pipe(int2str, strlen).__annotations__ == {
             "val": int,
             "return": int,
         }
-        assert attr.converters.pipe(
-            identity, strlen
-        ).converter.__annotations__ == {"return": int}
-        assert attr.converters.pipe(
-            int2str, identity
-        ).converter.__annotations__ == {"val": int}
+        assert attr.converters.pipe(identity, strlen).__annotations__ == {
+            "return": int
+        }
+        assert attr.converters.pipe(int2str, identity).__annotations__ == {
+            "val": int
+        }
 
         def int2str_(x: int, y: int = 0) -> str:
             return str(x)
 
-        assert attr.converters.pipe(int2str_).converter.__annotations__ == {
+        assert attr.converters.pipe(int2str_).__annotations__ == {
             "val": int,
             "return": str,
         }
@@ -310,19 +308,19 @@ class TestAnnotations:
 
         p = attr.converters.pipe()
 
-        assert "val" in p.converter.__annotations__
+        assert "val" in p.__annotations__
 
-        t = p.converter.__annotations__["val"]
+        t = p.__annotations__["val"]
 
         assert isinstance(t, typing.TypeVar)
-        assert p.converter.__annotations__ == {"val": t, "return": t}
+        assert p.__annotations__ == {"val": t, "return": t}
 
     def test_pipe_non_introspectable(self):
         """
         pipe() doesn't crash when passed a non-introspectable converter.
         """
 
-        assert attr.converters.pipe(print).converter.__annotations__ == {}
+        assert attr.converters.pipe(print).__annotations__ == {}
 
     def test_pipe_nullary(self):
         """
@@ -332,7 +330,7 @@ class TestAnnotations:
         def noop():
             pass
 
-        assert attr.converters.pipe(noop).converter.__annotations__ == {}
+        assert attr.converters.pipe(noop).__annotations__ == {}
 
     def test_optional(self):
         """
diff --git a/tests/test_converters.py b/tests/test_converters.py
index 4747f13..5726ae2 100644
--- a/tests/test_converters.py
+++ b/tests/test_converters.py
@@ -248,11 +248,11 @@ class TestPipe:
 
         # First wrapped converter fails:
         with pytest.raises(ValueError):
-            c.converter(33, None, None)
+            c(33)
 
         # Last wrapped converter fails:
         with pytest.raises(ValueError):
-            c.converter("33", None, None)
+            c("33")
 
     def test_sugar(self):
         """
@@ -273,7 +273,7 @@ class TestPipe:
         """
         o = object()
 
-        assert o is pipe().converter(o, None, None)
+        assert o is pipe()(o)
 
     def test_wrapped_annotation(self):
         """