Repository: python-attrs/attrs
Pull request #559 (2019-07-26 21:46:04)

Ability to have `kw_only` attributes defined anywhere, as an optional feature
Ability to have `kw_only` attributes defined anywhere, as an optional feature (when enabled, 
don't raise errors if a non keyword-only attribute is defined after a keyword-only one).

I'm really interested in allowing `kw_only` attrs anywhere, because it would fix the problem of 
not being able to add mandatory attrs to classes if the base class already has optional 
attrs.

By making the optional attrs keyword-only, and allowing keyword-only attrs anywhere, the
child classes will can add their own mandatory attrs after the otional keyword-only ones from 
the base class.

This would be super useful for me in several projects in which I hit that wall. And I suspect 
there are many facing the same problem (#38, questions in stack overflow, etc).

In #448 concerns were raised regarding the obvious ordering of attributes getting less obvious 
because of this. So to avoid unexpected ordering changes, I decided to make this behaviour 
optional and disabled by default, using a new parameter in `attr.s`.

```
@attr.s
class Animal:
    legs = attr.ib()
    alive = attr.ib(default=True, kw_only=True)

@attr.s(kw_only_order_check=False)
class Dog(Animal):
    name = attr.ib()
```

I know, terrible name for the parameter, can't think of a better one right now. Suggestions?

But the behaviour is this:

- `kw_only_order_check=True` (Default): don't allow non keyword-only attrs after keyword-only 
  attrs.
- `kw_only_order_check=False`: allow keyword-only attrs anywhere.

Is there anything else I should check or add tests to?
And I'm not sure where should I document this.

Also, I would like the change itself to be validated before I continue working on tests and docs, if possible, so I don't spend too much time on this if you don't think the feature should be added :)


# Pull Request Check List

- [x] Added **tests** for changed code.
- [ ] 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``).
    - [ ] ...and used in the stub test file `tests/typing_example.py`.
- [ ] Updated **documentation** for changed code.
    - [x] New functions/classes have to be added to `docs/api.rst` by hand.
    - [ ] 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).
- [ ] Documentation in `.rst` files is written using [semantic newlines](https://rhodesmill.org/brandon/2012/one-sentence-per-line/).
- [ ] 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!


hynek: I haven’t looked at that code in a while, so based on your patch, we already sort kw_only at the end anyways and all you have to do is to disable an artificial sanity check to leave it through? 🤔 
fisadev: @hynek kind of, but at least on `__init__` methods `kw_only` attrs don't really have an order. They are keyword only, so by definition they don't have a specific position, they can come in any place. Though maybe their order does matter somewhere else that I don't know of? (serializing? repr? anything like that?)
hynek: As far as I can see it, the idea is solid. The name not so much. :) I think it should reflect what the user gets, not how it's implemented. I think even `kw_only_anywhere` wouldn't be that bad but I'm open to suggestions.

Also: is there a scenario where leaving the check off by default could be considered a regression? 🤔
fisadev: I've updated the param name to the suggested `kw_only_anywhere`. I can change it if another better one is proposed :)

I will start working on the docs!
fisadev: Hmm, some checks are failing with stuff that seems to be unrelated to the change: `Git fetch failed with exit code: 128`
hynek: They pass now but for some reason one of the checks doesn’t run. I even think that’s because AP changed something between you opening this PR and now (it used to be only one check).

Could you rebase on master please? Maybe it’ll fix itself...
hynek: - I have updated the branch for you and it's passing.
- I have spelunked the history of kw_only and at no point we have discussed why that check should be there. I suspect @malinoff only added the check for symmetry with defaults.
- @wsanchez brought up sort of a reason in https://github.com/python-attrs/attrs/issues/448#issuecomment-433993501 but I'm honestly not convinced.
- I cannot imagine a case where this is a breaking change.

What I'm saying is: just remove the check, keep the test and add a newsfragment that announces it.  No option. Can you do that anytime soon or should I just do it quickly for you?
fisadev: @hynek great! Ok, I can do the change today :)
malinoff: I decided to implement `kw_only`  support in the first place because I want my apis sound better. It should be very clear what arguments are positional, and what are keyword-only, and that behavior should be consistent across different invocations of constructors for different classes in the hierarchy.
This PR introduces inconsistency, imho. Now the rules are quite simple, and follow the standard python language feature. With this PR merged, no more simple.
For example, such inconsistency arises when I work with a base class instance, `Animal('legs', alive=True)`. Then change my code to work with a subclass:
* I can't extend my constructors with a new positional argument, of course: `Dog('legs', alive=True, 'Max')
* I have to either specify name as a keyword argument `Dog('legs', alive=True, name='Max')`, which is good, and now possible if you specify `kw_only=True` to your `name` attribute
* or remove keyword `alive`: `Dog('legs', True, 'Max')`. That keyword argument was marked so for a reason, I won't recall what's `True` in a couple of days. Anyway, let's do this.

