From 9b228927e75c453ab5a862bc2431c2c324b8e838 Mon Sep 17 00:00:00 2001 From: Filipe Brandenburger Date: Tue, 12 Nov 2024 20:54:32 -0500 Subject: [PATCH 1/5] Allow converter.optional to take a converter such as converter.pipe as its argument --- changelog.d/1372.change.md | 1 + src/attr/converters.py | 21 ++++++++++++----- tests/test_annotations.py | 22 ++++++++++-------- tests/test_converters.py | 46 +++++++++++++++++++++++++++++++++++--- 4 files changed, 72 insertions(+), 18 deletions(-) create mode 100644 changelog.d/1372.change.md diff --git a/changelog.d/1372.change.md b/changelog.d/1372.change.md new file mode 100644 index 000000000..fcb94345b --- /dev/null +++ b/changelog.d/1372.change.md @@ -0,0 +1 @@ +`attrs.converters.optional()` works again when taking `attrs.converters.pipe()` or another Converter as its argument. diff --git a/src/attr/converters.py b/src/attr/converters.py index cac62dda7..e715d266c 100644 --- a/src/attr/converters.py +++ b/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, inst, field): + if val is None: + return None + return converter(val) xtr = _AnnotationExtractor(converter) @@ -48,7 +57,7 @@ def optional_converter(val): if rt: optional_converter.__annotations__["return"] = typing.Optional[rt] - return optional_converter + return Converter(optional_converter, takes_self=True, takes_field=True) def default_if_none(default=NOTHING, factory=None): diff --git a/tests/test_annotations.py b/tests/test_annotations.py index cd09a8c7e..3eb72a178 100644 --- a/tests/test_annotations.py +++ b/tests/test_annotations.py @@ -351,22 +351,26 @@ def strify(x) -> str: def identity(x): return x - assert attr.converters.optional(int2str).__annotations__ == { + assert attr.converters.optional(int2str).converter.__annotations__ == { "val": typing.Optional[int], "return": typing.Optional[str], } - assert attr.converters.optional(int_identity).__annotations__ == { - "val": typing.Optional[int] - } - assert attr.converters.optional(strify).__annotations__ == { + assert attr.converters.optional( + int_identity + ).converter.__annotations__ == {"val": typing.Optional[int]} + assert attr.converters.optional(strify).converter.__annotations__ == { "return": typing.Optional[str] } - assert attr.converters.optional(identity).__annotations__ == {} + assert ( + attr.converters.optional(identity).converter.__annotations__ == {} + ) def int2str_(x: int, y: int = 0) -> str: return str(x) - assert attr.converters.optional(int2str_).__annotations__ == { + assert attr.converters.optional( + int2str_ + ).converter.__annotations__ == { "val": typing.Optional[int], "return": typing.Optional[str], } @@ -377,7 +381,7 @@ def test_optional_non_introspectable(self): converter. """ - assert attr.converters.optional(print).__annotations__ == {} + assert attr.converters.optional(print).converter.__annotations__ == {} def test_optional_nullary(self): """ @@ -387,7 +391,7 @@ def test_optional_nullary(self): def noop(): pass - assert attr.converters.optional(noop).__annotations__ == {} + assert attr.converters.optional(noop).converter.__annotations__ == {} @pytest.mark.skipif( sys.version_info[:2] < (3, 11), diff --git a/tests/test_converters.py b/tests/test_converters.py index 164fb0ffa..5d7eb35f7 100644 --- a/tests/test_converters.py +++ b/tests/test_converters.py @@ -124,7 +124,7 @@ def test_success_with_type(self): """ c = optional(int) - assert c("42") == 42 + assert c("42", None, None) == 42 def test_success_with_none(self): """ @@ -132,7 +132,7 @@ def test_success_with_none(self): """ c = optional(int) - assert c(None) is None + assert c(None, None, None) is None def test_fail(self): """ @@ -141,7 +141,7 @@ def test_fail(self): c = optional(int) with pytest.raises(ValueError): - c("not_an_int") + c("not_an_int", None, None) class TestDefaultIfNone: @@ -272,6 +272,46 @@ 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): """ From cc8e100ab0a35aecb85838349643de84ac2e466b Mon Sep 17 00:00:00 2001 From: Filipe Brandenburger Date: Wed, 13 Nov 2024 13:38:00 -0500 Subject: [PATCH 2/5] Only turn optional into a Converter if needed --- src/attr/converters.py | 10 ++++++++-- tests/test_annotations.py | 22 +++++++++------------- tests/test_converters.py | 14 +++++++++++--- 3 files changed, 28 insertions(+), 18 deletions(-) diff --git a/src/attr/converters.py b/src/attr/converters.py index e715d266c..c24bcdf33 100644 --- a/src/attr/converters.py +++ b/src/attr/converters.py @@ -40,13 +40,19 @@ def optional_converter(val, inst, field): return None return converter(val, inst, field) + result = Converter( + optional_converter, takes_self=True, takes_field=True + ) + else: - def optional_converter(val, inst, field): + def optional_converter(val): if val is None: return None return converter(val) + result = optional_converter + xtr = _AnnotationExtractor(converter) t = xtr.get_first_param_type() @@ -57,7 +63,7 @@ def optional_converter(val, inst, field): if rt: optional_converter.__annotations__["return"] = typing.Optional[rt] - return Converter(optional_converter, takes_self=True, takes_field=True) + return result def default_if_none(default=NOTHING, factory=None): diff --git a/tests/test_annotations.py b/tests/test_annotations.py index 3eb72a178..cd09a8c7e 100644 --- a/tests/test_annotations.py +++ b/tests/test_annotations.py @@ -351,26 +351,22 @@ def strify(x) -> str: def identity(x): return x - assert attr.converters.optional(int2str).converter.__annotations__ == { + assert attr.converters.optional(int2str).__annotations__ == { "val": typing.Optional[int], "return": typing.Optional[str], } - assert attr.converters.optional( - int_identity - ).converter.__annotations__ == {"val": typing.Optional[int]} - assert attr.converters.optional(strify).converter.__annotations__ == { + assert attr.converters.optional(int_identity).__annotations__ == { + "val": typing.Optional[int] + } + assert attr.converters.optional(strify).__annotations__ == { "return": typing.Optional[str] } - assert ( - attr.converters.optional(identity).converter.__annotations__ == {} - ) + assert attr.converters.optional(identity).__annotations__ == {} def int2str_(x: int, y: int = 0) -> str: return str(x) - assert attr.converters.optional( - int2str_ - ).converter.__annotations__ == { + assert attr.converters.optional(int2str_).__annotations__ == { "val": typing.Optional[int], "return": typing.Optional[str], } @@ -381,7 +377,7 @@ def test_optional_non_introspectable(self): converter. """ - assert attr.converters.optional(print).converter.__annotations__ == {} + assert attr.converters.optional(print).__annotations__ == {} def test_optional_nullary(self): """ @@ -391,7 +387,7 @@ def test_optional_nullary(self): def noop(): pass - assert attr.converters.optional(noop).converter.__annotations__ == {} + assert attr.converters.optional(noop).__annotations__ == {} @pytest.mark.skipif( sys.version_info[:2] < (3, 11), diff --git a/tests/test_converters.py b/tests/test_converters.py index 5d7eb35f7..f87d4d9e3 100644 --- a/tests/test_converters.py +++ b/tests/test_converters.py @@ -124,7 +124,7 @@ def test_success_with_type(self): """ c = optional(int) - assert c("42", None, None) == 42 + assert c("42") == 42 def test_success_with_none(self): """ @@ -132,7 +132,7 @@ def test_success_with_none(self): """ c = optional(int) - assert c(None, None, None) is None + assert c(None) is None def test_fail(self): """ @@ -141,7 +141,15 @@ def test_fail(self): c = optional(int) with pytest.raises(ValueError): - c("not_an_int", None, None) + 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: From 4a0da063718ab65dc8bc69c219aa5c46d0956ba9 Mon Sep 17 00:00:00 2001 From: Filipe Brandenburger Date: Thu, 14 Nov 2024 11:42:03 -0500 Subject: [PATCH 3/5] 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 --- src/attr/converters.py | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/src/attr/converters.py b/src/attr/converters.py index c24bcdf33..0a79deef0 100644 --- a/src/attr/converters.py +++ b/src/attr/converters.py @@ -40,10 +40,6 @@ def optional_converter(val, inst, field): return None return converter(val, inst, field) - result = Converter( - optional_converter, takes_self=True, takes_field=True - ) - else: def optional_converter(val): @@ -51,8 +47,6 @@ def optional_converter(val): return None return converter(val) - result = optional_converter - xtr = _AnnotationExtractor(converter) t = xtr.get_first_param_type() @@ -63,7 +57,10 @@ def optional_converter(val): if rt: optional_converter.__annotations__["return"] = typing.Optional[rt] - return result + if isinstance(converter, Converter): + return Converter(optional_converter, takes_self=True, takes_field=True) + + return optional_converter def default_if_none(default=NOTHING, factory=None): From 60af5412c5dc1e328bd7472a291b8ca632d27a89 Mon Sep 17 00:00:00 2001 From: Hynek Schlawack Date: Sun, 17 Nov 2024 11:19:11 +0100 Subject: [PATCH 4/5] Update tests/test_converters.py --- tests/test_converters.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/test_converters.py b/tests/test_converters.py index f87d4d9e3..0ce378eaf 100644 --- a/tests/test_converters.py +++ b/tests/test_converters.py @@ -314,6 +314,7 @@ class C: ) c1 = C() + assert None is c1.x c2 = C("True") From d2ade0de594b5cd246c2a97b479b06894ae48fd6 Mon Sep 17 00:00:00 2001 From: Hynek Schlawack Date: Sun, 17 Nov 2024 11:19:16 +0100 Subject: [PATCH 5/5] Update tests/test_converters.py --- tests/test_converters.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/test_converters.py b/tests/test_converters.py index 0ce378eaf..d4aca4307 100644 --- a/tests/test_converters.py +++ b/tests/test_converters.py @@ -318,6 +318,7 @@ class C: assert None is c1.x c2 = C("True") + assert True is c2.x