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.

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

NG: convert on setattr by default #886

Merged
merged 5 commits into from
Dec 14, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 4 additions & 0 deletions changelog.d/835.breaking.rst
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
When using ``@attr.define``, converters are now run by default when setting an attribute on an instance -- additionally to validators.
I.e. the new default is ``on_setattr=[attr.setters.convert, attr.setters.validate]``.

This is unfortunately a breaking change, but it was an oversight, impossible to raise a ``DeprecationWarning`` about, and it's better to fix it now while the APIs are very fresh with few users.
4 changes: 4 additions & 0 deletions changelog.d/886.breaking.rst
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
When using ``@attr.define``, converters are now run by default when setting an attribute on an instance -- additionally to validators.
I.e. the new default is ``on_setattr=[attr.setters.convert, attr.setters.validate]``.

This is unfortunately a breaking change, but it was an oversight, impossible to raise a ``DeprecationWarning`` about, and it's better to fix it now while the APIs are very fresh with few users.
4 changes: 3 additions & 1 deletion docs/api.rst
Original file line number Diff line number Diff line change
Expand Up @@ -707,13 +707,15 @@ The most notable differences are:
- *auto_exc=True*
- *auto_detect=True*
- *eq=True*, but *order=False*
- Validators run when you set an attribute (*on_setattr=attr.setters.validate*).
- Converters and validators are run when you set an attribute (*on_setattr=[attr.setters.convert, attr.setters.validate*]).
- Some options that aren't relevant to Python 3 have been dropped.

Please note that these are *defaults* and you're free to override them, just like before.

Since the Python ecosystem has settled on the term ``field`` for defining attributes, we have also added `attr.field` as a substitute for `attr.ib`.

.. versionchanged:: 21.3.0 Converters are also run ``on_setattr``.

.. note::

`attr.s` and `attr.ib` (and their serious business cousins) aren't going anywhere.
Expand Down
32 changes: 25 additions & 7 deletions src/attr/_make.py
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,8 @@
# Unique object for unequivocal getattr() defaults.
_sentinel = object()

_ng_default_on_setattr = setters.pipe(setters.convert, setters.validate)


class _Nothing(object):
"""
Expand Down Expand Up @@ -722,13 +724,31 @@ def __init__(
self._cls_dict["__delattr__"] = _frozen_delattrs

self._wrote_own_setattr = True
elif on_setattr == setters.validate:
elif on_setattr in (
_ng_default_on_setattr,
setters.validate,
setters.convert,
):
has_validator = has_converter = False
for a in attrs:
if a.validator is not None:
has_validator = True
if a.converter is not None:
has_converter = True

if has_validator and has_converter:
break
else:
# If class-level on_setattr is set to validating, but there's
# no field to validate, pretend like there's no on_setattr.
if (
(
on_setattr == _ng_default_on_setattr
and not (has_validator or has_converter)
)
or (on_setattr == setters.validate and not has_validator)
or (on_setattr == setters.convert and not has_converter)
):
# If class-level on_setattr is set to convert + validate, but
# there's no field to convert or validate, pretend like there's
# no on_setattr.
self._on_setattr = None

if getstate_setstate:
Expand Down Expand Up @@ -2123,9 +2143,7 @@ def _make_init(
raise ValueError("Frozen classes can't use on_setattr.")

needs_cached_setattr = True
elif (
has_cls_on_setattr and a.on_setattr is not setters.NO_OP
) or _is_slot_attr(a.name, base_attr_map):
elif has_cls_on_setattr and a.on_setattr is not setters.NO_OP:
needs_cached_setattr = True

unique_filename = _generate_unique_filename(cls, "init")
Expand Down
22 changes: 16 additions & 6 deletions src/attr/_next_gen.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,13 @@
from attr.exceptions import UnannotatedAttributeError

from . import setters
from ._make import NOTHING, _frozen_setattrs, attrib, attrs
from ._make import (
NOTHING,
_frozen_setattrs,
_ng_default_on_setattr,
attrib,
attrs,
)


def define(
Expand All @@ -35,8 +41,10 @@ def define(
match_args=True,
):
r"""
The only behavioral differences are the handling of the *auto_attribs*
option:
Define an ``attrs`` class.

The behavioral differences to `attr.s` are the handling of the
*auto_attribs* option:

