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 --max-depth option to control diagram complexity #10077

Merged
merged 20 commits into from
Jan 24, 2025
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Update writer to only show modules below max depth setting
  • Loading branch information
Julfried committed Jan 5, 2025
commit 0c159a1d293e029df9ef90089969cbf2b1328877
45 changes: 45 additions & 0 deletions pylint/pyreverse/writer.py
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ def __init__(self, config: argparse.Namespace) -> None:
self.printer: Printer # defined in set_printer
self.file_name = "" # defined in set_printer
self.depth = self.config.max_color_depth
self.max_depth = self.config.max_depth
# default colors are an adaption of the seaborn colorblind palette
self.available_colors = itertools.cycle(self.config.color_palette)
self.used_colors: dict[str, str] = {}
Expand All @@ -53,6 +54,15 @@ def write(self, diadefs: Iterable[ClassDiagram | PackageDiagram]) -> None:
self.write_classes(diagram)
self.save()

def should_show_node(self, qualified_name: str) -> bool:
"""Determine if a node should be shown based on depth settings."""
# If no depth limit is set ==> show all nodes
if self.max_depth is None:
return True
# Count dots to determine depth, subtract 1 since root counts as depth 0
node_depth = qualified_name.count(".")
return bool(node_depth <= self.max_depth)

def write_packages(self, diagram: PackageDiagram) -> None:
"""Write a package diagram."""
module_info: dict[str, dict[str, int]] = {}
Expand All @@ -61,6 +71,10 @@ def write_packages(self, diagram: PackageDiagram) -> None:
for module in sorted(diagram.modules(), key=lambda x: x.title):
module.fig_id = module.node.qname()

# Filter nodes based on depth
if not self.should_show_node(module.fig_id):
continue

if self.config.no_standalone and not any(
module in (rel.from_object, rel.to_object)
for rel in diagram.get_relationships("depends")
Expand All @@ -83,6 +97,10 @@ def write_packages(self, diagram: PackageDiagram) -> None:
from_id = rel.from_object.fig_id
to_id = rel.to_object.fig_id

# Filter nodes based on depth ==> skip if either source or target nodes is beyond the max depth
if not self.should_show_node(from_id) or not self.should_show_node(to_id):
continue

self.printer.emit_edge(
from_id,
to_id,
Expand All @@ -96,6 +114,10 @@ def write_packages(self, diagram: PackageDiagram) -> None:
from_id = rel.from_object.fig_id
to_id = rel.to_object.fig_id

# Filter nodes based on depth ==> skip if either source or target nodes is beyond the max depth
if not self.should_show_node(from_id) or not self.should_show_node(to_id):
continue

self.printer.emit_edge(
from_id,
to_id,
Expand All @@ -115,6 +137,11 @@ def write_classes(self, diagram: ClassDiagram) -> None:
# sorted to get predictable (hence testable) results
for obj in sorted(diagram.objects, key=lambda x: x.title):
obj.fig_id = obj.node.qname()

# Filter class based on depth setting
if not self.should_show_node(obj.fig_id):
continue

if self.config.no_standalone and not any(
obj in (rel.from_object, rel.to_object)
for rel_type in ("specialization", "association", "aggregation")
Expand All @@ -129,6 +156,12 @@ def write_classes(self, diagram: ClassDiagram) -> None:
)
# inheritance links
for rel in diagram.get_relationships("specialization"):
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
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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 logic

That 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?

Copy link
Member

Choose a reason for hiding this comment

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

Looks great !

Copy link
Contributor Author

@Julfried Julfried Jan 17, 2025 β€’

Choose a reason for hiding this comment

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

Since this is a big change, I am not sure how to approach this. Are you ok with changing it in this PR?

Copy link
Member

Choose a reason for hiding this comment

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

Right, let's open another PR for the refactor required to add the depth argument.

# Filter nodes based on depth setting
if not self.should_show_node(
rel.from_object.fig_id
) or not self.should_show_node(rel.to_object.fig_id):
continue

self.printer.emit_edge(
rel.from_object.fig_id,
rel.to_object.fig_id,
Expand All @@ -137,6 +170,12 @@ def write_classes(self, diagram: ClassDiagram) -> None:
associations: dict[str, set[str]] = defaultdict(set)
# generate associations
for rel in diagram.get_relationships("association"):
# Filter nodes based on depth setting
if not self.should_show_node(
rel.from_object.fig_id
) or not self.should_show_node(rel.to_object.fig_id):
continue

associations[rel.from_object.fig_id].add(rel.to_object.fig_id)
self.printer.emit_edge(
rel.from_object.fig_id,
Expand All @@ -146,6 +185,12 @@ def write_classes(self, diagram: ClassDiagram) -> None:
)
# generate aggregations
for rel in diagram.get_relationships("aggregation"):
# Filter nodes based on depth setting
if not self.should_show_node(
rel.from_object.fig_id
) or not self.should_show_node(rel.to_object.fig_id):
continue

if rel.to_object.fig_id in associations[rel.from_object.fig_id]:
continue
self.printer.emit_edge(
Expand Down