Add --max-depth option to control diagram complexity #10077
Conversation
Julfried
changed the title
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #10077 +/- ##
==========================================
+ Coverage 95.80% 95.84% +0.03%
==========================================
Files 174 175 +1
Lines 18976 19056 +80
==========================================
+ Hits 18180 18264 +84
+ Misses 796 792 -4
|
Pierre-Sassoulas
added
Enhancement β¨
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Quick update: I've completed the implementation and added tests for the --max-depth option. I believe the feature is working as intended and is ready for review. Iβm currently fighting some CI issues, but they seem unrelated to my changes. Any help or advice would be much appreciated! |
This comment has been minimized.
This comment has been minimized.
|
Thanks! Appreciate your review and the clarification about the CI issue. I`ll keep an eye on any updates and am happy to rebase if needed. |
This comment has been minimized.
This comment has been minimized.
|
Thank you for rebasing, the mypy error is coming from main the other errors seems genuine. (Spelling is checked only in ci not in local pre-commit) |
|
Thanks for pointing out the mypy error, I`ll keep an eye on it if it gets fixed in main. How about disabling spelling check for these test cases because "-" is not allowed in Python naming anyway? |
This comment has been minimized.
This comment has been minimized.
|
It seems that the spelling checks still fail, because it checks for words and ignores the quotes. How about adding these testnames to [SPELLING]
# List of comma separated words that should not be checked.
spelling-ignore-words=subpkg,MyClass |
This comment has been minimized.
This comment has been minimized.
|
When working on test coverage I noticed tests/data/init.py is empty, which impacts pyreverse's ability to detect complete package relationships. This effects validation of the --max-depth functionality since module relationships aren't fully established and the resulting diagrams do not represent the whole structure. @Pierre-Sassoulas do you maybe remember if this was an intentional design choice, or did this just grow over time? Note: Addressing this would require modifying all affected test files to properly reflect package relationships. Given the scope, I think handling such changes in a separate PR would be better, if we decide to do this! |
|
I'm not sure. Pylint had a lot of issues with namespace package and missing |
|
Thanks for checking! You're right that it's about an empty (not missing) init.py. The difference is how many relationships pyreverse detects:
I think this effects depth filtering coverage since we're not testing package-level relationship filtering. Although I want to add, that I am not a testing expert. Here is what I added to init.py: from .clientmodule_test import Ancestor, Specialization
from .suppliermodule_test import Interface, DoNothing, DoNothing2
from .nullable_pattern import NullablePatterns
from .property_pattern import PropertyPatterns |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Thanks for your earlier guidance, Pierre! I hope this is PR is not getting to large. I now rebased to fix the mypy error and addressed the spelling issues. Also, I made some mistakes when setting up the tests, which resulted in pyreverse generating wrong diagrams. Now I use a proper test setup with fixtures and test multiple depths, to improve test coverage. The issue with pyreverse not properly detecting all relationships when init.py is empty still remains. I believe this is why coverage for package-level relationships is still incomplete. In my understanding, an empty init.py file limits the ability to fully establish package-level relationships, as it doesnβt define imports or explicitly link modules in the package. This might explain why pyreverse struggles with package-level filtering in such cases. However, Iβm not entirely sure if this is the root cause or if thereβs another factor at play. Iβm happy to explore this further and improve the tests in a follow-up PR if needed. Let me know what you thinkπ |
There was a problem hiding this comment.
Sorry for the delay. Changes seems sensible, I have a possible refactor to suggest. (I'd like a review from Andras if possible though, no pressure @DudeNr33 ;) )
| @@ -129,6 +179,12 @@ def write_classes(self, diagram: ClassDiagram) -> None: | |||
| ) | |||
| # inheritance links | |||
| for rel in diagram.get_relationships("specialization"): | |||
There was a problem hiding this comment.
| for rel in diagram.get_relationships("specialization"): | |
| for rel in diagram.get_relationships("specialization", depth=self.max_depth): |
Wonder if it's possible of filtering on depth directly in get_relationships instead. Seems like it could reduce duplication of .should_show_node checks when looping on it
There was a problem hiding this comment.
Thanks for the suggestion! I would agree, that handling everything regarding filtering in one place would benefit maintainability. However I think we still need node filtering when emitting nodes, to determine which ones to show in the final diagram. Not sure about splitting this up into 2 files either. What do you think?
There was a problem hiding this comment.
Actually, I feel like we could simplify this, by introducing get_nodes() alongside get_relationships(). Then the writer would look something like this.
def write_packages(self, diagram: PackageDiagram) -> None:
for module in diagram.get_nodes(depth=self.max_depth):
# Emit node logic
for rel in diagram.get_relationships(depth=self.max_depth):
# Emit edge logicThat way all the filtering logic would be handled consistently inside the diagram classes and the printer would only need to focus on rendering the final components. What do you think?
There was a problem hiding this comment.
Since this is a big change, I am not sure how to approach this. Are you ok with changing it in this PR?
There was a problem hiding this comment.
Right, let's open another PR for the refactor required to add the depth argument.
|
No worries, Currently I am also very packed. Will try to take a look at it later this week :) |
There was a problem hiding this comment.
Sorry for the delay, I am not very active on GitHub at the moment.
Thank you for the PR - this is actually a feature I saw in a similar tool and always thought "would be really helpful to have this in pyreverse"!
Regarding the changes (and following the discussion you already had with Pierre), I feel like the logic is better suited in diadefslib.py, and could be handled similarly to --show_associated.
But like Pierre I think handling this in a follow-up PR makes sense, so that this one does not get too big.
Besides that I only have on remark regarding the option group. π
|
Thanks, appreciate your review. Both of your suggestions make sense - especially the architectural one about moving depth filtering to diadefslib.py alongside other filters like --show_associated. I've moved the --max-depth option to the Filtering and Scope group to better align with similar options. I am happy to do a refactor in a follow up PR :) |
Type of Changes
Description
This PR adds a new
--max-depthoption to pyreverse that allows basic controlling of the visualization depth of packages and classes in the generated diagrams. The depth is simply calculated by counting dots in qualified names, where depth 0 represents top-level packages/classes.For packages, the full path depth is considered. For classes, the depth is calculated based on their containing module to ensure meaningful class diagrams while respecting the depth limit.
Closes #9233