Ability to have kw_only attributes defined anywhere, as an optional feature

[fisadev]

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

• Added tests for changed code.
• New features have been added to our Hypothesis testing strategy.
• 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.
  • 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.
  • Changed/added classes/methods/functions have appropriate versionadded, versionchanged, or deprecated directives.
• Documentation in .rst files is written using semantic newlines.
• Changes (and possible deprecations) have news fragments in 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 Allow keyword-only attributes before positional attributes #448 (comment) 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.

P.S. 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:

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:

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!

[fisadev]

@hynek ouch, sorry. And thanks for merging it! :)