New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[attrs plugin] Support kw_only=True #6107

Merged
merged 2 commits into from Dec 30, 2018

Conversation

Projects
None yet
2 participants
@euresti
Copy link
Contributor

euresti commented Dec 27, 2018

Fixes #6083

@ilevkivskyi
Copy link
Collaborator

ilevkivskyi left a comment

Thank you for PR! This looks very good, I just have some suggestions for more docs and tests.

auto_attribs: bool = ...) -> Callable[[_C], _C]: ...
auto_attribs: bool = ...,
kw_only: bool = ...,
cache_hash: bool = ...,

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Dec 27, 2018

Collaborator

I noticed that some of these are missing in typeshed. Could you please make a typeshed update PR?

This comment has been minimized.

@euresti

euresti Dec 27, 2018

Author Contributor

Now that attrs includes stubs in the package maybe removing them from typeshed is better. python-attrs/attrs#480

b = attr.ib("16") # type: str

B(b="foo", a=7)

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Dec 27, 2018

Collaborator

This empty line is not needed.

==
b.py:5: error: Non keyword-only attributes are not allowed after a keyword-only attribute.


This comment has been minimized.

@ilevkivskyi

ilevkivskyi Dec 27, 2018

Collaborator

We prefer single empty line between tests.

[builtins fixtures/attr.pyi]
[out]
==
b.py:5: error: Non keyword-only attributes are not allowed after a keyword-only attribute.

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Dec 27, 2018

Collaborator

This test doesn't test much of incremental logic. I would also add a test where base class in a.py is updated, while subclass is unchanged.

@@ -367,6 +398,11 @@ def _attribute_from_attrib_maker(ctx: 'mypy.plugin.ClassDefContext',

# Read all the arguments from the call.
init = _get_bool_argument(ctx, rvalue, 'init', True)
kw_only |= _get_bool_argument(ctx, rvalue, 'kw_only', False)
if kw_only and ctx.api.options.python_version[0] < 3:
ctx.api.fail("kw_only is not supported in Python 2", stmt)

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Dec 27, 2018

Collaborator

I would avoid repeating the same error message, and instead define a constant.

@@ -367,6 +398,11 @@ def _attribute_from_attrib_maker(ctx: 'mypy.plugin.ClassDefContext',

# Read all the arguments from the call.
init = _get_bool_argument(ctx, rvalue, 'init', True)
kw_only |= _get_bool_argument(ctx, rvalue, 'kw_only', False)

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Dec 27, 2018

Collaborator

I noticed this (a bit counterintuitive) behavior:

import attr
@attr.s(kw_only=True)
class A:
    a = attr.ib(kw_only=False)

A(1)  # Error

The logic here captures it correctly, but I would add a short comment that this is intentional.

This comment has been minimized.

@euresti

euresti Dec 27, 2018

Author Contributor

I filed an issue in attrs to see if this is expected: python-attrs/attrs#481

def _analyze_class(ctx: 'mypy.plugin.ClassDefContext', auto_attribs: bool) -> List[Attribute]:
def _analyze_class(ctx: 'mypy.plugin.ClassDefContext',
auto_attribs: bool,
kw_only: bool) -> List[Attribute]:

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Dec 27, 2018

Collaborator

Could you please document kw_only (and auto_attribs) in the docstring?

CR
@euresti

This comment has been minimized.

Copy link
Contributor Author

euresti commented Dec 29, 2018

Ok I made the requested changes.

@ilevkivskyi
Copy link
Collaborator

ilevkivskyi left a comment

LG now, thanks for updates!

@ilevkivskyi ilevkivskyi merged commit baef333 into python:master Dec 30, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment