-
-
Notifications
You must be signed in to change notification settings - Fork 275
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
Add support for attrs v21.3.0+ #1331
Conversation
Since version [21.3.0](https://github.com/python-attrs/attrs/releases/tag/21.3.0) you can now `import attrs` instead of just `import attr`. This patch adds support so that astroid doesn't barf on classes created using `@attrs.define`.
astroid/brain/brain_attrs.py
Outdated
ATTRS_NAMES = frozenset( | ||
( | ||
"attr.s", | ||
"attrs", | ||
"define", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we should add this.
Sadly this brain does not infer the actual function behind the decorator but only looks at the name. However, if anybody uses define
as a decorator name in non-attr
code they will now also get picked up by this brain. Fort some reason we allowed attrib
and field
here, see #1187 (review).
Do we want to add define
as well @Pierre-Sassoulas? I can't imagine it being a very often used name for decorators, but it might create troubles down the line..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well I don't like assuming that something is part of a lib only because of its name especially since we have inference capability in astroid. I know we're doing it for np
in some place. But inference would permit to infer the real name of the origin lib. I don't know enough about brains to suggest something proper instead. What do you think @hippo91 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Pierre-Sassoulas i agree with you and @DanielNoord as well.
IMHO define
should not be part of this set.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay, I've removed @define
from the list of supported decorator names.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for making the PR as well 😄
Just one question about what we should actually add to the list.
for more information, see https://pre-commit.ci
If you want black to format properly small line, you need to remove the ending comma (it's black magic comma) |
Thanks for the pointer! I think the issue is that I have an older |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @jacobbogdanov!
One recommendation for a little test docstring but rest LGTM!
@@ -10,7 +10,9 @@ | |||
from astroid.nodes.node_classes import AnnAssign, Assign, AssignName, Call, Unknown | |||
from astroid.nodes.scoped_nodes import ClassDef | |||
|
|||
ATTRIB_NAMES = frozenset(("attr.ib", "attrib", "attr.attrib", "attr.field", "field")) | |||
ATTRIB_NAMES = frozenset( | |||
("attr.ib", "attrib", "attr.attrib", "attr.field", "attrs.field", "field") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we could discuss if attrib
and field
should be in this list but that ship has sailed I think.
Thanks for removing define
! 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you're right, it's also a problem.
("attr.ib", "attrib", "attr.attrib", "attr.field", "attrs.field", "field") | |
("attr.ib", "attr.attrib", "attr.field", "attrs.field") |
Suggestion done in order to check if it's causing test fails. Imo we can most probably not infer the value as aliasing of attrs must be rare considering it's a small name, but we also should infer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks like it breaks the existing test_attr_transform
as well as the newly added test_attrs_transform
.
@attrs
class Bah:
d = attrib(attr.Factory(dict))
Also there's a regression test in pylint
that also relies on the existing attrib
; https://github.com/PyCQA/pylint/blob/main/tests/functional/r/regression/regression_4439.py
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Pierre-Sassoulas Let's revert then. Thanks @jacobbogdanov for checking this out.
Co-authored-by: Daniël van Noord <13665637+DanielNoord@users.noreply.github.com>
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
Thanks a lot for the fix @jacobbogdanov ! It will be available in astroid 2.10 ! |
We forgot to add a changelog entry 😅 |
Description
Since version 21.3.0
you can now
import attrs
instead of justimport attr
.This patch adds support so that astroid doesn't barf on classes created using
@attrs.define
.NOTE: Not not all names, most notably
attr.ib
andattr.s
are available inattrs
.The only decorators in that list are
@frozen
,@mutable
and@define
. The rest are classes or functions for interacting with attrs classes.I added support for
@define
because the first example from the release notes has it,but was unsure about whether
@mutable
and@frozen
should be done too.Type of Changes
Related Issue
Closes #1330