-
-
Notifications
You must be signed in to change notification settings - Fork 278
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 is_dataclass
attribute to ClassDef
#1391
Conversation
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.
Adding a new attribute is certainly possible. I'm just wondering if that's a good choice here. What do we do with the other dataclass-like libraries? Do we add new attributes for each of them or would we reuse is_dataclass
?
You mentioned writing a helper function. Wouldn't it be possible to add that in pylint
? I feel like for the specific issue, that might be a better solution. We can always decide to come back and add the attribute later if we want to.
The problem with a helper or utility function comes from changes to the modules that can create dataclasses. For example, with We could create an I chose option three. I think option two might work as well, but to me it seemed more "natural" to let |
My main concern is that it's not well defined what constitutes a dataclass. Of course there is the stdlib module but aside from that there aren't any real guidelines. Adding an attribute for it thus might be a bit confusing. We could fall back to say it should behave similarly to the stdlib implementation. That might work. I.e. what you've done here, everything that we can handle with the dataclass brain is fine, |
Co-authored-by: Marc Mueller <[email protected]>
I think that makes the most sense, although it could be improved a little. See #1394.
Is that necessary? Won't it be inferred automatically from |
I think of it more as a sanity check that everything worked. It's not strictly required but nice to have. |
self.is_dataclass: bool = False | ||
"""Whether this class is a dataclass.""" |
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 would move this directly below self.doc = doc
. That way all "initial" assignments are grouped together.
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.
:+1
|
||
def test_non_dataclass_is_not_dataclass() -> None: | ||
"""Test that something that isn't a dataclass has the correct attribute.""" | ||
module = astroid.parse( |
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.
You could also use extract_node
together with #@
. That would only return selected nodes.
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'll change it. I always use parse
, but don't have a preference.
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.
Both work fine. extract_node
might just be a bit easier to annotate if you only selected one node type.
Pff.. What's wrong on |
tests/unittest_brain_dataclasses.py
Outdated
|
||
def test_non_dataclass_is_not_dataclass() -> None: | ||
"""Test that something that isn't a dataclass has the correct attribute.""" | ||
ast_nodes = astroid.extract_node( |
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.
ast_nodes = astroid.extract_node( | |
ast_nodes: List[nodes.NodeNG] = astroid.extract_node( # type: ignore[assignment] |
extract_node
can return either a list or an individual node. (Not the best API design 😕) Typing it as List[nodes.NodeNG]
should help.
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.
That's why I added _extract_single_node
in https://github.com/PyCQA/astroid/pull/1358/files. However, now that I think about it it should probably use an assert
instead of returning the first element..
We could create extract_nodes
as well?
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.
Something for another day. We have took many open problems already 😅
Seems like an issue with the selection logic for |
Co-authored-by: Marc Mueller <[email protected]>
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 good! Btw. I had the same issue with extract_node
and ClassDef
😄 Opened #1395
Steps
Description
This would solve the issue in pylint-dev/pylint#5544.
I explored three options for this:
Instance
for dataclasses. However, this is incorrect.If you instantiate
A
it is not aDataclass
, it is an instance ofA
which happens to be a dataclass. Thus, a newInstance
is incorrect.2) I explored if there is any merit in using
is_decorated_with_dataclass
or exposing it as util. However, it usesnodes
to do som stuff which became difficult because of cyclic imports really fast. I don't think there is a good way to extract it from the brain (I tried) so if we want to make a similar util we will need to maintain the same code in two places.3) The current solution. Actually quite simple. If we make any changes to dataclass inference it will also be easily accessible to
pylint
without any changes.Let me know what you think!
One remaining issue is that this doesn't allow setting the
decorators
attribute to be non-optional (which it is for dataclasses). However, creating a newDataclassDef
node seemed a bit overkill (for now).Type of Changes
Related Issue