Refactor pyreverse Association Logic #10397
Conversation
This comment has been minimized.
This comment has been minimized.
Pierre-Sassoulas
added
pyreverse
There was a problem hiding this comment.
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].
|
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 :) |
This comment has been minimized.
This comment has been minimized.
|
@Julfried please ping me once the PR is ready to be reviewed again. No hurry though, just want to point this out as I am not actively monitoring the open PRs and instead rely more on the email notifications. π |
jacobtylerwalls
changed the title
β¦o that the differentation between aggregation and composition is correct according to UMLEnhance composition and aggregation handling in AST node processing so that the differentation between aggregation and composition is correct according to UML
β¦unreliable for now
β¦ons for extracting element types and resolving class definitionsEnhance type resolution in AssociationsHandler and add utility functions for extracting element types and resolving class definitions
β¦w pyreverse correctly extracts Dummy as Association
This comment has been minimized.
This comment has been minimized.
β¦ instead of assoiciation)
β¦ass level attributes separate
β¦s composition instead of association)
β¦ fixes last failing test
Codecov ReportAttention: Patch coverage is
β Your patch check has failed because the patch coverage (98.26%) is below the target coverage (100.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #10397 +/- ##
==========================================
- Coverage 95.91% 95.88% -0.03%
==========================================
Files 176 176
Lines 19147 19227 +80
==========================================
+ Hits 18364 18436 +72
- Misses 783 791 +8
π New features to boost your workflow:
|
This comment has been minimized.
This comment has been minimized.
for more information, see https://pre-commit.ci
This comment has been minimized.
This comment has been minimized.
|
@DudeNr33 Finally ready for review again. All functional tests are passing and the association logic is working correctly now. Interestingly, I figured while playing around that this PR also fixes #10333 (duplicate arrows issue) as a side effect. The root cause was the same - improper relationship classification led to the same attribute being processed multiple times across different relationship types. Our priority-based processing with duplicate prevention solves this at the source. This example from #10333: class A:
def __init__(self) -> None:
self.var = 2
class B:
def __init__(self) -> None:
self.a_obj = A()
def func(self):
self.a_obj = A()
self.a_obj = A()Now correctly produces single composition relationship instead of duplicates: classDiagram
class A {
var : int
}
class B {
a_obj
func()
}
A --* B : a_objShould I add the functional test cases from #10333 to this PR, or handle them in a follow-up? The PR is getting quite large, but its up to you. The core association logic is solid and ready for review either way. |
| @@ -0,0 +1,3 @@ | |||
| Enhanced pyreverse to properly distinguish between UML relationship types (association, aggregation, composition) based on object ownership semantics. Type annotations without assignment are now treated as associations, parameter assignments as aggregations, and object instantiation as compositions. | |||
|
|
|||
| Closes #9045 | |||
There was a problem hiding this comment.
If you acccidentally fixed multiple issues, you can close more then one :D
|
I think it's better to add functional test cases for the other issue here as you fixed it here. Usually we do another MR because we don't realise an issue is fixed until later. |
|
I updated the functional tests, so now this also Closes #9267 The example from #8046 currently produces this output: @startuml classes_toy_code
set namespaceSeparator none
class "ExampleClass" as parser.toy_code.ExampleClass {
example1 : Optional[int]
example2 : Optional[int]
}
@endumlHowever I think this is not the output the was discussed in the issue, since the class level attributes are still not handled correctly. Let me know what you think :) |
Type of Changes
Description
Closes #9045
Summary
Fixed incorrect UML semantics in pyreverse for aggregation vs composition relationships. The previous implementation had no explicit
CompositionsHandlerclass 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.pyfields.mmdChanges Made
1. Added CompositionsHandler
Created dedicated handler following chain of responsibility pattern:
CompositionsHandler β AggregationsHandler β AssociationsHandler
2. Updated Printers
EdgeType.COMPOSITIONenum 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 :)