Skip to content
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

Merged
merged 8 commits into from
Feb 13, 2022
Merged

Conversation

DanielNoord
Copy link
Collaborator

Steps

  • For new features or bug fixes, add a ChangeLog entry describing what your PR does.
  • Write a good description on what the PR does.

Description

This would solve the issue in pylint-dev/pylint#5544.

I explored three options for this:

  1. My first idea was to create a new Instance for dataclasses. However, this is incorrect.
@dataclass
class A(): ...

If you instantiate A it is not a Dataclass, it is an instance of A which happens to be a dataclass. Thus, a new Instance is incorrect.
2) I explored if there is any merit in using is_decorated_with_dataclass or exposing it as util. However, it uses nodes 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 new DataclassDef node seemed a bit overkill (for now).

Type of Changes

Type
✨ New feature

Related Issue

Copy link
Member

@cdce8p cdce8p left a 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.

@DanielNoord
Copy link
Collaborator Author

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 marshmallow recently (in #1298).
We need some place to store the "authorative list of dataclasses".

We could create an util in pylint that builds on a set of names in astroid, but then we would need to maintain a util both in pylint and in astroid.
We could create an util in astroid, but that seemed out of the ordinary compared to how brains usually behave.
We could create an attribute that coveys the information based on the util in astroid.

I chose option three. I think option two might work as well, but to me it seemed more "natural" to let astroid do the inference and "type determination" of the class and then let pylint access that, rather than pylint having to rely on an util from astroid.

@cdce8p
Copy link
Member

cdce8p commented Feb 12, 2022

I chose option three. I think option two might work as well, but to me it seemed more "natural" to let astroid do the inference and "type determination" of the class and then let pylint access that, rather than pylint having to rely on an util from astroid.

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, attrs is not (?).

ChangeLog Outdated Show resolved Hide resolved
astroid/nodes/scoped_nodes/scoped_nodes.py Outdated Show resolved Hide resolved
tests/unittest_brain_dataclasses.py Show resolved Hide resolved
DanielNoord and others added 2 commits February 12, 2022 19:34
@DanielNoord
Copy link
Collaborator Author

That might work. I.e. what you've done here, everything that we can handle with the dataclass brain is fine, attrs is not (?).

I think that makes the most sense, although it could be improved a little. See #1394.

This should be an instance attribute, i.e. set in __init__. Could you add a type annotation, too?

Is that necessary? Won't it be inferred automatically from False?

@cdce8p
Copy link
Member

cdce8p commented Feb 12, 2022

Is that necessary? Won't it be inferred automatically from False?

I think of it more as a sanity check that everything worked. It's not strictly required but nice to have.

Comment on lines 2228 to 2229
self.is_dataclass: bool = False
"""Whether this class is a dataclass."""
Copy link
Member

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.

Copy link
Collaborator Author

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(
Copy link
Member

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.

Copy link
Collaborator Author

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.

Copy link
Member

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.

@DanielNoord
Copy link
Collaborator Author

Pff.. What's wrong on 3.7? 😢


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(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

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?

Copy link
Member

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 😅

@cdce8p
Copy link
Member

cdce8p commented Feb 12, 2022

Pff.. What's wrong on 3.7? 😢

Seems like an issue with the selection logic for #@ in extract_node. I would need a closer look to say more.

Copy link
Member

@cdce8p cdce8p left a 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

@cdce8p cdce8p merged commit cfd9e74 into pylint-dev:main Feb 13, 2022
@DanielNoord DanielNoord deleted the dataclass branch February 13, 2022 09:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement ✨ Improvement to a component
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants