Repository: python-attrs/attrs
Pull request #731 (2020-12-13 00:27:55)

__attrs_init__()
Here's a first pass for #725. @hynek, please take a look to see if you like this approach, and I can then do all the polish work below. Tests were passing at least locally, for py38. ~Please ignore all the `poetry`-related noise, I'll get rid of that.~

It's a bit ugly for a few reasons:

1.  I wanted to have `{args}` in both the auto-generated `__init__` and `__attrs_init__`, but didn't want to repeat or call that code twice; and it seems non-trival to factor out.

2. We have to always generate `__attrs_init__` (so that the user-provided `__init__` can refer to it), but conditionally generate `__init__`  as before. 

3. Because of (2), `Factory` now calls `_make_init` (even though it has defined `__init__`), but `_make_init` refers to `Factory`, so we need a small hack for this.

I think that `__attrs_pre_init__` will be cleaner to implement, and I'm not sure I fully see the advantages of `__attrs_init__` over `__attrs_pre_init__`. Thoughts?

# 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://www.attrs.org/en/latest/contributing.html) 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] Added **tests** for changed code.
- [x] New features have been added to our [Hypothesis testing strategy](https://github.com/python-attrs/attrs/blob/master/tests/strategies.py).
-  [x] Changes or additions to public APIs are reflected in our type stubs (files ending in ``.pyi``).
    - [x] ...and used in the stub test file `tests/typing_example.py`.
- [x] Updated **documentation** for changed code.
    - [x] New functions/classes have to be added to `docs/api.rst` by hand.
    - [x] Changes to the signature of `@attr.s()` have to be added by hand too.
    - [x] Changed/added classes/methods/functions have appropriate `versionadded`, `versionchanged`, or `deprecated` [directives](http://www.sphinx-doc.org/en/stable/markup/para.html#directive-versionadded).  Find the appropriate next version in our [``__init__.py``](https://github.com/python-attrs/attrs/blob/master/src/attr/__init__.py) file.
- [x] Documentation in `.rst` files is written using [semantic newlines](https://rhodesmill.org/brandon/2012/one-sentence-per-line/).
- [x] Changes (and possible deprecations) have news fragments in [`changelog.d`](https://github.com/python-attrs/attrs/blob/master/changelog.d).

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!


indigoviolet: Looks like I need to handle kw-only args from py2 in some way.  (basically unpacking `_kw_only` in `__init__` before passing to `__attrs_init__`
hynek: First an important lifehack: the solution to your `.envrc` problem is called `.git/info/exclude` which is a directory-specific ignore file. I use it everywhere too.

---

As for the PR: it is quite intrusive, are you sure you wouldn't be fine with `__attrs_pre_init__` for now? I think that should be a lot easier to implement and has the mentioned upside of having proper types. I wanna do both but the length of the PoC is somewhat daunting.

indigoviolet: I'm fine with `__attrs_pre_init__` as mentioned in the issue and summary above, I was under the impression that you preferred this approach. I will abandon this and replace with a different PR for that instead.

Note, however, that this PR does maintain the types for `__init__`. In fact that accounts for much of its intrusiveness, since we need to do the same generation for both `__init__`
and `__attrs_init__`.
hynek: >  I was under the impression that you preferred this approach.

Your impression is 100% on point! It's just that we need both anyways and `__attrs_pre_init__` seems like the lower hanging fruit?

> Note, however, that this PR does maintain the types for `__init__`.

Maybe I misunderstand, but the point of `__attrs_init__` as original intended is that `attrs` does _not_ write an `__init__` at all and provides the user with with an `__attrs_init__` instead.

IOW instead of:

```python
@attr.define
class C:
    x: int

    def __attrs_pre_init__(self):
        do_a()
    def __attrs_post_init__(self):
        do_b()
```

The user can write:

```python
@attr.define(init=some_magic_value_we_need_to_agree_on)
class C:
    x: int
    def __init__(self, x: int) -> None:
        do_a()
        self.__attrs_init__()
        do_b()
```

Are we on the same page at all or am I being confusing?
indigoviolet: > Maybe I misunderstand, but the point of `__attrs_init__` as original intended is that `attrs` does _not_ write an `__init__` at all and provides the user with with an `__attrs_init__` instead.

As I understand it now, your idea is: based on the `init` flag, we will either write `__init__` (as before), or instead name that method (minus the call to `__attrs_post_init__`) as `__attrs_init__`. That makes sense to me, and I will update accordingly.

I was doing something slightly different: always providing an `__attrs_init__` and a default `__init__` implementation that calls `__attrs_init__()` and `__attrs_post_init__()`, which the user can override -- thus removing the need for the `init` flag. But your version makes more sense, and I think it will be simpler, since I'm only renaming that function. 

Separately:

1) why do you think it is useful to have `__attrs_pre_init__()` + `__attrs_post_init__()` as well as  `__attrs_init__()`? It seems like these two have overlapping functionality - when do you anticipate someone using `__attrs_init__()` and their own `__init__()` instead of the pre/post init hooks? I can think of two situations: (a) they want `__init__` to take different arguments than their declared attr.ibs() would generate or (b) they need the arguments to do their pre-init work (and we won't pass arguments to `__attrs_pre_init__()`).

2) Currently `init=True` means an `__init__` is generated, and `init=False` means `__init__` is not. Can we treat `init=False` as indicating that they want an `__attrs_init__`?

indigoviolet: This is now ready for review; not sure how to appease the codecov check, it became red after I added some docs.
indigoviolet: @hynek just want to check that this is on your radar. no rush but if you happen to know when you might get to it, i won't bug you until then :)
hynek: Yes it is, sorry. I need to get out a structlog release before I can focus on bigger attrs work.
indigoviolet: ping?
indigoviolet: Yes, I understood that, but I'm not exactly sure which features used in the
Attribute class are needed to correctly create the Factory class. For
example, does it need to be frozen, does it need to support pickling etc.

On Wed, Jan 20, 2021, 11:20 PM Hynek Schlawack <notifications@github.com>
wrote:

> *@hynek* commented on this pull request.
> ------------------------------
>
> In src/attr/_make.py
> <https://github.com/python-attrs/attrs/pull/731#discussion_r561649535>:
>
> > @@ -2120,7 +2153,7 @@ def fmt_setter_with_converter(
>          )
>          arg_name = a.name.lstrip("_")
>
> -        has_factory = isinstance(a.default, Factory)
> +        has_factory = Factory is not None and isinstance(a.default, Factory)
>
> I'm not saying you should copy paste. What I mean is that I'd like you to
> re-create Factory (as it is right now) using the _add_XXX primities like
> Attribute. Sorry for being fuzzy.
>
> —
> You are receiving this because you authored the thread.
> Reply to this email directly, view it on GitHub
> <https://github.com/python-attrs/attrs/pull/731#discussion_r561649535>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AAA32WX4C5YGGTE6AZRZBHDS27IUNANCNFSM4UY3DKFQ>
> .
>


Patch:
diff --git a/changelog.d/731.change.rst b/changelog.d/731.change.rst
new file mode 100644
index 0000000..c49dba3
--- /dev/null
+++ b/changelog.d/731.change.rst
@@ -0,0 +1,5 @@
+``__attrs__init__()`` will now be injected if ``init==False`` or if ``auto_detect=True`` and a user-defined ``__init__()`` exists.
+
+This enables users to do "pre-init" work in their ``__init__()`` (such as ``super().__init__()``).
+
+``__init__()`` can then delegate constructor argument processing to ``__attrs_init__(*args, **kwargs)``.
diff --git a/src/attr/_make.py b/src/attr/_make.py
index 09a41b1..f817b95 100644
--- a/src/attr/_make.py
+++ b/src/attr/_make.py
@@ -897,6 +897,26 @@ class _ClassBuilder(object):
                 self._is_exc,
                 self._on_setattr is not None
                 and self._on_setattr is not setters.NO_OP,
+                attrs_init=False,
+            )
+        )
+
+        return self
+
+    def add_attrs_init(self):
+        self._cls_dict["__attrs_init__"] = self._add_method_dunders(
+            _make_init(
+                self._cls,
+                self._attrs,
+                self._has_post_init,
+                self._frozen,
+                self._slots,
+                self._cache_hash,
+                self._base_attr_map,
+                self._is_exc,
+                self._on_setattr is not None
+                and self._on_setattr is not setters.NO_OP,
+                attrs_init=True,
             )
         )
 
@@ -1160,6 +1180,11 @@ def attrs(
         ``attrs`` attributes.  Leading underscores are stripped for the
         argument name.  If a ``__attrs_post_init__`` method exists on the
         class, it will be called after the class is fully initialized.
+
+        If ``init`` is ``False``, an ``__attrs_init__`` method will be
+        injected instead. This allows you to define a custom ``__init__``
+        method that can do pre-init work such as ``super().__init__()``,
+        and then call ``__attrs_init__()`` and ``__attrs_post_init__()``.
     :param bool slots: Create a `slotted class <slotted classes>` that's more
         memory-efficient. Slotted classes are generally superior to the default
         dict classes, but have some gotchas you should know about, so we
@@ -1299,6 +1324,8 @@ def attrs(
     .. versionadded:: 20.1.0 *getstate_setstate*
     .. versionadded:: 20.1.0 *on_setattr*
     .. versionadded:: 20.3.0 *field_transformer*
+    .. versionchanged:: 21.1.0
+       ``init=False`` injects ``__attrs_init__``
     """
     if auto_detect and PY2:
         raise PythonTooOldError(
@@ -1408,6 +1435,7 @@ def attrs(
         ):
             builder.add_init()
         else:
+            builder.add_attrs_init()
             if cache_hash:
                 raise TypeError(
                     "Invalid value for cache_hash.  To use hash caching,"
@@ -1872,6 +1900,7 @@ def _make_init(
     base_attr_map,
     is_exc,
     has_global_on_setattr,
+    attrs_init,
 ):
     if frozen and has_global_on_setattr:
         raise ValueError("Frozen classes can't use on_setattr.")
@@ -1908,6 +1937,7 @@ def _make_init(
         is_exc,
         needs_cached_setattr,
         has_global_on_setattr,
+        attrs_init,
     )
     locs = {}
     bytecode = compile(script, unique_filename, "exec")
@@ -1929,10 +1959,10 @@ def _make_init(
         unique_filename,
     )
 
-    __init__ = locs["__init__"]
-    __init__.__annotations__ = annotations
+    init = locs["__attrs_init__"] if attrs_init else locs["__init__"]
+    init.__annotations__ = annotations
 
-    return __init__
+    return init
 
 
 def _setattr(attr_name, value_var, has_on_setattr):
@@ -2047,6 +2077,7 @@ def _attrs_to_init_script(
     is_exc,
     needs_cached_setattr,
     has_global_on_setattr,
+    attrs_init,
 ):
     """
     Return a script of an initializer for *attrs* and a dict of globals.
@@ -2317,10 +2348,12 @@ def _attrs_to_init_script(
             )
     return (
         """\
-def __init__(self, {args}):
+def {init_name}(self, {args}):
     {lines}
 """.format(
-            args=args, lines="\n    ".join(lines) if lines else "pass"
+            init_name=("__attrs_init__" if attrs_init else "__init__"),
+            args=args,
+            lines="\n    ".join(lines) if lines else "pass",
         ),
         names_for_globals,
         annotations,
@@ -2666,7 +2699,6 @@ class _CountingAttr(object):
 _CountingAttr = _add_eq(_add_repr(_CountingAttr))
 
 
-@attrs(slots=True, init=False, hash=True)
 class Factory(object):
     """
     Stores a factory callable.
@@ -2682,8 +2714,7 @@ class Factory(object):
     .. versionadded:: 17.1.0  *takes_self*
     """
 
-    factory = attrib()
-    takes_self = attrib()
+    __slots__ = ("factory", "takes_self")
 
     def __init__(self, factory, takes_self=False):
         """
@@ -2693,6 +2724,38 @@ class Factory(object):
         self.factory = factory
         self.takes_self = takes_self
 
+    def __getstate__(self):
+        """
+        Play nice with pickle.
+        """
+        return tuple(getattr(self, name) for name in self.__slots__)
+
+    def __setstate__(self, state):
+        """
+        Play nice with pickle.
+        """
+        for name, value in zip(self.__slots__, state):
+            setattr(self, name, value)
+
+
+_f = [
+    Attribute(
+        name=name,
+        default=NOTHING,
+        validator=None,
+        repr=True,
+        cmp=None,
+        eq=True,
+        order=False,
+        hash=True,
+        init=True,
+        inherited=False,
+    )
+    for name in Factory.__slots__
+]
+
+Factory = _add_hash(_add_eq(_add_repr(Factory, attrs=_f), attrs=_f), attrs=_f)
+
 
 def make_class(name, attrs, bases=(object,), **attributes_arguments):
     """
@@ -2727,11 +2790,15 @@ def make_class(name, attrs, bases=(object,), **attributes_arguments):
         raise TypeError("attrs argument must be a dict or a list.")
 
     post_init = cls_dict.pop("__attrs_post_init__", None)
-    type_ = type(
-        name,
-        bases,
-        {} if post_init is None else {"__attrs_post_init__": post_init},
-    )
+    user_init = cls_dict.pop("__init__", None)
+
+    body = {}
+    if post_init is not None:
+        body["__attrs_post_init__"] = post_init
+    if user_init is not None:
+        body["__init__"] = user_init
+
+    type_ = type(name, bases, body)
     # For pickling to work, the __module__ variable needs to be set to the
     # frame where the class is created.  Bypass this step in environments where
     # sys._getframe is not defined (Jython for example) or sys._getframe is not


Test patch:
diff --git a/tests/strategies.py b/tests/strategies.py
index a9bc264..fab9716 100644
--- a/tests/strategies.py
+++ b/tests/strategies.py
@@ -154,6 +154,8 @@ def simple_classes(
 
     cls_dict = dict(zip(attr_names, attrs))
     post_init_flag = draw(st.booleans())
+    init_flag = draw(st.booleans())
+
     if post_init_flag:
 
         def post_init(self):
@@ -161,12 +163,20 @@ def simple_classes(
 
         cls_dict["__attrs_post_init__"] = post_init
 
+    if not init_flag:
+
+        def init(self, *args, **kwargs):
+            self.__attrs_init__(*args, **kwargs)
+
+        cls_dict["__init__"] = init
+
     return make_class(
         "HypClass",
         cls_dict,
         slots=slots_flag if slots is None else slots,
         frozen=frozen_flag if frozen is None else frozen,
         weakref_slot=weakref_flag if weakref_slot is None else weakref_slot,
+        init=init_flag,
     )
 
 
diff --git a/tests/test_dunders.py b/tests/test_dunders.py
index 52b72f7..87aa36f 100644
--- a/tests/test_dunders.py
+++ b/tests/test_dunders.py
@@ -69,6 +69,7 @@ def _add_init(cls, frozen):
         base_attr_map={},
         is_exc=False,
         has_global_on_setattr=False,
+        attrs_init=False,
     )
     return cls
 
diff --git a/tests/test_funcs.py b/tests/test_funcs.py
index 20e2747..2fc73dc 100644
--- a/tests/test_funcs.py
+++ b/tests/test_funcs.py
@@ -544,7 +544,14 @@ class TestEvolve(object):
         # No generated class will have a four letter attribute.
         with pytest.raises(TypeError) as e:
             evolve(C(), aaaa=2)
-        expected = "__init__() got an unexpected keyword argument 'aaaa'"
+
+        if hasattr(C, "__attrs_init__"):
+            expected = (
+                "__attrs_init__() got an unexpected keyword argument 'aaaa'"
+            )
+        else:
+            expected = "__init__() got an unexpected keyword argument 'aaaa'"
+
         assert (expected,) == e.value.args
 
     def test_validator_failure(self):
diff --git a/tests/test_make.py b/tests/test_make.py
index fad4ec7..232e770 100644
--- a/tests/test_make.py
+++ b/tests/test_make.py
@@ -569,6 +569,19 @@ class TestAttributes(object):
 
         assert sentinel == getattr(C, method_name)
 
+    @pytest.mark.parametrize("init", [True, False])
+    def test_respects_init_attrs_init(self, init):
+        """
+        If init=False, adds __attrs_init__ to the class.
+        Otherwise, it does not.
+        """
+
+        class C(object):
+            x = attr.ib()
+
+        C = attr.s(init=init)(C)
+        assert hasattr(C, "__attrs_init__") != init
+
     @pytest.mark.skipif(PY2, reason="__qualname__ is PY3-only.")
     @given(slots_outer=booleans(), slots_inner=booleans())
     def test_repr_qualname(self, slots_outer, slots_inner):
@@ -1527,6 +1540,7 @@ class TestClassBuilder(object):
             .add_order()
             .add_hash()
             .add_init()
+            .add_attrs_init()
             .add_repr("ns")
             .add_str()
             .build_class()