Fix duplicate fields Pyreverse bug #9004
Conversation
for more information, see https://pre-commit.ci
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #9004 +/- ##
==========================================
- Coverage 95.76% 95.75% -0.02%
==========================================
Files 173 173
Lines 18639 18648 +9
==========================================
+ Hits 17850 17856 +6
- Misses 789 792 +3
|
tests/pyreverse/functional/class_diagrams/attributes/duplicates.py
Outdated
Show resolved
Hide resolved
pylint/pyreverse/diagrams.py
Outdated
| for n, m in list(node.locals_type.items()) + list( | ||
| node.instance_attrs_type.items() | ||
| ): | ||
| if n not in properties: | ||
| properties[n] = m |
There was a problem hiding this comment.
Let's use explicit name instead, I don't know what n/m is and I took 30s to try to come up with a proper suggestion: definitely a read blocker π
There was a problem hiding this comment.
I'm not totally sure what m is, so I went with name and attr.
|
Also not sure if we backported the tests previously so we can't backport to 2.17.6 afaik (unless we also backport the prior PR which is still possible). Added to the 3.0.0a8 milestone because it might not be that important. |
There was a problem hiding this comment.
I agree with Pierre and the wish for more explicit names, but I also have nothing more to add. Thanks!
We did not backport the PR with the tests yet. I'd say if 3.0 is going to be released soon(ish) we don't need to, but if it's going to take a few more months we can backport, as it is also not a big code change.
|
Realistically 3.0 could be released far in the future, I'm going to backport #8983 (especially since it should just a label to apply) |
|
Thanks! |
pylint/pyreverse/diagrams.py
Outdated
| + properties | ||
| properties = { | ||
| name: attr | ||
| for name, attr in node.items() |
There was a problem hiding this comment.
I had to look into the code as well, but as I see it:
Here, node is of type nodes.ClassDef, and node.items() is a mapping from local name to the node defining the local:
https://github.com/pylint-dev/astroid/blob/600229f5dbe29ce9074b3c76a9d195951cb561a4/astroid/nodes/scoped_nodes/mixin.py#L179-L180
Thus for local_name, local_node in node.items() would be best IMO.
pylint/pyreverse/diagrams.py
Outdated
| for name, attr in list(node.locals_type.items()) + list( | ||
| node.instance_attrs_type.items() |
There was a problem hiding this comment.
Here, locals_type and instance_attrs_type are mappings from attribute name to possible types (from annotations or inference results), so for attr_name, attr_type in ... would be better.
|
π€ According to the primer, this change has no effect on the checked open source code. π€π This comment was generated for commit 306e454 |
|
π |
* Fix duplicate fields --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> (cherry picked from commit 022988a)
(cherry picked from commit 022988a) Co-authored-by: Nick Drozd <[email protected]>
Type of Changes
Description
Closes #8189