:param Optional[bool] auto_attribs: If set to `True` or `False`, it behaves
exactly like `attr.s`. If left `None`, `attr.s` will try to guess:
Expand All @@ -46,9 +54,11 @@ def define(
2. Otherwise it assumes *auto_attribs=False* and tries to collect
`attr.ib`\ s.

and that mutable classes (``frozen=False``) validate on ``__setattr__``.
and that mutable classes (``frozen=False``) convert and validate on
``__setattr__``.

.. versionadded:: 20.1.0
.. versionchanged:: 21.3.0 Converters are also run ``on_setattr``.
"""

def do_it(cls, auto_attribs):
Expand Down Expand Up @@ -86,9 +96,9 @@ def wrap(cls):

had_on_setattr = on_setattr not in (None, setters.NO_OP)

# By default, mutable classes validate on setattr.
# By default, mutable classes convert & validate on setattr.
if frozen is False and on_setattr is None:
on_setattr = setters.validate
on_setattr = _ng_default_on_setattr

# However, if we subclass a frozen class, we inherit the immutability
# and disable on_setattr.
Expand Down
59 changes: 56 additions & 3 deletions tests/test_functional.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@

import attr

from attr._compat import PY2, TYPE
from attr._compat import PY2, PY36, TYPE
from attr._make import NOTHING, Attribute
from attr.exceptions import FrozenInstanceError

Expand Down Expand Up @@ -692,8 +692,9 @@ class C(object):
@pytest.mark.parametrize("slots", [True, False])
def test_no_setattr_if_validate_without_validators(self, slots):
"""
If a class has on_setattr=attr.setters.validate (default in NG APIs)
but sets no validators, don't use the (slower) setattr in __init__.
If a class has on_setattr=attr.setters.validate (former default in NG
APIs) but sets no validators, don't use the (slower) setattr in
__init__.

Regression test for #816.
"""
Expand All @@ -713,6 +714,58 @@ class D(C):
assert "self.y = y" in src
assert object.__setattr__ == D.__setattr__

@pytest.mark.parametrize("slots", [True, False])
def test_no_setattr_if_convert_without_converters(self, slots):
"""
If a class has on_setattr=attr.setters.convert but sets no validators,
don't use the (slower) setattr in __init__.
"""

@attr.s(on_setattr=attr.setters.convert)
class C(object):
x = attr.ib()

@attr.s(on_setattr=attr.setters.convert)
class D(C):
y = attr.ib()

src = inspect.getsource(D.__init__)

assert "setattr" not in src
assert "self.x = x" in src
assert "self.y = y" in src
assert object.__setattr__ == D.__setattr__

@pytest.mark.skipif(not PY36, reason="NG APIs are 3.6+")
@pytest.mark.parametrize("slots", [True, False])
def test_no_setattr_with_ng_defaults(self, slots):
"""
If a class has the NG default on_setattr=[convert, validate] but sets
no validators or converters, don't use the (slower) setattr in
__init__.
"""

@attr.define
class C(object):
x = attr.ib()

src = inspect.getsource(C.__init__)

assert "setattr" not in src
assert "self.x = x" in src
assert object.__setattr__ == C.__setattr__

@attr.define
class D(C):
y = attr.ib()

src = inspect.getsource(D.__init__)

assert "setattr" not in src
assert "self.x = x" in src
assert "self.y = y" in src
assert object.__setattr__ == D.__setattr__

def test_on_setattr_detect_inherited_validators(self):
"""
_make_init detects the presence of a validator even if the field is
Expand Down
25 changes: 25 additions & 0 deletions tests/test_next_gen.py
Original file line number Diff line number Diff line change
Expand Up @@ -308,3 +308,28 @@ class MyException(Exception):

assert "foo" == ei.value.x
assert ei.value.__cause__ is None

def test_converts_and_validates_by_default(self):
"""
If no on_setattr is set, assume setters.convert, setters.validate.
"""

@attr.define
class C:
x: int = attr.field(converter=int)

@x.validator
def _v(self, _, value):
if value < 10:
raise ValueError("must be >=10")

inst = C(10)

# Converts
inst.x = "11"

assert 11 == inst.x

# Validates
with pytest.raises(ValueError, match="must be >=10"):
inst.x = "9"