kw_only python 2 backport

[Drino]

Hello!

Thank you for bringing an awesome package and supporting Python 2 in 2020!

In my experience I find kw_only attributes really addictive, as they allow to inherit from classes ignoring attributes with defaults. Unfortunately, they don't work on Python 2, so it's actually impossible to write some compatible code.

Even writing __init__ by hand:

@attr.s(kw_only=not PY2)
class Base(object):
    kw_only1 = attr.ib(default=None)
    kw_only2 = attr.ib(default=42)

@attr.s(init=not PY2)
class Child(Base):
    attr1 = attr.ib()
    attr2 = attr.ib()

    if PY2:
        def __init__(self, attr1, attr2, **kwargs):
            super(Child, self).__init__(**kwargs)
            self.attr1, self.attr2 = attr1, attr2

Will fail, as attrs disallows mandatory attributes after an attribute with a default value or factory. Even this non-working code snippet seems as boilerplate, so in this PR I propose a backport of kw_only attr.ib to Python 2.

There are four approaches to implementation that I've tested:

1. Just move kw_only attributes to the end of __init__. It's the fastest approach, on my benckmark (3 positional args, 3 kw_only args) it's about ~100ns for argument parsing. However, it doesn't allow kw_only attributes without default values and also doesn't make them real kw_only.
2. Pop from a dictionary of kwargs. This is the slowest approach with a somewhat-reasonable error message, ~400ns for argument parsing.
3. On every __init__ call create a function that takes kw_only attributes and returns a tuple of unpacked arguments (implemented in this PR). It's slightly faster than previous approach, ~390ns for argument parsing.
4. Cache that function on class instance - it saves ~40-50ns, giving 340ns. I plan to rewrite this PR to use this approach.

Both approaches with function have error message lacking some information, however it's a default py2 error and the constructor call should be somewhere in the stacktrace.

Is this a desired feature or should it not happen? Should I add some fancy error handling?

P.S. I'm unfamiliar with changelog.d - should I just add an rst file to the dir?

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 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.

• 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. Find the appropriate next version in our __init__.py file.
• 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 honestly don’t care about the micro-performance on Python 2 since it’s only run on class creation anyways. So I say keep it as simple as possible.

But it looks really gnarly…could you isolate it into a function please (not the much simpler Python 3 path, I don’t want the function call overhead in the common case)?

Speaking of performance: please avoid the usage of str.format if you can. I know attrs is still full of it but it’s slow af and I don’t want it in new code. 🙏

You can find details on changelog.d in our contribution guide: https://www.attrs.org/en/latest/contributing.html#changelog

[Drino]

@hynek thank you for the answer!

As we don't care about performance in this case I rewritten it to poping from a dict (approach 2) and added fancy error-handling - now error-message is much closer to python2/python3 native ones for __init__().

Am I correct that this change doesn't need a testing strategy?

[Drino]

@hynek could you please help me with the failing codecov check?

I don't understand why the coverage decreased. I've added two functions, both are PY2-only and hidden under if PY2:. The only branching depends on presence of default value, but we have tests for both cases (kw_only with default and kw_only without default).

UPD: The line that is "not covered" is this list comprehension: https://codecov.io/gh/python-attrs/attrs/src/edd79f21aeb7bf71b6ea4af13af66992e04e42e2/src/attr/_make.py#L1949

UPD2: Seems like this only occurs if I use list comprehension - changed lines += to lines.extend and got rid of this issue.

[hynek]

Thanks, let's hope it doesn't fall apart in production. :)