Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Allow converter.optional to take a Converter such as converter.pipe as its argument #1372

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
+66 −5
Open

Allow converter.optional to take a Converter such as converter.pipe as its argument #1372

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9b22892
Allow converter.optional to take a converter such as converter.pipe a…
filbranden Nov 13, 2024
cc8e100
Only turn optional into a Converter if needed
filbranden Nov 13, 2024
4a0da06
Move call to Converter constructor to the end of optional()
filbranden Nov 14, 2024
9450c77
Merge branch 'main' into optionalpipe1
hynek Nov 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog.d/1372.change.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
`attrs.converters.optional()` works again when taking `attrs.converters.pipe()` or another Converter as its argument.
22 changes: 17 additions & 5 deletions src/attr/converters.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
import typing

from ._compat import _AnnotationExtractor
from ._make import NOTHING, Factory, pipe
from ._make import NOTHING, Converter, Factory, pipe


__all__ = [
Expand 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)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not guaranteed that the converter takes 3 arguments.

The root issue is that earlier versions of attrs had a "basic" converter that just took a single val. But now with the Converter class, Converter callables may take one, two (self), two(field), or three parameters.

Some introspection here may be necessary.

Also, since converters now may be callables with one two or three params, the type signature ought to be updated as well

https://github.com/python-attrs/attrs/blob/main/src/attrs/__init__.pyi#L54

Something like Callable[[Any], Any] | Callable[[Any, Any], Any] | Callable[[Any, Any, Any], Any]?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually a Converter instance always takes three arguments:

attrs/src/attr/_make.py

Lines 2712 to 2725 in 13e9a6a

if not (self.takes_self or self.takes_field):
self.__call__ = lambda value, _, __: self.converter(value)
elif self.takes_self and not self.takes_field:
self.__call__ = lambda value, instance, __: self.converter(
value, instance
)
elif not self.takes_self and self.takes_field:
self.__call__ = lambda value, __, field: self.converter(
value, field
)
else:
self.__call__ = lambda value, instance, field: self.converter(
value, instance, field
)

The callable it takes as an argument can take one, two or three, but the Converter class normalizes it to always take three arguments

So I believe this is correct

I think the typing information might need an update, to accept Callable[[Any], Any] | Converter but I think that's a separate issue so maybe we should follow up on that on a separate PR?


else:

def optional_converter(val):
if val is None:
return None
return converter(val)

xtr = _AnnotationExtractor(converter)

Expand All @@ -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


Expand Down
48 changes: 48 additions & 0 deletions tests/test_converters.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Expand Down Expand Up @@ -272,6 +280,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):
"""
Expand Down