A couple of days passed, and I want to refactor my code back to using the base class. Now I have to change all constructor invocations everywhere, again, from the positional argument to the keyword one.

May I ask, why you can't mark `name` attribute in `Dog` class as keyword-only? A snippet from your real project may greatly help.

N.B. of course, I am not a member of `attrs`, so I can't decide to merge or not to merge this PR. The above is only my humble opinion.
hynek: I don't disagree with you, but this sounds like a preference, not a technical necessity? Moreover, it seems like you could add a your own decorator/validator that catches this issue?

Again, I personally don't disagree, but this seems a bit too opinionated. If I'd be like that with everything, there wouldn't be any subclassing in `attrs` at all. ;) And I'm kinda afraid of keeping getting more bug reports about this.
malinoff: To me, it is a technical necessity from the point that pure python doesn't work like that, you cannot magically change a keyword-only argument of a base class constructor to be positional in a subclass without overriding the whole signature.
hynek: > you cannot magically change a keyword-only argument of a base class constructor to be positional in a subclass without overriding the whole signature.

I'm not sure I understand? That's not what we are trying to do here and it's something you can do now anyway:

```python

In [1]: import attr

In [2]: @attr.s
   ...: class C1:
   ...:     x = attr.ib(kw_only=True)
   ...: @attr.s
   ...: class C2(C1):
   ...:     x = attr.ib()
   ...:

In [3]: C2(1)
Out[3]: C2(x=1)

In [4]: C1(1)
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
<ipython-input-4-a55995036880> in <module>
----> 1 C1(1)

TypeError: __init__() takes 1 positional argument but 2 were given
```

This PR is about this use case:

```python
In [4]: @attr.s
   ...: class C1:
   ...:     always_kw = attr.ib(kw_only=True)
   ...: @attr.s
   ...: class C2(C1):
   ...:     pos = attr.ib()
   ...:

In [5]: C2(1, always_kw=2)
Out[5]: C2(always_kw=2, pos=1)
```

Which I kinda can see the value of. Maybe there are use-cases where it makes sense to carry around a kw-only attribute –especially if it has a default value – and leave the rest pos-only? Am I missing something?
malinoff: Ah, right, I should've looked to the test case, sorry. So, all keyword-only attributes will jump to the end of the list of arguments. What if there are multiple base classes, settings multiple keyword-only arguments?
```
@attr.s(kw_only_order_check=False)
class A:
  a1 = attr.ib(kw_only=True)
  a2 = attr.ib()

@attr.s(kw_only_order_check=False)
class B(A):
  b1 = attr.ib(kw_only=True)
  b2 = attr.ib()

@attr.s(kw_only_order_check=False)
class C(B):
  c1 = attr.ib()
  c2 = attr.ib(kw_only=True)
```
What would be the order of arguments of `C`?

What if we throw in multiple inheritance?
```

@attr.s(kw_only_order_check=False)
class X:
  x1 = attr.ib(kw_only=True)
  x2 = attr.ib()

@attr.s(kw_only_order_check=False)
class Y(C, X):
  y1 = attr.ib()
  y2 = attr.ib(kw_only=True)
```
hynek: They already _do_ jump at the end of the arguments – behind a `*`. :)

```
In [8]: inspect.signature(C.__init__)
Out[8]: <Signature (self, a2, b2, c1, *, a1, b1, c2) -> None>
In [11]: inspect.signature(Y.__init__)
Out[11]: <Signature (self, a2, b2, c1, x2, y1, *, a1, b1, c2, x1, y2) -> None>
```

The changes don't modify any behavior at all, they just remove a restriction.
malinoff: Well, I did not expect that :) Honestly, I've never tried to do so.
Sure, removing that restriction seems okay.
hynek: Great, thanks for taking the time to talk it through!
fisadev: Ok, change done: now we can specify kw_only attrs anywhere and they won't be checked for ordering. No more flag to enable or disable the check.

