-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Refactor pyerverse Association Logic #10397
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
base: main
Are you sure you want to change the base?
Conversation
π€ According to the primer, this change has no effect on the checked open source code. π€π This comment was generated for commit d0ea86e |
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 like the addition! π
I only left two remarks for cases where at least from the comment it seems like the relationship is processed by the wrong handler. Am I missing something?
It would be great if you can add tests for all possible output formats as all printers where changed. You can specify multiple output formats for a single functional test in the [testoptions]
.
if isinstance(value, astroid.node_classes.Name): | ||
current = set(parent.aggregations_type[node.attrname]) | ||
parent.aggregations_type[node.attrname] = list( | ||
current | utils.infer_node(node) | ||
) | ||
return | ||
|
||
# Handle comprehensions | ||
# Aggregation: comprehensions (self.x = [P() for ...]) |
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.
Wouldn't this be composition as well as in the example P
is instantiated by the class?
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, I would agree here. I think we need to differentiate here aswell (I need to handle list comprehensions in both the AggregationsHandler and the CompostionsHandler)
@@ -358,12 +390,23 @@ def handle(self, node: nodes.AssignAttr, parent: nodes.ClassDef) -> None: | |||
parent.aggregations_type[node.attrname] = list(current | {element_type}) | |||
return | |||
|
|||
# Fallback to parent handler | |||
# Type annotation only (x: P) defaults to aggregation |
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.
From the PR description the type annotation case would be association, not aggregation?
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.
Agree, thank you. That case needs to move aswell
Thanks for the review. I agree that the logic is not complete and it needs some more work and I need to figure out all the possible edge cases. Also I think some of the other tests have changed , and I need to rebase aswell. I think this will take some time, but lets see how it goes :) |
Type of Changes
Description
Closes #9045
Summary
Fixed incorrect UML semantics in pyreverse for aggregation vs composition relationships. The previous implementation had no explicit
CompositionsHandler
class and did not differentiate between Aggregation and Composition properly, treating most relationships as aggregation by default.Problem
Pyreverse previously lacked proper UML relationship semantics. So I would proposes the following distinction for Python class relationships:
fields.py
fields.mmd
Changes Made
1. Added CompositionsHandler
Created dedicated handler following chain of responsibility pattern:
CompositionsHandler β AggregationsHandler β AssociationsHandler
2. Updated Printers
EdgeType.COMPOSITION
enum value3. Prevented Duplicate Relationships
Modified extract_relationships() to process relationships by priority and avoid duplicates.
Currently I only made the modifications needed to implement the new Associations Logic. If you like the proposed distinction I would work on making the tests pass :)