From fedc57055fbd79a50e5f522510755653dfd71145 Mon Sep 17 00:00:00 2001 From: Hynek Schlawack Date: Mon, 13 Dec 2021 14:11:19 +0100 Subject: [PATCH 1/5] NG: convert on setattr by default Not doing that from the get-go was an oversight. Fixes #835 --- changelog.d/835.breaking.rst | 4 ++++ changelog.d/886.breaking.rst | 4 ++++ docs/api.rst | 4 +++- src/attr/_next_gen.py | 14 +++++++++----- tests/test_next_gen.py | 25 +++++++++++++++++++++++++ 5 files changed, 45 insertions(+), 6 deletions(-) create mode 100644 changelog.d/835.breaking.rst create mode 100644 changelog.d/886.breaking.rst diff --git a/changelog.d/835.breaking.rst b/changelog.d/835.breaking.rst new file mode 100644 index 000000000..8cdf0412d --- /dev/null +++ b/changelog.d/835.breaking.rst @@ -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. diff --git a/changelog.d/886.breaking.rst b/changelog.d/886.breaking.rst new file mode 100644 index 000000000..8cdf0412d --- /dev/null +++ b/changelog.d/886.breaking.rst @@ -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. diff --git a/docs/api.rst b/docs/api.rst index afbc87025..1dcaf978e 100644 --- a/docs/api.rst +++ b/docs/api.rst @@ -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. diff --git a/src/attr/_next_gen.py b/src/attr/_next_gen.py index 1d8acac36..6dd2ebc11 100644 --- a/src/attr/_next_gen.py +++ b/src/attr/_next_gen.py @@ -35,8 +35,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: @@ -46,9 +48,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): @@ -86,9 +90,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 = [setters.convert, setters.validate] # However, if we subclass a frozen class, we inherit the immutability # and disable on_setattr. diff --git a/tests/test_next_gen.py b/tests/test_next_gen.py index fce01ad45..fdc06b719 100644 --- a/tests/test_next_gen.py +++ b/tests/test_next_gen.py @@ -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" From 7d4c1845b436a63d769935eb8de522f01a5256e9 Mon Sep 17 00:00:00 2001 From: Hynek Schlawack Date: Mon, 13 Dec 2021 15:32:27 +0100 Subject: [PATCH 2/5] Add optimization for default on_setattr w/ no work to do Otherwise we'd end up with an explicit setattr every time. --- src/attr/_make.py | 11 ++++++++--- src/attr/_next_gen.py | 10 ++++++++-- 2 files changed, 16 insertions(+), 5 deletions(-) diff --git a/src/attr/_make.py b/src/attr/_make.py index 7f22b273b..e229a1e81 100644 --- a/src/attr/_make.py +++ b/src/attr/_make.py @@ -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): """ @@ -722,13 +724,16 @@ def __init__( self._cls_dict["__delattr__"] = _frozen_delattrs self._wrote_own_setattr = True - elif on_setattr == setters.validate: + elif on_setattr == _ng_default_on_setattr: for a in attrs: if a.validator is not None: break + if a.converter is not None: + 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 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: diff --git a/src/attr/_next_gen.py b/src/attr/_next_gen.py index 6dd2ebc11..843447173 100644 --- a/src/attr/_next_gen.py +++ b/src/attr/_next_gen.py @@ -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( @@ -92,7 +98,7 @@ def wrap(cls): # By default, mutable classes convert & validate on setattr. if frozen is False and on_setattr is None: - on_setattr = [setters.convert, setters.validate] + on_setattr = _ng_default_on_setattr # However, if we subclass a frozen class, we inherit the immutability # and disable on_setattr. From f4ee131003274e856bff0ed2891d651998ff220a Mon Sep 17 00:00:00 2001 From: Hynek Schlawack Date: Mon, 13 Dec 2021 16:16:19 +0100 Subject: [PATCH 3/5] Fix optimization for NG default & j/ convert --- src/attr/_make.py | 25 +++++++++++++++++++------ tests/test_functional.py | 35 +++++++++++++++++++++++++++++++++-- 2 files changed, 52 insertions(+), 8 deletions(-) diff --git a/src/attr/_make.py b/src/attr/_make.py index e229a1e81..990786954 100644 --- a/src/attr/_make.py +++ b/src/attr/_make.py @@ -724,13 +724,28 @@ def __init__( self._cls_dict["__delattr__"] = _frozen_delattrs self._wrote_own_setattr = True - elif on_setattr == _ng_default_on_setattr: + 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: - break + has_validator = True if a.converter is not None: + has_converter = True + + if has_validator and has_converter: break - else: + 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. @@ -2128,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") diff --git a/tests/test_functional.py b/tests/test_functional.py index e9aa3c267..77a4ea060 100644 --- a/tests/test_functional.py +++ b/tests/test_functional.py @@ -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. """ @@ -713,6 +714,36 @@ class D(C): assert "self.y = y" in src assert object.__setattr__ == D.__setattr__ + @pytest.mark.skipif(PY2, reason="NG APIs are 3+") + @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 From 5a927a0ee98928d2def9245175fb5e1aa85b93df Mon Sep 17 00:00:00 2001 From: Hynek Schlawack Date: Mon, 13 Dec 2021 16:20:53 +0100 Subject: [PATCH 4/5] NG is actually 3.6+ --- tests/test_functional.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/test_functional.py b/tests/test_functional.py index 77a4ea060..3b9d95eb7 100644 --- a/tests/test_functional.py +++ b/tests/test_functional.py @@ -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 @@ -714,7 +714,7 @@ class D(C): assert "self.y = y" in src assert object.__setattr__ == D.__setattr__ - @pytest.mark.skipif(PY2, reason="NG APIs are 3+") + @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): """ From cb490f8abd0fe4afe2e64b9005ae37264d59844c Mon Sep 17 00:00:00 2001 From: Hynek Schlawack Date: Mon, 13 Dec 2021 16:29:35 +0100 Subject: [PATCH 5/5] Add test for convert optimization for good measure --- tests/test_functional.py | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/tests/test_functional.py b/tests/test_functional.py index 3b9d95eb7..c616f8c1f 100644 --- a/tests/test_functional.py +++ b/tests/test_functional.py @@ -714,6 +714,28 @@ 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):