It won't even check if mandatory kw_only attrs are before or after non-mandatory kw_only args, which I think is the right thing to do, because they don't actually have any order when calling init (they can't be positional because they are kw_only). This allowed me to simplify a lot of the code that checked for ordering of mandatory vs non-mandatory attrs.
hynek: You skipped the newsfragment again but I'll do it myself real quick to avoid further cycles.

Thanks!

Patch:
diff --git a/src/attr/_make.py b/src/attr/_make.py
index 40acf43..13fc8bc 100644
--- a/src/attr/_make.py
+++ b/src/attr/_make.py
@@ -374,38 +374,24 @@ def _transform_attrs(cls, these, auto_attribs, kw_only):
 
     attrs = AttrsClass(base_attrs + own_attrs)
 
+    # mandatory vs non-mandatory attr order only matters when they are part of
+    # the __init__ signature and when they aren't kw_only (which are moved to
+    # the end and can be mandatory or non-mandatory in any order, as they will
+    # be specified as keyword args anyway). Check the order of those attrs:
+    attrs_to_check = [
+        a for a in attrs if a.init is not False and a.kw_only is False
+    ]
+
     had_default = False
-    was_kw_only = False
-    for a in attrs:
-        if (
-            was_kw_only is False
-            and had_default is True
-            and a.default is NOTHING
-            and a.init is True
-            and a.kw_only is False
-        ):
+    for a in attrs_to_check:
+        if had_default is True and a.default is NOTHING:
             raise ValueError(
                 "No mandatory attributes allowed after an attribute with a "
                 "default value or factory.  Attribute in question: %r" % (a,)
             )
-        elif (
-            had_default is False
-            and a.default is not NOTHING
-            and a.init is not False
-            and
-            # Keyword-only attributes without defaults can be specified
-            # after keyword-only attributes with defaults.
-            a.kw_only is False
-        ):
+
+        if had_default is False and a.default is not NOTHING:
             had_default = True
-        if was_kw_only is True and a.kw_only is False and a.init is True:
-            raise ValueError(
-                "Non keyword-only attributes are not allowed after a "
-                "keyword-only attribute (unless they are init=False).  "
-                "Attribute in question: {a!r}".format(a=a)
-            )
-        if was_kw_only is False and a.init is True and a.kw_only is True:
-            was_kw_only = True
 
     return _Attributes((attrs, base_attrs, base_attr_map))
 


Test patch:
diff --git a/tests/test_make.py b/tests/test_make.py
index 43acd74..51476a5 100644
--- a/tests/test_make.py
+++ b/tests/test_make.py
@@ -620,27 +620,35 @@ class TestKeywordOnlyAttributes(object):
             "missing 1 required keyword-only argument: 'x'"
         ) in e.value.args[0]
 
-    def test_conflicting_keyword_only_attributes(self):
+    def test_keyword_only_attributes_can_come_in_any_order(self):
         """
-        Raises `ValueError` if keyword-only attributes are followed by
-        regular (non keyword-only) attributes.
+        Mandatory vs non-mandatory attr order only matters when they are part
+        of the __init__ signature and when they aren't kw_only (which are
+        moved to the end and can be mandatory or non-mandatory in any order,
+        as they will be specified as keyword args anyway).
         """
 
+        @attr.s
         class C(object):
-            x = attr.ib(kw_only=True)
-            y = attr.ib()
-
-        with pytest.raises(ValueError) as e:
-            _transform_attrs(C, None, False, False)
-
-        assert (
-            "Non keyword-only attributes are not allowed after a "
-            "keyword-only attribute (unless they are init=False).  "
-            "Attribute in question: Attribute"
-            "(name='y', default=NOTHING, validator=None, repr=True, "
-            "cmp=True, hash=None, init=True, metadata=mappingproxy({}), "
-            "type=None, converter=None, kw_only=False)",
-        ) == e.value.args
+            a = attr.ib(kw_only=True)
+            b = attr.ib(kw_only=True, default="b")
+            c = attr.ib(kw_only=True)
+            d = attr.ib()
+            e = attr.ib(default="e")
+            f = attr.ib(kw_only=True)
+            g = attr.ib(kw_only=True, default="g")
+            h = attr.ib(kw_only=True)
+
+        c = C("d", a="a", c="c", f="f", h="h")
+
+        assert c.a == "a"
+        assert c.b == "b"
+        assert c.c == "c"
+        assert c.d == "d"
+        assert c.e == "e"
+        assert c.f == "f"
+        assert c.g == "g"
+        assert c.h == "h"
 
     def test_keyword_only_attributes_allow_subclassing(self):
         """