From 075441898cdb112324329c973756c59648cfb4db Mon Sep 17 00:00:00 2001 From: Julfried Date: Sun, 26 Jan 2025 23:02:29 +0100 Subject: [PATCH 01/39] Remove current depth filtering logic --- pylint/pyreverse/writer.py | 66 -------------------------------------- 1 file changed, 66 deletions(-) diff --git a/pylint/pyreverse/writer.py b/pylint/pyreverse/writer.py index a021b1b287..e822f67096 100644 --- a/pylint/pyreverse/writer.py +++ b/pylint/pyreverse/writer.py @@ -54,38 +54,6 @@ def write(self, diadefs: Iterable[ClassDiagram | PackageDiagram]) -> None: self.write_classes(diagram) self.save() - def should_show_node(self, qualified_name: str, is_class: bool = False) -> bool: - """Determine if a node should be shown based on depth settings. - - Depth is calculated by counting dots in the qualified name: - - depth 0: top-level packages (no dots) - - depth 1: first level sub-packages (one dot) - - depth 2: second level sub-packages (two dots) - - For classes, depth is measured from their containing module, excluding - the class name itself from the depth calculation. - """ - # If no depth limit is set ==> show all nodes - if self.max_depth is None: - return True - - # For classes, we want to measure depth from their containing module - if is_class: - # Get the module part (everything before the last dot) - last_dot = qualified_name.rfind(".") - if last_dot == -1: - module_path = "" - else: - module_path = qualified_name[:last_dot] - - # Count module depth - module_depth = module_path.count(".") - return bool(module_depth <= self.max_depth) - - # For packages/modules, count full depth - 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]] = {} @@ -94,10 +62,6 @@ 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") @@ -120,10 +84,6 @@ 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, @@ -137,10 +97,6 @@ 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, @@ -161,10 +117,6 @@ def write_classes(self, diagram: ClassDiagram) -> None: 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, is_class=True): - continue - if self.config.no_standalone and not any( obj in (rel.from_object, rel.to_object) for rel_type in ("specialization", "association", "aggregation") @@ -179,12 +131,6 @@ def write_classes(self, diagram: ClassDiagram) -> None: ) # inheritance links for rel in diagram.get_relationships("specialization"): - # Filter nodes based on depth setting - if not self.should_show_node( - rel.from_object.fig_id, is_class=True - ) or not self.should_show_node(rel.to_object.fig_id, is_class=True): - continue - self.printer.emit_edge( rel.from_object.fig_id, rel.to_object.fig_id, @@ -193,12 +139,6 @@ 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, is_class=True - ) or not self.should_show_node(rel.to_object.fig_id, is_class=True): - continue - associations[rel.from_object.fig_id].add(rel.to_object.fig_id) self.printer.emit_edge( rel.from_object.fig_id, @@ -208,12 +148,6 @@ 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, is_class=True - ) or not self.should_show_node(rel.to_object.fig_id, is_class=True): - continue - if rel.to_object.fig_id in associations[rel.from_object.fig_id]: continue self.printer.emit_edge( From 5a3bd44344a20a5b7609f6157a0107b6b4c2981e Mon Sep 17 00:00:00 2001 From: Julfried Date: Sun, 26 Jan 2025 23:18:36 +0100 Subject: [PATCH 02/39] add depth filtering logic to diadefslib --- pylint/pyreverse/diadefslib.py | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/pylint/pyreverse/diadefslib.py b/pylint/pyreverse/diadefslib.py index 59a2f59560..77f6d63bcd 100644 --- a/pylint/pyreverse/diadefslib.py +++ b/pylint/pyreverse/diadefslib.py @@ -66,6 +66,13 @@ def _set_default_options(self) -> None: def _get_levels(self) -> tuple[int, int]: """Help function for search levels.""" return self.anc_level, self.association_level + + def _should_include_by_depth(self, node: nodes.NodeNG) -> bool: + """Check if a node should be included based on depth.""" + if self.config.max_depth is None: + return True + module_depth = node.root().name.count(".") + return module_depth <= self.config.max_depth def show_node(self, node: nodes.ClassDef) -> bool: """Determine if node should be shown based on config.""" @@ -74,8 +81,9 @@ def show_node(self, node: nodes.ClassDef) -> bool: if is_stdlib_module(node.root().name): return self.config.show_stdlib # type: ignore[no-any-return] - - return True + + # Filter node by depth + return self._should_include_by_depth(node) def add_class(self, node: nodes.ClassDef) -> None: """Visit one class and add it to diagram.""" @@ -163,7 +171,7 @@ def visit_module(self, node: nodes.Module) -> None: add this class to the package diagram definition """ - if self.pkgdiagram: + if self.pkgdiagram and self._should_include_by_depth(node): self.linker.visit(node) self.pkgdiagram.add_object(node.name, node) @@ -177,7 +185,7 @@ def visit_classdef(self, node: nodes.ClassDef) -> None: def visit_importfrom(self, node: nodes.ImportFrom) -> None: """Visit astroid.ImportFrom and catch modules for package diagram.""" - if self.pkgdiagram: + if self.pkgdiagram and self._should_include_by_depth(node): self.pkgdiagram.add_from_depend(node, node.modname) From 1301d7695d279eab3c2ad6414390e34e3c32e0db Mon Sep 17 00:00:00 2001 From: Julfried Date: Sun, 26 Jan 2025 23:32:04 +0100 Subject: [PATCH 03/39] add comments --- pylint/pyreverse/diadefslib.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/pylint/pyreverse/diadefslib.py b/pylint/pyreverse/diadefslib.py index 77f6d63bcd..1627da0019 100644 --- a/pylint/pyreverse/diadefslib.py +++ b/pylint/pyreverse/diadefslib.py @@ -69,8 +69,11 @@ def _get_levels(self) -> tuple[int, int]: def _should_include_by_depth(self, node: nodes.NodeNG) -> bool: """Check if a node should be included based on depth.""" + # If max_depth is not set, include all nodes if self.config.max_depth is None: return True + + # For other nodes, calculate depth based on their root module module_depth = node.root().name.count(".") return module_depth <= self.config.max_depth From 5cb79fff332f9b5209eee62eee5fd6ecd092d401 Mon Sep 17 00:00:00 2001 From: Julfried Date: Sun, 26 Jan 2025 23:50:29 +0100 Subject: [PATCH 04/39] add type ignore comment for max_depth return value --- pylint/pyreverse/diadefslib.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/pylint/pyreverse/diadefslib.py b/pylint/pyreverse/diadefslib.py index 1627da0019..b4c31a73c3 100644 --- a/pylint/pyreverse/diadefslib.py +++ b/pylint/pyreverse/diadefslib.py @@ -66,7 +66,7 @@ def _set_default_options(self) -> None: def _get_levels(self) -> tuple[int, int]: """Help function for search levels.""" return self.anc_level, self.association_level - + def _should_include_by_depth(self, node: nodes.NodeNG) -> bool: """Check if a node should be included based on depth.""" # If max_depth is not set, include all nodes @@ -75,7 +75,7 @@ def _should_include_by_depth(self, node: nodes.NodeNG) -> bool: # For other nodes, calculate depth based on their root module module_depth = node.root().name.count(".") - return module_depth <= self.config.max_depth + return module_depth <= self.config.max_depth # type: ignore[no-any-return] def show_node(self, node: nodes.ClassDef) -> bool: """Determine if node should be shown based on config.""" @@ -84,7 +84,7 @@ def show_node(self, node: nodes.ClassDef) -> bool: if is_stdlib_module(node.root().name): return self.config.show_stdlib # type: ignore[no-any-return] - + # Filter node by depth return self._should_include_by_depth(node) From 8946c26d3d8e3d95e9bdf5b26de2111bfe4b1589 Mon Sep 17 00:00:00 2001 From: Julfried Date: Sun, 26 Jan 2025 23:51:19 +0100 Subject: [PATCH 05/39] Remove tests for legacy depth filtering logic --- tests/pyreverse/test_writer.py | 81 ---------------------------------- 1 file changed, 81 deletions(-) diff --git a/tests/pyreverse/test_writer.py b/tests/pyreverse/test_writer.py index 85596e1238..0af343b7a8 100644 --- a/tests/pyreverse/test_writer.py +++ b/tests/pyreverse/test_writer.py @@ -277,84 +277,3 @@ def test_package_name_with_slash(default_config: PyreverseConfig) -> None: writer.write([obj]) assert os.path.exists("test_package_name_with_slash_.dot") - - -def test_should_show_node_no_depth_limit(default_config: PyreverseConfig) -> None: - """Test that nodes are shown when no depth limit is set.""" - writer = DiagramWriter(default_config) - writer.max_depth = None - - assert writer.should_show_node("pkg") - assert writer.should_show_node("pkg.subpkg") - assert writer.should_show_node("pkg.subpkg.module") - assert writer.should_show_node("pkg.subpkg.module.submodule") - - -@pytest.mark.parametrize("max_depth", range(5)) -def test_should_show_node_with_depth_limit( - default_config: PyreverseConfig, max_depth: int -) -> None: - """Test that nodes are filtered correctly when depth limit is set. - - Depth counting is zero-based, determined by number of dots in path: - - 'pkg' -> depth 0 (0 dots) - - 'pkg.subpkg' -> depth 1 (1 dot) - - 'pkg.subpkg.module' -> depth 2 (2 dots) - - 'pkg.subpkg.module.submodule' -> depth 3 (3 dots) - """ - writer = DiagramWriter(default_config) - print("max_depth:", max_depth) - writer.max_depth = max_depth - - # Test cases for different depths - test_cases = [ - "pkg", - "pkg.subpkg", - "pkg.subpkg.module", - "pkg.subpkg.module.submodule", - ] - - # Test if nodes are shown based on their depth and max_depth setting - for i, path in enumerate(test_cases): - should_show = i <= max_depth - print( - f"Path {path} (depth {i}) with max_depth={max_depth} " - f"{'should show' if should_show else 'should not show'}:" - f"{writer.should_show_node(path, is_class=True)}" - ) - assert writer.should_show_node(path) == should_show - - -@pytest.mark.parametrize("max_depth", range(5)) -def test_should_show_node_classes( - default_config: PyreverseConfig, max_depth: int -) -> None: - """Test class visibility based on their containing module depth. - - Classes are filtered based on their containing module's depth: - - 'MyClass' -> depth 0 (no module) - - 'pkg.MyClass' -> depth 0 (module has no dots) - - 'pkg.subpkg.MyClass' -> depth 1 (module has 1 dot) - - 'pkg.subpkg.mod.MyClass' -> depth 2 (module has 2 dots) - """ - writer = DiagramWriter(default_config) - print("max_depth:", max_depth) - writer.max_depth = max_depth - - # Test cases for different depths - test_cases = [ - "MyClass", - "pkg.MyClass", - "pkg.subpkg.MyClass", - "pkg.subpkg.mod.MyClass", - ] - - # Test if nodes are shown based on their depth and max_depth setting - for i, path in enumerate(test_cases): - should_show = i - 1 <= max_depth # Subtract 1 to account for the class name - print( - f"Path {path} (depth {i}) with max_depth={max_depth} " - f"{'should show' if should_show else 'should not show'}:" - f"{writer.should_show_node(path, is_class=True)}" - ) - assert writer.should_show_node(path, is_class=True) == should_show From 3ea6824205fa398b78bb0e204494afded1e77492 Mon Sep 17 00:00:00 2001 From: Julfried Date: Mon, 27 Jan 2025 00:28:27 +0100 Subject: [PATCH 06/39] Add fixtures for a mocked node and DiaDefGenerator --- tests/pyreverse/test_diadefs.py | 21 ++++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-) diff --git a/tests/pyreverse/test_diadefs.py b/tests/pyreverse/test_diadefs.py index 06affe12e5..7a9d65d568 100644 --- a/tests/pyreverse/test_diadefs.py +++ b/tests/pyreverse/test_diadefs.py @@ -8,8 +8,9 @@ from __future__ import annotations -from collections.abc import Iterator +from collections.abc import Callable, Iterator from pathlib import Path +from unittest.mock import Mock import pytest from astroid import extract_node, nodes @@ -58,6 +59,24 @@ def PROJECT(get_project: GetProjectCallable) -> Iterator[Project]: yield get_project("data") +# Fixture for creating mocked nodes +@pytest.fixture +def mock_node() -> Callable[[str], Mock]: + def _mock_node(module_path: str) -> Mock: + """Create a mocked node with a given module path.""" + node = Mock() + node.root.return_value.name = module_path + return node + + return _mock_node + + +# Fixture for the DiaDefGenerator +@pytest.fixture +def generator(HANDLER: DiadefsHandler, PROJECT: Project) -> DiaDefGenerator: + return DiaDefGenerator(Linker(PROJECT), HANDLER) + + def test_option_values( default_config: PyreverseConfig, HANDLER: DiadefsHandler, PROJECT: Project ) -> None: From 55815dfdcab14c9d8a8ec048ef6f217de150fb27 Mon Sep 17 00:00:00 2001 From: Julfried Date: Mon, 27 Jan 2025 00:39:44 +0100 Subject: [PATCH 07/39] Add tests for _should_include_by_depth --- tests/pyreverse/test_diadefs.py | 53 +++++++++++++++++++++++++++++++++ 1 file changed, 53 insertions(+) diff --git a/tests/pyreverse/test_diadefs.py b/tests/pyreverse/test_diadefs.py index 7a9d65d568..807f9247fd 100644 --- a/tests/pyreverse/test_diadefs.py +++ b/tests/pyreverse/test_diadefs.py @@ -276,3 +276,56 @@ def test_regression_dataclasses_inference( special = "regrtest_data.dataclasses_pyreverse.InventoryItem" cd = cdg.class_diagram(path, special) assert cd.title == special + + +# Test for no depth limit +def test_should_include_by_depth_no_limit( + generator: DiaDefGenerator, mock_node: Mock +) -> None: + """Test that nodes are included when no depth limit is set.""" + generator.config.max_depth = None + + # Create mocked nodes with different depths + node1 = mock_node("pkg") # Depth 0 + node2 = mock_node("pkg.subpkg") # Depth 1 + node3 = mock_node("pkg.subpkg.module") # Depth 2 + + # All nodes should be included when max_depth is None + assert generator._should_include_by_depth(node1) + assert generator._should_include_by_depth(node2) + assert generator._should_include_by_depth(node3) + + +@pytest.mark.parametrize("max_depth", range(5)) +def test_should_include_by_depth_with_limit( + generator: DiaDefGenerator, mock_node: Mock, max_depth: int +) -> None: + """Test that nodes are filtered correctly when depth limit is set. + + Depth counting is zero-based, determined by number of dots in path: + - 'pkg' -> depth 0 (0 dots) + - 'pkg.subpkg' -> depth 1 (1 dot) + - 'pkg.subpkg.module' -> depth 2 (2 dots) + - 'pkg.subpkg.module.submodule' -> depth 3 (3 dots) + """ + # Set generator config + generator.config.max_depth = max_depth + + # Test cases for different depths + test_cases = [ + "pkg", + "pkg.subpkg", + "pkg.subpkg.module", + "pkg.subpkg.module.submodule", + ] + nodes = [mock_node(path) for path in test_cases] + + # Test if nodes are shown based on their depth and max_depth setting + for i, node in enumerate(nodes): + should_show = i <= max_depth + print( + f"Node {node.root.return_value.name} (depth {i}) with max_depth={max_depth} " + f"{'should show' if should_show else 'should not show'}:" + f"{generator._should_include_by_depth(node)}" + ) + assert generator._should_include_by_depth(node) == should_show From ca4f2dfda77d51af7af13ae56a45fd8e625b7646 Mon Sep 17 00:00:00 2001 From: Julian Grimm <51880314+Julfried@users.noreply.github.com> Date: Wed, 29 Jan 2025 21:30:37 +0100 Subject: [PATCH 08/39] Apply suggestions from code review Co-authored-by: Pierre Sassoulas --- pylint/pyreverse/diadefslib.py | 10 +++------- tests/pyreverse/test_diadefs.py | 8 ++------ 2 files changed, 5 insertions(+), 13 deletions(-) diff --git a/pylint/pyreverse/diadefslib.py b/pylint/pyreverse/diadefslib.py index b4c31a73c3..b7e88016a9 100644 --- a/pylint/pyreverse/diadefslib.py +++ b/pylint/pyreverse/diadefslib.py @@ -69,13 +69,9 @@ def _get_levels(self) -> tuple[int, int]: def _should_include_by_depth(self, node: nodes.NodeNG) -> bool: """Check if a node should be included based on depth.""" - # If max_depth is not set, include all nodes - if self.config.max_depth is None: - return True - - # For other nodes, calculate depth based on their root module - module_depth = node.root().name.count(".") - return module_depth <= self.config.max_depth # type: ignore[no-any-return] + # If max_depth is not set, include all nodes, otherwise, calculate depth based on + # node's root module + return self.config.max_depth is None or bool(node.root().name.count(".") <= self.config.max_depth) def show_node(self, node: nodes.ClassDef) -> bool: """Determine if node should be shown based on config.""" diff --git a/tests/pyreverse/test_diadefs.py b/tests/pyreverse/test_diadefs.py index 807f9247fd..fa273426af 100644 --- a/tests/pyreverse/test_diadefs.py +++ b/tests/pyreverse/test_diadefs.py @@ -278,7 +278,6 @@ def test_regression_dataclasses_inference( assert cd.title == special -# Test for no depth limit def test_should_include_by_depth_no_limit( generator: DiaDefGenerator, mock_node: Mock ) -> None: @@ -308,10 +307,7 @@ def test_should_include_by_depth_with_limit( - 'pkg.subpkg.module' -> depth 2 (2 dots) - 'pkg.subpkg.module.submodule' -> depth 3 (3 dots) """ - # Set generator config generator.config.max_depth = max_depth - - # Test cases for different depths test_cases = [ "pkg", "pkg.subpkg", @@ -323,9 +319,9 @@ def test_should_include_by_depth_with_limit( # Test if nodes are shown based on their depth and max_depth setting for i, node in enumerate(nodes): should_show = i <= max_depth - print( + msg = ( f"Node {node.root.return_value.name} (depth {i}) with max_depth={max_depth} " f"{'should show' if should_show else 'should not show'}:" f"{generator._should_include_by_depth(node)}" ) - assert generator._should_include_by_depth(node) == should_show + assert generator._should_include_by_depth(node) == should_show, msg From 369e19dbf5cbf15616008023eae1d941eb2db712 Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Wed, 29 Jan 2025 20:31:40 +0000 Subject: [PATCH 09/39] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- pylint/pyreverse/diadefslib.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/pylint/pyreverse/diadefslib.py b/pylint/pyreverse/diadefslib.py index b7e88016a9..9aaeee2d7e 100644 --- a/pylint/pyreverse/diadefslib.py +++ b/pylint/pyreverse/diadefslib.py @@ -69,9 +69,11 @@ def _get_levels(self) -> tuple[int, int]: def _should_include_by_depth(self, node: nodes.NodeNG) -> bool: """Check if a node should be included based on depth.""" - # If max_depth is not set, include all nodes, otherwise, calculate depth based on + # If max_depth is not set, include all nodes, otherwise, calculate depth based on # node's root module - return self.config.max_depth is None or bool(node.root().name.count(".") <= self.config.max_depth) + return self.config.max_depth is None or bool( + node.root().name.count(".") <= self.config.max_depth + ) def show_node(self, node: nodes.ClassDef) -> bool: """Determine if node should be shown based on config.""" From 826bcca72c3b080e303171e799e46e7b5153320c Mon Sep 17 00:00:00 2001 From: Julfried <51880314+Julfried@users.noreply.github.com> Date: Fri, 7 Feb 2025 21:24:53 +0100 Subject: [PATCH 10/39] Modify argument flow to also pass args to DiadefsHandler --- pylint/pyreverse/diadefslib.py | 5 +++-- pylint/pyreverse/main.py | 2 +- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/pylint/pyreverse/diadefslib.py b/pylint/pyreverse/diadefslib.py index 9aaeee2d7e..2c28959639 100644 --- a/pylint/pyreverse/diadefslib.py +++ b/pylint/pyreverse/diadefslib.py @@ -7,7 +7,7 @@ from __future__ import annotations import argparse -from collections.abc import Generator +from collections.abc import Generator, Sequence from typing import Any import astroid @@ -217,8 +217,9 @@ def class_diagram(self, project: Project, klass: nodes.ClassDef) -> ClassDiagram class DiadefsHandler: """Get diagram definitions from user (i.e. xml files) or generate them.""" - def __init__(self, config: argparse.Namespace) -> None: + def __init__(self, config: argparse.Namespace, args: Sequence[str]) -> None: self.config = config + self.args = args def get_diadefs(self, project: Project, linker: Linker) -> list[ClassDiagram]: """Get the diagram's configuration data. diff --git a/pylint/pyreverse/main.py b/pylint/pyreverse/main.py index f297856c63..7eba7739c2 100644 --- a/pylint/pyreverse/main.py +++ b/pylint/pyreverse/main.py @@ -354,7 +354,7 @@ def run(self) -> int: verbose=self.config.verbose, ) linker = Linker(project, tag=True) - handler = DiadefsHandler(self.config) + handler = DiadefsHandler(self.config, self.args) diadefs = handler.get_diadefs(project, linker) writer.DiagramWriter(self.config).write(diadefs) return 0 From a431ca421c6b16e79e342f3c817742d0e903ee3c Mon Sep 17 00:00:00 2001 From: Julfried <51880314+Julfried@users.noreply.github.com> Date: Fri, 7 Feb 2025 23:10:02 +0100 Subject: [PATCH 11/39] Fix diadefs tests --- tests/pyreverse/test_diadefs.py | 34 ++++++++++++++++++++++----------- 1 file changed, 23 insertions(+), 11 deletions(-) diff --git a/tests/pyreverse/test_diadefs.py b/tests/pyreverse/test_diadefs.py index fa273426af..ab4ebe58d3 100644 --- a/tests/pyreverse/test_diadefs.py +++ b/tests/pyreverse/test_diadefs.py @@ -8,7 +8,7 @@ from __future__ import annotations -from collections.abc import Callable, Iterator +from collections.abc import Callable, Iterator, Sequence from pathlib import Path from unittest.mock import Mock @@ -49,8 +49,10 @@ def _process_relations( @pytest.fixture -def HANDLER(default_config: PyreverseConfig) -> DiadefsHandler: - return DiadefsHandler(default_config) +def HANDLER( + default_config: PyreverseConfig, default_args: Sequence[str] +) -> DiadefsHandler: + return DiadefsHandler(config=default_config, args=default_args) @pytest.fixture(scope="module") @@ -78,13 +80,16 @@ def generator(HANDLER: DiadefsHandler, PROJECT: Project) -> DiaDefGenerator: def test_option_values( - default_config: PyreverseConfig, HANDLER: DiadefsHandler, PROJECT: Project + default_config: PyreverseConfig, + default_args: Sequence[str], + HANDLER: DiadefsHandler, + PROJECT: Project, ) -> None: """Test for ancestor, associated and module options.""" df_h = DiaDefGenerator(Linker(PROJECT), HANDLER) cl_config = default_config cl_config.classes = ["Specialization"] - cl_h = DiaDefGenerator(Linker(PROJECT), DiadefsHandler(cl_config)) + cl_h = DiaDefGenerator(Linker(PROJECT), DiadefsHandler(config=cl_config, args=[])) assert df_h._get_levels() == (0, 0) assert not df_h.module_names assert cl_h._get_levels() == (-1, -1) @@ -96,11 +101,13 @@ def test_option_values( hndl._set_default_options() assert hndl._get_levels() == (-1, -1) assert hndl.module_names - handler = DiadefsHandler(default_config) + handler = DiadefsHandler(config=default_config, args=default_args) df_h = DiaDefGenerator(Linker(PROJECT), handler) cl_config = default_config cl_config.classes = ["Specialization"] - cl_h = DiaDefGenerator(Linker(PROJECT), DiadefsHandler(cl_config)) + cl_h = DiaDefGenerator( + Linker(PROJECT), DiadefsHandler(config=cl_config, args=default_args) + ) for hndl in (df_h, cl_h): hndl.config.show_ancestors = 2 hndl.config.show_associated = 1 @@ -127,7 +134,8 @@ class CustomDict(collections.OrderedDict): ) config = PyreverseConfig() - dd_gen = DiaDefGenerator(Linker(PROJECT), DiadefsHandler(config)) + args: Sequence[str] = [] + dd_gen = DiaDefGenerator(Linker(PROJECT), DiadefsHandler(config, args)) # Default behavior assert not list(dd_gen.get_ancestors(example, 1)) @@ -147,7 +155,8 @@ class CustomError(Exception): ) config = PyreverseConfig() - dd_gen = DiaDefGenerator(Linker(PROJECT), DiadefsHandler(config)) + args: Sequence[str] = [] + dd_gen = DiaDefGenerator(Linker(PROJECT), DiadefsHandler(config, args)) # Default behavior assert not list(dd_gen.get_ancestors(example, 1)) @@ -175,7 +184,10 @@ def test_extract_relations(self, HANDLER: DiadefsHandler, PROJECT: Project) -> N assert relations == self._should_rels def test_functional_relation_extraction( - self, default_config: PyreverseConfig, get_project: GetProjectCallable + self, + default_config: PyreverseConfig, + default_args: Sequence[str], + get_project: GetProjectCallable, ) -> None: """Functional test of relations extraction; different classes possibly in different modules. @@ -183,7 +195,7 @@ def test_functional_relation_extraction( # XXX should be catching pyreverse environment problem but doesn't # pyreverse doesn't extract the relations but this test ok project = get_project("data") - handler = DiadefsHandler(default_config) + handler = DiadefsHandler(default_config, default_args) diadefs = handler.get_diadefs(project, Linker(project, tag=True)) cd = diadefs[1] relations = _process_relations(cd.relationships) From 25877bbbc90f253914b5bae48adb478b85de09c7 Mon Sep 17 00:00:00 2001 From: Julfried <51880314+Julfried@users.noreply.github.com> Date: Fri, 7 Feb 2025 23:10:54 +0100 Subject: [PATCH 12/39] Add fixture for default args --- tests/pyreverse/conftest.py | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/tests/pyreverse/conftest.py b/tests/pyreverse/conftest.py index 2b05d4403b..fa8674ee27 100644 --- a/tests/pyreverse/conftest.py +++ b/tests/pyreverse/conftest.py @@ -4,7 +4,7 @@ from __future__ import annotations -from collections.abc import Callable +from collections.abc import Callable, Sequence import pytest from astroid.nodes.scoped_nodes import Module @@ -15,8 +15,15 @@ from pylint.typing import GetProjectCallable +@pytest.fixture() +def default_args() -> Sequence[str]: + """Provides default command-line arguments for tests.""" + return [] + + @pytest.fixture() def default_config() -> PyreverseConfig: + """Provides default configuration for tests.""" return PyreverseConfig() From 6d508579eb171bccd3f143af50425aed77bdaf2f Mon Sep 17 00:00:00 2001 From: Julfried <51880314+Julfried@users.noreply.github.com> Date: Fri, 7 Feb 2025 23:11:52 +0100 Subject: [PATCH 13/39] Fix diagram tests --- tests/pyreverse/test_diagrams.py | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/tests/pyreverse/test_diagrams.py b/tests/pyreverse/test_diagrams.py index 761aa06923..eb47bf2653 100644 --- a/tests/pyreverse/test_diagrams.py +++ b/tests/pyreverse/test_diagrams.py @@ -6,6 +6,8 @@ from __future__ import annotations +from collections.abc import Sequence + from pylint.pyreverse.diadefslib import DefaultDiadefGenerator, DiadefsHandler from pylint.pyreverse.inspector import Linker from pylint.testutils.pyreverse import PyreverseConfig @@ -13,11 +15,13 @@ def test_property_handling( - default_config: PyreverseConfig, get_project: GetProjectCallable + default_config: PyreverseConfig, + default_args: Sequence[str], + get_project: GetProjectCallable, ) -> None: project = get_project("data.property_pattern") class_diagram = DefaultDiadefGenerator( - Linker(project), DiadefsHandler(default_config) + Linker(project), DiadefsHandler(default_config, default_args) ).visit(project)[0] obj = class_diagram.classe("PropertyPatterns") assert len(class_diagram.get_methods(obj.node)) == 0 From 82ab7cd78e7cd625cb886f57e0af08ed38297e5b Mon Sep 17 00:00:00 2001 From: Julfried <51880314+Julfried@users.noreply.github.com> Date: Fri, 7 Feb 2025 23:12:30 +0100 Subject: [PATCH 14/39] Fix writer tests --- tests/pyreverse/test_writer.py | 63 ++++++++++++++++++++++------------ 1 file changed, 42 insertions(+), 21 deletions(-) diff --git a/tests/pyreverse/test_writer.py b/tests/pyreverse/test_writer.py index 0af343b7a8..3f828b75b0 100644 --- a/tests/pyreverse/test_writer.py +++ b/tests/pyreverse/test_writer.py @@ -8,7 +8,7 @@ import codecs import os -from collections.abc import Iterator +from collections.abc import Iterator, Sequence from difflib import unified_diff from pathlib import Path from unittest.mock import Mock @@ -84,34 +84,42 @@ def change_to_temp_dir(monkeypatch: MonkeyPatch, tmp_path: Path) -> None: @pytest.fixture() def setup_dot( - default_config: PyreverseConfig, get_project: GetProjectCallable + default_config: PyreverseConfig, + default_args: Sequence[str], + get_project: GetProjectCallable, ) -> Iterator[None]: writer = DiagramWriter(default_config) project = get_project(TEST_DATA_DIR) - yield from _setup(project, default_config, writer) + yield from _setup(project, default_config, default_args, writer) @pytest.fixture() def setup_colorized_dot( - colorized_dot_config: PyreverseConfig, get_project: GetProjectCallable + colorized_dot_config: PyreverseConfig, + default_args: Sequence[str], + get_project: GetProjectCallable, ) -> Iterator[None]: writer = DiagramWriter(colorized_dot_config) project = get_project(TEST_DATA_DIR, name="colorized") - yield from _setup(project, colorized_dot_config, writer) + yield from _setup(project, colorized_dot_config, default_args, writer) @pytest.fixture() def setup_no_standalone_dot( - no_standalone_dot_config: PyreverseConfig, get_project: GetProjectCallable + no_standalone_dot_config: PyreverseConfig, + default_args: Sequence[str], + get_project: GetProjectCallable, ) -> Iterator[None]: writer = DiagramWriter(no_standalone_dot_config) project = get_project(TEST_DATA_DIR, name="no_standalone") - yield from _setup(project, no_standalone_dot_config, writer) + yield from _setup(project, no_standalone_dot_config, default_args, writer) @pytest.fixture() def setup_type_check_imports_dot( - default_config: PyreverseConfig, get_project: GetProjectCallable + default_config: PyreverseConfig, + default_args: Sequence[str], + get_project: GetProjectCallable, ) -> Iterator[None]: writer = DiagramWriter(default_config) project = get_project( @@ -119,64 +127,77 @@ def setup_type_check_imports_dot( name="type_check_imports", ) - yield from _setup(project, default_config, writer) + yield from _setup(project, default_config, default_args, writer) @pytest.fixture() def setup_puml( - puml_config: PyreverseConfig, get_project: GetProjectCallable + puml_config: PyreverseConfig, + default_args: Sequence[str], + get_project: GetProjectCallable, ) -> Iterator[None]: writer = DiagramWriter(puml_config) project = get_project(TEST_DATA_DIR) - yield from _setup(project, puml_config, writer) + yield from _setup(project, puml_config, default_args, writer) @pytest.fixture() def setup_colorized_puml( - colorized_puml_config: PyreverseConfig, get_project: GetProjectCallable + colorized_puml_config: PyreverseConfig, + default_args: Sequence[str], + get_project: GetProjectCallable, ) -> Iterator[None]: writer = DiagramWriter(colorized_puml_config) project = get_project(TEST_DATA_DIR, name="colorized") - yield from _setup(project, colorized_puml_config, writer) + yield from _setup(project, colorized_puml_config, default_args, writer) @pytest.fixture() def setup_mmd( - mmd_config: PyreverseConfig, get_project: GetProjectCallable + mmd_config: PyreverseConfig, + default_args: Sequence[str], + get_project: GetProjectCallable, ) -> Iterator[None]: writer = DiagramWriter(mmd_config) project = get_project(TEST_DATA_DIR) - yield from _setup(project, mmd_config, writer) + yield from _setup(project, mmd_config, default_args, writer) @pytest.fixture() def setup_html( - html_config: PyreverseConfig, get_project: GetProjectCallable + html_config: PyreverseConfig, + default_args: Sequence[str], + get_project: GetProjectCallable, ) -> Iterator[None]: writer = DiagramWriter(html_config) project = get_project(TEST_DATA_DIR) - yield from _setup(project, html_config, writer) + yield from _setup(project, html_config, default_args, writer) @pytest.fixture() def setup_depth_limited( - depth_limited_config: PyreverseConfig, get_project: GetProjectCallable + depth_limited_config: PyreverseConfig, + default_args: Sequence[str], + get_project: GetProjectCallable, ) -> Iterator[None]: writer = DiagramWriter(depth_limited_config) project = get_project( TEST_DATA_DIR, name=f"depth_limited_{depth_limited_config.max_depth}" ) - yield from _setup(project, depth_limited_config, writer) + yield from _setup(project, depth_limited_config, default_args, writer) def _setup( - project: Project, config: PyreverseConfig, writer: DiagramWriter + project: Project, + config: PyreverseConfig, + args: Sequence[str], + writer: DiagramWriter, ) -> Iterator[None]: linker = Linker(project) - handler = DiadefsHandler(config) + handler = DiadefsHandler(config, args) dd = DefaultDiadefGenerator(linker, handler).visit(project) for diagram in dd: diagram.extract_relationships() From 8add6cb583ab091b2195f004128b6b1398a200c5 Mon Sep 17 00:00:00 2001 From: Julfried <51880314+Julfried@users.noreply.github.com> Date: Sat, 8 Feb 2025 00:58:33 +0100 Subject: [PATCH 15/39] Implement depth limiting relative to the shallowest specified package for consistent grouping --- pylint/pyreverse/diadefslib.py | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/pylint/pyreverse/diadefslib.py b/pylint/pyreverse/diadefslib.py index 2c28959639..ce05c1515d 100644 --- a/pylint/pyreverse/diadefslib.py +++ b/pylint/pyreverse/diadefslib.py @@ -27,6 +27,7 @@ class DiaDefGenerator: def __init__(self, linker: Linker, handler: DiadefsHandler) -> None: """Common Diagram Handler initialization.""" self.config = handler.config + self.args = handler.args self.module_names: bool = False self._set_default_options() self.linker = linker @@ -69,11 +70,17 @@ def _get_levels(self) -> tuple[int, int]: def _should_include_by_depth(self, node: nodes.NodeNG) -> bool: """Check if a node should be included based on depth.""" - # If max_depth is not set, include all nodes, otherwise, calculate depth based on - # node's root module - return self.config.max_depth is None or bool( - node.root().name.count(".") <= self.config.max_depth + # If max_depth is not set, include all nodes + if self.config.max_depth is None: + return True + + # Check based on relative depth + abs_depth = node.root().name.count(".") + base_depth = min( + (arg.count(".") for arg in self.args if node.root().name.startswith(arg)), + default=abs_depth, # Fallback to absolute depth if no other packages are found ) + return bool((abs_depth - base_depth) <= self.config.max_depth) def show_node(self, node: nodes.ClassDef) -> bool: """Determine if node should be shown based on config.""" From fcec1e54c17c1bce22246ff9d8720e11bb8419db Mon Sep 17 00:00:00 2001 From: Julfried <51880314+Julfried@users.noreply.github.com> Date: Sat, 8 Feb 2025 01:05:42 +0100 Subject: [PATCH 16/39] Modify default args to pass writer tests --- tests/pyreverse/conftest.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/pyreverse/conftest.py b/tests/pyreverse/conftest.py index fa8674ee27..406b316f24 100644 --- a/tests/pyreverse/conftest.py +++ b/tests/pyreverse/conftest.py @@ -18,7 +18,7 @@ @pytest.fixture() def default_args() -> Sequence[str]: """Provides default command-line arguments for tests.""" - return [] + return ["data"] @pytest.fixture() From f2f806e37058299eb6484c426491d8c33ab84d8b Mon Sep 17 00:00:00 2001 From: Julfried <51880314+Julfried@users.noreply.github.com> Date: Sat, 8 Feb 2025 01:12:27 +0100 Subject: [PATCH 17/39] Update diadefs tests --- tests/pyreverse/test_diadefs.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/pyreverse/test_diadefs.py b/tests/pyreverse/test_diadefs.py index ab4ebe58d3..8043015feb 100644 --- a/tests/pyreverse/test_diadefs.py +++ b/tests/pyreverse/test_diadefs.py @@ -320,6 +320,7 @@ def test_should_include_by_depth_with_limit( - 'pkg.subpkg.module.submodule' -> depth 3 (3 dots) """ generator.config.max_depth = max_depth + generator.args = "pkg" # Specify root package test_cases = [ "pkg", "pkg.subpkg", From 11d109b87a7d81658102d39d438db43d3ce41ab7 Mon Sep 17 00:00:00 2001 From: Julfried <51880314+Julfried@users.noreply.github.com> Date: Sat, 8 Feb 2025 01:20:23 +0100 Subject: [PATCH 18/39] Add tests for relative depth limiting --- tests/pyreverse/test_diadefs.py | 95 +++++++++++++++++++++++++++++---- 1 file changed, 86 insertions(+), 9 deletions(-) diff --git a/tests/pyreverse/test_diadefs.py b/tests/pyreverse/test_diadefs.py index 8043015feb..97b1699d02 100644 --- a/tests/pyreverse/test_diadefs.py +++ b/tests/pyreverse/test_diadefs.py @@ -308,19 +308,19 @@ def test_should_include_by_depth_no_limit( @pytest.mark.parametrize("max_depth", range(5)) -def test_should_include_by_depth_with_limit( +def test_should_include_by_depth_absolute( generator: DiaDefGenerator, mock_node: Mock, max_depth: int ) -> None: - """Test that nodes are filtered correctly when depth limit is set. + """Test absolute depth filtering (no relative package logic). - Depth counting is zero-based, determined by number of dots in path: - - 'pkg' -> depth 0 (0 dots) - - 'pkg.subpkg' -> depth 1 (1 dot) - - 'pkg.subpkg.module' -> depth 2 (2 dots) - - 'pkg.subpkg.module.submodule' -> depth 3 (3 dots) + - 'pkg' -> depth 0 + - 'pkg.subpkg' -> depth 1 + - 'pkg.subpkg.module' -> depth 2 + - 'pkg.subpkg.module.submodule' -> depth 3 """ generator.config.max_depth = max_depth - generator.args = "pkg" # Specify root package + generator.args = ["pkg"] # Single root package + test_cases = [ "pkg", "pkg.subpkg", @@ -329,7 +329,6 @@ def test_should_include_by_depth_with_limit( ] nodes = [mock_node(path) for path in test_cases] - # Test if nodes are shown based on their depth and max_depth setting for i, node in enumerate(nodes): should_show = i <= max_depth msg = ( @@ -338,3 +337,81 @@ def test_should_include_by_depth_with_limit( f"{generator._should_include_by_depth(node)}" ) assert generator._should_include_by_depth(node) == should_show, msg + + +@pytest.mark.parametrize("max_depth", range(5)) +@pytest.mark.parametrize( + "specified_package", [["pkg"], ["pkg.subpkg"], ["pkg.subpkg.module"]] +) +def test_should_include_by_depth_relative_single_package( + generator: DiaDefGenerator, + mock_node: Mock, + max_depth: int, + specified_package: list[str], +) -> None: + """Test relative depth filtering when only one package is specified. + + Each test case ensures that depth is calculated **relative** to the specified package. + """ + generator.config.max_depth = max_depth + generator.args = specified_package # Only one package is specified + + test_cases = [ + "pkg", + "pkg.subpkg", + "pkg.subpkg.module", + "pkg.subpkg.module.submodule", + ] + nodes = [mock_node(path) for path in test_cases] + + base_depth = specified_package[0].count(".") + + for node in nodes: + abs_depth = node.root.return_value.name.count(".") + relative_depth = abs_depth - base_depth + should_show = relative_depth <= max_depth + + msg = ( + f"Node {node.root.return_value.name} (abs_depth {abs_depth}, base_depth {base_depth}, " + f"relative_depth {relative_depth}) with max_depth={max_depth} " + f"{'should show' if should_show else 'should not show'}:" + f"{generator._should_include_by_depth(node)}" + ) + assert generator._should_include_by_depth(node) == should_show, msg + + +@pytest.mark.parametrize("max_depth", range(5)) +def test_should_include_by_depth_relative_multiple_packages( + generator: DiaDefGenerator, mock_node: Mock, max_depth: int +) -> None: + """Test relative depth filtering when multiple packages are specified. + + The base depth should be determined by the **shallowest** specified package. + """ + specified_packages = ["pkg", "pkg.subpkg"] + generator.config.max_depth = max_depth + generator.args = specified_packages # Multiple packages specified ==> depth relative to the shallowest + + test_cases = [ + "pkg", + "pkg.subpkg", + "pkg.subpkg.module", + "pkg.subpkg.module.submodule", + ] + nodes = [mock_node(path) for path in test_cases] + + # Compute base depth relative to the shallowest specified package + base_depth = min(arg.count(".") for arg in specified_packages) + + for node in nodes: + abs_depth = node.root.return_value.name.count(".") + relative_depth = abs_depth - base_depth + should_show = relative_depth <= max_depth + + msg = ( + f"Node {node.root.return_value.name} (abs_depth {abs_depth}, base_depth {base_depth}, " + f"relative_depth {relative_depth}) with max_depth={max_depth} " + f"{'should show' if should_show else 'should not show'}:" + f"{generator._should_include_by_depth(node)}" + ) + assert generator._should_include_by_depth(node) == should_show, msg From e767914cbf0e9d22dc2cb462ada6dd20aa88f6e1 Mon Sep 17 00:00:00 2001 From: Julfried <51880314+Julfried@users.noreply.github.com> Date: Sat, 22 Feb 2025 14:53:10 +0100 Subject: [PATCH 19/39] Precompute node depths in costructor of DiaDefGenerator --- pylint/pyreverse/diadefslib.py | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/pylint/pyreverse/diadefslib.py b/pylint/pyreverse/diadefslib.py index ce05c1515d..6c3259c791 100644 --- a/pylint/pyreverse/diadefslib.py +++ b/pylint/pyreverse/diadefslib.py @@ -32,6 +32,7 @@ def __init__(self, linker: Linker, handler: DiadefsHandler) -> None: self._set_default_options() self.linker = linker self.classdiagram: ClassDiagram # defined by subclasses + self.args_depths = {arg: arg.count(".") for arg in self.args} def get_title(self, node: nodes.ClassDef) -> str: """Get title for objects.""" @@ -76,8 +77,14 @@ def _should_include_by_depth(self, node: nodes.NodeNG) -> bool: # Check based on relative depth abs_depth = node.root().name.count(".") - base_depth = min( - (arg.count(".") for arg in self.args if node.root().name.startswith(arg)), + + # Select the base depth to compare against + base_depth = max( + ( + value + for key, value in self.args_depths.items() + if node.root().name.startswith(key) + ), default=abs_depth, # Fallback to absolute depth if no other packages are found ) return bool((abs_depth - base_depth) <= self.config.max_depth) From 2f152241c8505a04413e7024c5469e42030d8a98 Mon Sep 17 00:00:00 2001 From: Julfried <51880314+Julfried@users.noreply.github.com> Date: Sat, 22 Feb 2025 15:00:16 +0100 Subject: [PATCH 20/39] Only pre-calculate argument depths if a max_depth is specified --- pylint/pyreverse/diadefslib.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/pylint/pyreverse/diadefslib.py b/pylint/pyreverse/diadefslib.py index 6c3259c791..5d449fd54b 100644 --- a/pylint/pyreverse/diadefslib.py +++ b/pylint/pyreverse/diadefslib.py @@ -32,7 +32,9 @@ def __init__(self, linker: Linker, handler: DiadefsHandler) -> None: self._set_default_options() self.linker = linker self.classdiagram: ClassDiagram # defined by subclasses - self.args_depths = {arg: arg.count(".") for arg in self.args} + # Only pre-calculate depths if user has requested a max_depth + if handler.config.max_depth is not None: + self.args_depths = {arg: arg.count(".") for arg in self.args} def get_title(self, node: nodes.ClassDef) -> str: """Get title for objects.""" From 903116b426fd0ac7a00351265be5b11a256739c1 Mon Sep 17 00:00:00 2001 From: Julfried <51880314+Julfried@users.noreply.github.com> Date: Sat, 22 Feb 2025 15:40:24 +0100 Subject: [PATCH 21/39] Add a function to detect leaf nodes in args --- pylint/pyreverse/diadefslib.py | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/pylint/pyreverse/diadefslib.py b/pylint/pyreverse/diadefslib.py index 5d449fd54b..28dcd81f4e 100644 --- a/pylint/pyreverse/diadefslib.py +++ b/pylint/pyreverse/diadefslib.py @@ -43,6 +43,22 @@ def get_title(self, node: nodes.ClassDef) -> str: title = f"{node.root().name}.{title}" return title # type: ignore[no-any-return] + def get_leaf_nodes(self, module_names: Sequence[str]) -> list[str]: + """ + Given a list of module/package names, return only the leaf nodes. + + A leaf node is one that is not a prefix (with an extra dot) of any other node. + """ + leaf_nodes = [ + module + for module in module_names + if not any( + other != module and other.startswith(module + ".") + for other in module_names + ) + ] + return leaf_nodes + def _set_option(self, option: bool | None) -> bool: """Activate some options if not explicitly deactivated.""" # if we have a class diagram, we want more information by default; From 6b30097a4a41e91b634fc4d7eedb6c84b0f0d97e Mon Sep 17 00:00:00 2001 From: Julfried <51880314+Julfried@users.noreply.github.com> Date: Sat, 22 Feb 2025 15:41:33 +0100 Subject: [PATCH 22/39] Compute package depths of the specified args based on the determined leaf nodes --- pylint/pyreverse/diadefslib.py | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/pylint/pyreverse/diadefslib.py b/pylint/pyreverse/diadefslib.py index 28dcd81f4e..884c7e409f 100644 --- a/pylint/pyreverse/diadefslib.py +++ b/pylint/pyreverse/diadefslib.py @@ -34,7 +34,9 @@ def __init__(self, linker: Linker, handler: DiadefsHandler) -> None: self.classdiagram: ClassDiagram # defined by subclasses # Only pre-calculate depths if user has requested a max_depth if handler.config.max_depth is not None: - self.args_depths = {arg: arg.count(".") for arg in self.args} + # Detect which of the args are leaf nodes + leaf_nodes = self.get_leaf_nodes(self.args) + self.args_depths = {module: module.count(".") for module in leaf_nodes} def get_title(self, node: nodes.ClassDef) -> str: """Get title for objects.""" @@ -93,19 +95,17 @@ def _should_include_by_depth(self, node: nodes.NodeNG) -> bool: if self.config.max_depth is None: return True - # Check based on relative depth - abs_depth = node.root().name.count(".") - - # Select the base depth to compare against - base_depth = max( - ( - value - for key, value in self.args_depths.items() - if node.root().name.startswith(key) - ), - default=abs_depth, # Fallback to absolute depth if no other packages are found + # Calculate the absolute depth of the node + name = node.root().name + abs_depth = name.count(".") + + # Retrieve the base depth to compare against + rel_depth = next( + (v for k, v in self.args_depths.items() if name.startswith(k)), None + ) + return rel_depth is not None and bool( + (abs_depth - rel_depth) <= self.config.max_depth ) - return bool((abs_depth - base_depth) <= self.config.max_depth) def show_node(self, node: nodes.ClassDef) -> bool: """Determine if node should be shown based on config.""" From a25237f8fded8c3a92c84573c156198f881a5cf2 Mon Sep 17 00:00:00 2001 From: Julfried <51880314+Julfried@users.noreply.github.com> Date: Sat, 22 Feb 2025 19:09:20 +0100 Subject: [PATCH 23/39] Emit a warning if user specifies non leaf nodes --- pylint/pyreverse/diadefslib.py | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/pylint/pyreverse/diadefslib.py b/pylint/pyreverse/diadefslib.py index 884c7e409f..0eda915664 100644 --- a/pylint/pyreverse/diadefslib.py +++ b/pylint/pyreverse/diadefslib.py @@ -7,6 +7,7 @@ from __future__ import annotations import argparse +import logging from collections.abc import Generator, Sequence from typing import Any @@ -36,6 +37,19 @@ def __init__(self, linker: Linker, handler: DiadefsHandler) -> None: if handler.config.max_depth is not None: # Detect which of the args are leaf nodes leaf_nodes = self.get_leaf_nodes(self.args) + + # Emit a warning if any of the args are not leaf nodes + diff = set(self.args).difference(set(leaf_nodes)) + if len(diff) > 0: + logging.warning( + "Detected nested names within the specified packages. " + "The following packages: %s will be ignored for " + "depth calculations, using only: %s as the base for limiting " + "package depth.", + sorted(diff), + sorted(leaf_nodes), + ) + self.args_depths = {module: module.count(".") for module in leaf_nodes} def get_title(self, node: nodes.ClassDef) -> str: From f55677e2db1ef239adbe5da2649493c4d0d128ad Mon Sep 17 00:00:00 2001 From: Julfried <51880314+Julfried@users.noreply.github.com> Date: Sat, 22 Feb 2025 19:15:59 +0100 Subject: [PATCH 24/39] shorten include by depth --- pylint/pyreverse/diadefslib.py | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/pylint/pyreverse/diadefslib.py b/pylint/pyreverse/diadefslib.py index 0eda915664..82ecb25671 100644 --- a/pylint/pyreverse/diadefslib.py +++ b/pylint/pyreverse/diadefslib.py @@ -105,10 +105,6 @@ def _get_levels(self) -> tuple[int, int]: def _should_include_by_depth(self, node: nodes.NodeNG) -> bool: """Check if a node should be included based on depth.""" - # If max_depth is not set, include all nodes - if self.config.max_depth is None: - return True - # Calculate the absolute depth of the node name = node.root().name abs_depth = name.count(".") @@ -129,8 +125,8 @@ def show_node(self, node: nodes.ClassDef) -> bool: if is_stdlib_module(node.root().name): return self.config.show_stdlib # type: ignore[no-any-return] - # Filter node by depth - return self._should_include_by_depth(node) + # Filter node by depth ==> if max_depth is not set, show all nodes + return self._should_include_by_depth(node) if self.config.max_depth else True def add_class(self, node: nodes.ClassDef) -> None: """Visit one class and add it to diagram.""" From c9e7259179e403c7cb446288a8b4c34ad824a7fa Mon Sep 17 00:00:00 2001 From: Julfried <51880314+Julfried@users.noreply.github.com> Date: Sat, 22 Feb 2025 19:20:31 +0100 Subject: [PATCH 25/39] Revert "shorten include by depth" This reverts commit f55677e2db1ef239adbe5da2649493c4d0d128ad. --- pylint/pyreverse/diadefslib.py | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/pylint/pyreverse/diadefslib.py b/pylint/pyreverse/diadefslib.py index 82ecb25671..0eda915664 100644 --- a/pylint/pyreverse/diadefslib.py +++ b/pylint/pyreverse/diadefslib.py @@ -105,6 +105,10 @@ def _get_levels(self) -> tuple[int, int]: def _should_include_by_depth(self, node: nodes.NodeNG) -> bool: """Check if a node should be included based on depth.""" + # If max_depth is not set, include all nodes + if self.config.max_depth is None: + return True + # Calculate the absolute depth of the node name = node.root().name abs_depth = name.count(".") @@ -125,8 +129,8 @@ def show_node(self, node: nodes.ClassDef) -> bool: if is_stdlib_module(node.root().name): return self.config.show_stdlib # type: ignore[no-any-return] - # Filter node by depth ==> if max_depth is not set, show all nodes - return self._should_include_by_depth(node) if self.config.max_depth else True + # Filter node by depth + return self._should_include_by_depth(node) def add_class(self, node: nodes.ClassDef) -> None: """Visit one class and add it to diagram.""" From fbc5309e0c21e8fa5e6b456cfa546dc66e6943e3 Mon Sep 17 00:00:00 2001 From: Julfried <51880314+Julfried@users.noreply.github.com> Date: Sat, 22 Feb 2025 21:48:17 +0100 Subject: [PATCH 26/39] Parameterize depth limited config --- tests/pyreverse/conftest.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/pyreverse/conftest.py b/tests/pyreverse/conftest.py index 406b316f24..bb477bd441 100644 --- a/tests/pyreverse/conftest.py +++ b/tests/pyreverse/conftest.py @@ -74,7 +74,7 @@ def html_config() -> PyreverseConfig: ) -@pytest.fixture() +@pytest.fixture(params=[None, *range(5)]) def depth_limited_config(default_max_depth: int) -> PyreverseConfig: return PyreverseConfig( max_depth=default_max_depth, From a48462087d66f1e54057a88ad2af9e16790b544c Mon Sep 17 00:00:00 2001 From: Julfried <51880314+Julfried@users.noreply.github.com> Date: Sat, 22 Feb 2025 22:48:55 +0100 Subject: [PATCH 27/39] Revert "Parameterize depth limited config" This reverts commit fbc5309e0c21e8fa5e6b456cfa546dc66e6943e3. --- tests/pyreverse/conftest.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/pyreverse/conftest.py b/tests/pyreverse/conftest.py index bb477bd441..406b316f24 100644 --- a/tests/pyreverse/conftest.py +++ b/tests/pyreverse/conftest.py @@ -74,7 +74,7 @@ def html_config() -> PyreverseConfig: ) -@pytest.fixture(params=[None, *range(5)]) +@pytest.fixture() def depth_limited_config(default_max_depth: int) -> PyreverseConfig: return PyreverseConfig( max_depth=default_max_depth, From 8601863980311bc46bf839dc123a336d715b4831 Mon Sep 17 00:00:00 2001 From: Julfried <51880314+Julfried@users.noreply.github.com> Date: Sun, 23 Feb 2025 10:53:21 +0100 Subject: [PATCH 28/39] construct generator using a factory function --- pylint/typing.py | 10 ++++++- tests/pyreverse/test_diadefs.py | 51 +++++++++++++++++++-------------- 2 files changed, 39 insertions(+), 22 deletions(-) diff --git a/pylint/typing.py b/pylint/typing.py index 779a78aa7c..a43674d249 100644 --- a/pylint/typing.py +++ b/pylint/typing.py @@ -7,7 +7,7 @@ from __future__ import annotations import argparse -from collections.abc import Iterable +from collections.abc import Iterable, Sequence from pathlib import Path from re import Pattern from typing import ( @@ -24,8 +24,10 @@ if TYPE_CHECKING: from pylint.config.callback_actions import _CallbackAction + from pylint.pyreverse.diadefslib import DiaDefGenerator from pylint.pyreverse.inspector import Project from pylint.reporters.ureports.nodes import Section + from pylint.testutils.pyreverse import PyreverseConfig from pylint.utils import LinterStats @@ -134,3 +136,9 @@ class GetProjectCallable(Protocol): def __call__( self, module: str, name: str | None = "No Name" ) -> Project: ... # pragma: no cover + + +class GeneratorFactory(Protocol): + def __call__( + self, config: PyreverseConfig | None = None, args: Sequence[str] | None = None + ) -> DiaDefGenerator: ... diff --git a/tests/pyreverse/test_diadefs.py b/tests/pyreverse/test_diadefs.py index 97b1699d02..52fa56572f 100644 --- a/tests/pyreverse/test_diadefs.py +++ b/tests/pyreverse/test_diadefs.py @@ -25,7 +25,7 @@ from pylint.pyreverse.inspector import Linker, Project from pylint.testutils.pyreverse import PyreverseConfig from pylint.testutils.utils import _test_cwd -from pylint.typing import GetProjectCallable +from pylint.typing import GeneratorFactory, GetProjectCallable HERE = Path(__file__) TESTS = HERE.parent.parent @@ -61,6 +61,21 @@ def PROJECT(get_project: GetProjectCallable) -> Iterator[Project]: yield get_project("data") +# Fixture with factory function to create DiaDefGenerator instances +@pytest.fixture +def generator_factory(HANDLER: DiadefsHandler, PROJECT: Project) -> GeneratorFactory: + def _factory( + config: PyreverseConfig | None = None, args: Sequence[str] | None = None + ) -> DiaDefGenerator: + if config: + HANDLER.config = config + if args: + HANDLER.args = args + return DiaDefGenerator(Linker(PROJECT), HANDLER) + + return _factory + + # Fixture for creating mocked nodes @pytest.fixture def mock_node() -> Callable[[str], Mock]: @@ -73,12 +88,6 @@ def _mock_node(module_path: str) -> Mock: return _mock_node -# Fixture for the DiaDefGenerator -@pytest.fixture -def generator(HANDLER: DiadefsHandler, PROJECT: Project) -> DiaDefGenerator: - return DiaDefGenerator(Linker(PROJECT), HANDLER) - - def test_option_values( default_config: PyreverseConfig, default_args: Sequence[str], @@ -291,10 +300,10 @@ def test_regression_dataclasses_inference( def test_should_include_by_depth_no_limit( - generator: DiaDefGenerator, mock_node: Mock + generator_factory: GeneratorFactory, mock_node: Mock ) -> None: """Test that nodes are included when no depth limit is set.""" - generator.config.max_depth = None + generator = generator_factory() # Create mocked nodes with different depths node1 = mock_node("pkg") # Depth 0 @@ -309,7 +318,7 @@ def test_should_include_by_depth_no_limit( @pytest.mark.parametrize("max_depth", range(5)) def test_should_include_by_depth_absolute( - generator: DiaDefGenerator, mock_node: Mock, max_depth: int + generator_factory: GeneratorFactory, mock_node: Mock, max_depth: int ) -> None: """Test absolute depth filtering (no relative package logic). @@ -318,8 +327,7 @@ def test_should_include_by_depth_absolute( - 'pkg.subpkg.module' -> depth 2 - 'pkg.subpkg.module.submodule' -> depth 3 """ - generator.config.max_depth = max_depth - generator.args = ["pkg"] # Single root package + generator = generator_factory(PyreverseConfig(max_depth=max_depth), ["pkg"]) test_cases = [ "pkg", @@ -344,7 +352,7 @@ def test_should_include_by_depth_absolute( "specified_package", [["pkg"], ["pkg.subpkg"], ["pkg.subpkg.module"]] ) def test_should_include_by_depth_relative_single_package( - generator: DiaDefGenerator, + generator_factory: GeneratorFactory, mock_node: Mock, max_depth: int, specified_package: list[str], @@ -353,8 +361,10 @@ def test_should_include_by_depth_relative_single_package( Each test case ensures that depth is calculated **relative** to the specified package. """ - generator.config.max_depth = max_depth - generator.args = specified_package # Only one package is specified + generator = generator_factory( + PyreverseConfig(max_depth=max_depth), specified_package + ) + print(generator.args_depths) test_cases = [ "pkg", @@ -369,7 +379,7 @@ def test_should_include_by_depth_relative_single_package( for node in nodes: abs_depth = node.root.return_value.name.count(".") relative_depth = abs_depth - base_depth - should_show = relative_depth <= max_depth + should_show = relative_depth >= max_depth msg = ( f"Node {node.root.return_value.name} (abs_depth {abs_depth}, base_depth {base_depth}, " @@ -382,15 +392,14 @@ def test_should_include_by_depth_relative_single_package( @pytest.mark.parametrize("max_depth", range(5)) def test_should_include_by_depth_relative_multiple_packages( - generator: DiaDefGenerator, mock_node: Mock, max_depth: int + generator_factory: GeneratorFactory, mock_node: Mock, max_depth: int ) -> None: """Test relative depth filtering when multiple packages are specified. The base depth should be determined by the **shallowest** specified package. """ - specified_packages = ["pkg", "pkg.subpkg"] - generator.config.max_depth = max_depth - generator.args = specified_packages # Multiple packages specified ==> depth relative to the shallowest + pkg = ["pkg", "pkg.subpkg"] + generator = generator_factory(PyreverseConfig(max_depth=max_depth), pkg) test_cases = [ "pkg", @@ -401,7 +410,7 @@ def test_should_include_by_depth_relative_multiple_packages( nodes = [mock_node(path) for path in test_cases] # Compute base depth relative to the shallowest specified package - base_depth = min(arg.count(".") for arg in specified_packages) + base_depth = min(arg.count(".") for arg in pkg) for node in nodes: abs_depth = node.root.return_value.name.count(".") From 19f39079bd18c7b64a2884f9414ea33611a54eae Mon Sep 17 00:00:00 2001 From: Julfried <51880314+Julfried@users.noreply.github.com> Date: Sun, 23 Feb 2025 12:41:02 +0100 Subject: [PATCH 29/39] Simplify test case definition to be more explicit --- tests/pyreverse/test_diadefs.py | 86 ++++++++++++++++++--------------- 1 file changed, 47 insertions(+), 39 deletions(-) diff --git a/tests/pyreverse/test_diadefs.py b/tests/pyreverse/test_diadefs.py index 52fa56572f..c8320484f9 100644 --- a/tests/pyreverse/test_diadefs.py +++ b/tests/pyreverse/test_diadefs.py @@ -320,72 +320,80 @@ def test_should_include_by_depth_no_limit( def test_should_include_by_depth_absolute( generator_factory: GeneratorFactory, mock_node: Mock, max_depth: int ) -> None: - """Test absolute depth filtering (no relative package logic). + """Test absolute depth filtering when root package is specified. - 'pkg' -> depth 0 - 'pkg.subpkg' -> depth 1 - 'pkg.subpkg.module' -> depth 2 - 'pkg.subpkg.module.submodule' -> depth 3 """ - generator = generator_factory(PyreverseConfig(max_depth=max_depth), ["pkg"]) + specified_pkg = ["pkg"] + generator = generator_factory(PyreverseConfig(max_depth=max_depth), specified_pkg) - test_cases = [ - "pkg", - "pkg.subpkg", - "pkg.subpkg.module", - "pkg.subpkg.module.submodule", - ] - nodes = [mock_node(path) for path in test_cases] + test_cases = { + "pkg": [True, True, True, True, True], + "pkg.subpkg": [False, True, True, True, True], + "pkg.subpkg.module": [False, False, True, True, True], + "pkg.subpkg.module.submodule": [False, False, False, True, True], + } + nodes = [mock_node(path) for path, _ in test_cases.items()] - for i, node in enumerate(nodes): - should_show = i <= max_depth + for node in nodes: + should_show = test_cases[node.root.return_value.name][max_depth] msg = ( - f"Node {node.root.return_value.name} (depth {i}) with max_depth={max_depth} " - f"{'should show' if should_show else 'should not show'}:" - f"{generator._should_include_by_depth(node)}" + f"Node {node.root.return_value.name} with max_depth={max_depth} and " + f"specified package {specified_pkg} " + f"{'should show' if should_show else 'should not show'}. " + f"Generator returns: {generator._should_include_by_depth(node)}" ) assert generator._should_include_by_depth(node) == should_show, msg @pytest.mark.parametrize("max_depth", range(5)) -@pytest.mark.parametrize( - "specified_package", [["pkg"], ["pkg.subpkg"], ["pkg.subpkg.module"]] -) +@pytest.mark.parametrize("args", [["pkg"], ["pkg.subpkg"], ["pkg.subpkg.module"]]) def test_should_include_by_depth_relative_single_package( generator_factory: GeneratorFactory, mock_node: Mock, max_depth: int, - specified_package: list[str], + args: list[str], ) -> None: """Test relative depth filtering when only one package is specified. Each test case ensures that depth is calculated **relative** to the specified package. """ - generator = generator_factory( - PyreverseConfig(max_depth=max_depth), specified_package - ) - print(generator.args_depths) - - test_cases = [ - "pkg", - "pkg.subpkg", - "pkg.subpkg.module", - "pkg.subpkg.module.submodule", - ] - nodes = [mock_node(path) for path in test_cases] - - base_depth = specified_package[0].count(".") + specified_pkg = args[0] + generator = generator_factory(PyreverseConfig(max_depth=max_depth), args) + + test_cases = { + "pkg": { + "pkg": [True, True, True, True, True], + "pkg.subpkg": [False, True, True, True, True], + "pkg.subpkg.module": [False, False, True, True, True], + "pkg.subpkg.module.submodule": [False, False, False, True, True], + }, + "pkg.subpkg": { + "pkg": [False, False, False, False, False], + "pkg.subpkg": [True, True, True, True, True], + "pkg.subpkg.module": [False, True, True, True, True], + "pkg.subpkg.module.submodule": [False, False, True, True, True], + }, + "pkg.subpkg.module": { + "pkg": [False, False, False, False, False], + "pkg.subpkg": [False, False, False, False, False], + "pkg.subpkg.module": [True, True, True, True, True], + "pkg.subpkg.module.submodule": [False, True, True, True, True], + }, + } + nodes = [mock_node(path) for path, _ in test_cases.items()] for node in nodes: - abs_depth = node.root.return_value.name.count(".") - relative_depth = abs_depth - base_depth - should_show = relative_depth >= max_depth + should_show = test_cases[specified_pkg][node.root.return_value.name][max_depth] msg = ( - f"Node {node.root.return_value.name} (abs_depth {abs_depth}, base_depth {base_depth}, " - f"relative_depth {relative_depth}) with max_depth={max_depth} " - f"{'should show' if should_show else 'should not show'}:" - f"{generator._should_include_by_depth(node)}" + f"Node {node.root.return_value.name} with max_depth={max_depth} and " + f"specified package {specified_pkg} " + f"{'should show' if should_show else 'should not show'}. " + f"Generator returns: {generator._should_include_by_depth(node)}" ) assert generator._should_include_by_depth(node) == should_show, msg From 801916bd09074f25b9a3070334ab87fbc6d5a53c Mon Sep 17 00:00:00 2001 From: Julfried <51880314+Julfried@users.noreply.github.com> Date: Sun, 23 Feb 2025 14:20:43 +0100 Subject: [PATCH 30/39] Update docstring of _should_include_by_depth function --- pylint/pyreverse/diadefslib.py | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/pylint/pyreverse/diadefslib.py b/pylint/pyreverse/diadefslib.py index 0eda915664..f470514097 100644 --- a/pylint/pyreverse/diadefslib.py +++ b/pylint/pyreverse/diadefslib.py @@ -104,7 +104,13 @@ def _get_levels(self) -> tuple[int, int]: return self.anc_level, self.association_level def _should_include_by_depth(self, node: nodes.NodeNG) -> bool: - """Check if a node should be included based on depth.""" + """Check if a node should be included based on depth. + + A node will be included if it is at or below the max_depth relative to the + specified base packages. A node is considered to be a base package if it is the + deepest package in the list of specified packages. In other words the base nodes + are the leaf nodes of the specified package tree. + """ # If max_depth is not set, include all nodes if self.config.max_depth is None: return True From e4a86e132af27cf220906feac41b2d4948f8f71c Mon Sep 17 00:00:00 2001 From: Julfried <51880314+Julfried@users.noreply.github.com> Date: Sun, 23 Feb 2025 14:41:57 +0100 Subject: [PATCH 31/39] Fix failing tests in test_diadefs.py --- tests/pyreverse/test_diadefs.py | 50 ++++++++++++++++----------------- 1 file changed, 25 insertions(+), 25 deletions(-) diff --git a/tests/pyreverse/test_diadefs.py b/tests/pyreverse/test_diadefs.py index c8320484f9..5103862c00 100644 --- a/tests/pyreverse/test_diadefs.py +++ b/tests/pyreverse/test_diadefs.py @@ -8,12 +8,14 @@ from __future__ import annotations +import logging from collections.abc import Callable, Iterator, Sequence from pathlib import Path from unittest.mock import Mock import pytest from astroid import extract_node, nodes +from pytest import LogCaptureFixture from pylint.pyreverse.diadefslib import ( ClassDiadefGenerator, @@ -400,35 +402,33 @@ def test_should_include_by_depth_relative_single_package( @pytest.mark.parametrize("max_depth", range(5)) def test_should_include_by_depth_relative_multiple_packages( - generator_factory: GeneratorFactory, mock_node: Mock, max_depth: int + caplog: LogCaptureFixture, + generator_factory: GeneratorFactory, + mock_node: Mock, + max_depth: int, ) -> None: - """Test relative depth filtering when multiple packages are specified. - - The base depth should be determined by the **shallowest** specified package. - """ - pkg = ["pkg", "pkg.subpkg"] - generator = generator_factory(PyreverseConfig(max_depth=max_depth), pkg) - - test_cases = [ - "pkg", - "pkg.subpkg", - "pkg.subpkg.module", - "pkg.subpkg.module.submodule", - ] - nodes = [mock_node(path) for path in test_cases] + """Test relative depth filtering when multiple packages are specified.""" + specified_pkg = ["pkg", "pkg.subpkg"] + generator = generator_factory(PyreverseConfig(max_depth=max_depth), specified_pkg) - # Compute base depth relative to the shallowest specified package - base_depth = min(arg.count(".") for arg in pkg) + test_cases = { + "pkg": [False, False, False, False, False], + "pkg.subpkg": [True, True, True, True, True], + "pkg.subpkg.module": [False, True, True, True, True], + "pkg.subpkg.module.submodule": [False, False, True, True, True], + } + nodes = [mock_node(path) for path, _ in test_cases.items()] for node in nodes: - abs_depth = node.root.return_value.name.count(".") - relative_depth = abs_depth - base_depth - should_show = relative_depth <= max_depth + should_show = test_cases[node.root.return_value.name][max_depth] + + with caplog.at_level(logging.WARNING): + result = generator._should_include_by_depth(node) msg = ( - f"Node {node.root.return_value.name} (abs_depth {abs_depth}, base_depth {base_depth}, " - f"relative_depth {relative_depth}) with max_depth={max_depth} " - f"{'should show' if should_show else 'should not show'}:" - f"{generator._should_include_by_depth(node)}" + f"Node {node.root.return_value.name} with max_depth={max_depth} and " + f"specified package {specified_pkg} " + f"{'should show' if should_show else 'should not show'}. " + f"Generator returns: {result}" ) - assert generator._should_include_by_depth(node) == should_show, msg + assert result == should_show, msg From 2d59f0d3b7e779c3c1cce1b12f5037041e2501b9 Mon Sep 17 00:00:00 2001 From: Julfried <51880314+Julfried@users.noreply.github.com> Date: Sun, 23 Feb 2025 15:52:51 +0100 Subject: [PATCH 32/39] Fix failing tests --- tests/pyreverse/test_diadefs.py | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/tests/pyreverse/test_diadefs.py b/tests/pyreverse/test_diadefs.py index 5103862c00..3792e2ede6 100644 --- a/tests/pyreverse/test_diadefs.py +++ b/tests/pyreverse/test_diadefs.py @@ -431,4 +431,12 @@ def test_should_include_by_depth_relative_multiple_packages( f"{'should show' if should_show else 'should not show'}. " f"Generator returns: {result}" ) + logging_msg = ( + "Detected nested names within the specified packages. " + "The following packages: ['pkg'] will be ignored for " + "depth calculations, using only: ['pkg.subpkg'] as the base for limiting " + ) + print(logging_msg) + print(caplog.text) assert result == should_show, msg + assert logging_msg in caplog.text From b5516a09340ffab052dc44f30a2ebcdcab7842bb Mon Sep 17 00:00:00 2001 From: Julfried <51880314+Julfried@users.noreply.github.com> Date: Sun, 23 Feb 2025 17:48:17 +0100 Subject: [PATCH 33/39] Clean up tests --- tests/pyreverse/test_diadefs.py | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/tests/pyreverse/test_diadefs.py b/tests/pyreverse/test_diadefs.py index 3792e2ede6..f3ab2c37d8 100644 --- a/tests/pyreverse/test_diadefs.py +++ b/tests/pyreverse/test_diadefs.py @@ -342,13 +342,15 @@ def test_should_include_by_depth_absolute( for node in nodes: should_show = test_cases[node.root.return_value.name][max_depth] + result = generator._should_include_by_depth(node) + msg = ( f"Node {node.root.return_value.name} with max_depth={max_depth} and " f"specified package {specified_pkg} " f"{'should show' if should_show else 'should not show'}. " - f"Generator returns: {generator._should_include_by_depth(node)}" + f"Generator returns: {result}" ) - assert generator._should_include_by_depth(node) == should_show, msg + assert result == should_show, msg @pytest.mark.parametrize("max_depth", range(5)) @@ -390,14 +392,15 @@ def test_should_include_by_depth_relative_single_package( for node in nodes: should_show = test_cases[specified_pkg][node.root.return_value.name][max_depth] + result = generator._should_include_by_depth(node) msg = ( f"Node {node.root.return_value.name} with max_depth={max_depth} and " f"specified package {specified_pkg} " f"{'should show' if should_show else 'should not show'}. " - f"Generator returns: {generator._should_include_by_depth(node)}" + f"Generator returns: {result}" ) - assert generator._should_include_by_depth(node) == should_show, msg + assert result == should_show, msg @pytest.mark.parametrize("max_depth", range(5)) @@ -436,7 +439,5 @@ def test_should_include_by_depth_relative_multiple_packages( "The following packages: ['pkg'] will be ignored for " "depth calculations, using only: ['pkg.subpkg'] as the base for limiting " ) - print(logging_msg) - print(caplog.text) assert result == should_show, msg assert logging_msg in caplog.text From 0b3af62076306446f8c5a57ad638b0369471b7a1 Mon Sep 17 00:00:00 2001 From: Julfried <51880314+Julfried@users.noreply.github.com> Date: Sun, 23 Feb 2025 18:18:56 +0100 Subject: [PATCH 34/39] Add a tests for get_leaf_node function --- tests/pyreverse/test_diadefs.py | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/tests/pyreverse/test_diadefs.py b/tests/pyreverse/test_diadefs.py index f3ab2c37d8..f681f5f75a 100644 --- a/tests/pyreverse/test_diadefs.py +++ b/tests/pyreverse/test_diadefs.py @@ -441,3 +441,20 @@ def test_should_include_by_depth_relative_multiple_packages( ) assert result == should_show, msg assert logging_msg in caplog.text + + +def test_get_leaf_nodes(generator_factory: GeneratorFactory) -> None: + """Test that leaf nodes are correctly identified.""" + # Create a tree with a single root node and multiple leaf nodes + specified_packeges = [ + "pkg", + "pkg.subpkg1", + "pkg.subpkg2", + "pkg.subpkg1.module1", + "pkg.subpkg2.module2", + "pkg.subpkg1.module1.submodule", + ] + corr = set(["pkg.subpkg2.module2", "pkg.subpkg1.module1.submodule"]) + + generator = generator_factory(args=specified_packeges) + assert len(corr.difference(generator.get_leaf_nodes(specified_packeges))) == 0 From 5902e4de7b948f2ecc63c93ca18c5ba43e71e98d Mon Sep 17 00:00:00 2001 From: Julfried <51880314+Julfried@users.noreply.github.com> Date: Sun, 23 Feb 2025 18:23:34 +0100 Subject: [PATCH 35/39] Refactor get_leaf_nodes method to use instance arguments directly --- pylint/pyreverse/diadefslib.py | 10 +++++----- tests/pyreverse/test_diadefs.py | 2 +- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/pylint/pyreverse/diadefslib.py b/pylint/pyreverse/diadefslib.py index f470514097..bd89f01a17 100644 --- a/pylint/pyreverse/diadefslib.py +++ b/pylint/pyreverse/diadefslib.py @@ -36,7 +36,7 @@ def __init__(self, linker: Linker, handler: DiadefsHandler) -> None: # Only pre-calculate depths if user has requested a max_depth if handler.config.max_depth is not None: # Detect which of the args are leaf nodes - leaf_nodes = self.get_leaf_nodes(self.args) + leaf_nodes = self.get_leaf_nodes() # Emit a warning if any of the args are not leaf nodes diff = set(self.args).difference(set(leaf_nodes)) @@ -59,18 +59,18 @@ def get_title(self, node: nodes.ClassDef) -> str: title = f"{node.root().name}.{title}" return title # type: ignore[no-any-return] - def get_leaf_nodes(self, module_names: Sequence[str]) -> list[str]: + def get_leaf_nodes(self) -> list[str]: """ - Given a list of module/package names, return only the leaf nodes. + Get the leaf nodes from the list of args in the generator. A leaf node is one that is not a prefix (with an extra dot) of any other node. """ leaf_nodes = [ module - for module in module_names + for module in self.args if not any( other != module and other.startswith(module + ".") - for other in module_names + for other in self.args ) ] return leaf_nodes diff --git a/tests/pyreverse/test_diadefs.py b/tests/pyreverse/test_diadefs.py index f681f5f75a..783b156749 100644 --- a/tests/pyreverse/test_diadefs.py +++ b/tests/pyreverse/test_diadefs.py @@ -457,4 +457,4 @@ def test_get_leaf_nodes(generator_factory: GeneratorFactory) -> None: corr = set(["pkg.subpkg2.module2", "pkg.subpkg1.module1.submodule"]) generator = generator_factory(args=specified_packeges) - assert len(corr.difference(generator.get_leaf_nodes(specified_packeges))) == 0 + assert len(corr.difference(generator.get_leaf_nodes())) == 0 From c33e88b60974b57e949cc1a399cab6916e6af01d Mon Sep 17 00:00:00 2001 From: Julian Grimm <51880314+Julfried@users.noreply.github.com> Date: Tue, 25 Feb 2025 20:41:46 +0100 Subject: [PATCH 36/39] Apply suggestions from code review Co-authored-by: Pierre Sassoulas --- pylint/pyreverse/diadefslib.py | 10 +++++----- tests/pyreverse/test_diadefs.py | 10 +++++----- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/pylint/pyreverse/diadefslib.py b/pylint/pyreverse/diadefslib.py index bd89f01a17..5882642e7f 100644 --- a/pylint/pyreverse/diadefslib.py +++ b/pylint/pyreverse/diadefslib.py @@ -41,7 +41,7 @@ def __init__(self, linker: Linker, handler: DiadefsHandler) -> None: # Emit a warning if any of the args are not leaf nodes diff = set(self.args).difference(set(leaf_nodes)) if len(diff) > 0: - logging.warning( + warnings.warn( "Detected nested names within the specified packages. " "The following packages: %s will be ignored for " "depth calculations, using only: %s as the base for limiting " @@ -117,14 +117,14 @@ def _should_include_by_depth(self, node: nodes.NodeNG) -> bool: # Calculate the absolute depth of the node name = node.root().name - abs_depth = name.count(".") + absolute_depth = name.count(".") # Retrieve the base depth to compare against - rel_depth = next( + relative_depth = next( (v for k, v in self.args_depths.items() if name.startswith(k)), None ) - return rel_depth is not None and bool( - (abs_depth - rel_depth) <= self.config.max_depth + return relative_depth is not None and bool( + (absolute_depth - relative_depth) <= self.config.max_depth ) def show_node(self, node: nodes.ClassDef) -> bool: diff --git a/tests/pyreverse/test_diadefs.py b/tests/pyreverse/test_diadefs.py index 783b156749..5728f4ca90 100644 --- a/tests/pyreverse/test_diadefs.py +++ b/tests/pyreverse/test_diadefs.py @@ -346,8 +346,8 @@ def test_should_include_by_depth_absolute( msg = ( f"Node {node.root.return_value.name} with max_depth={max_depth} and " - f"specified package {specified_pkg} " - f"{'should show' if should_show else 'should not show'}. " + f"specified package {specified_pkg} should" + f"{'' if should_show else ' not'} show. " f"Generator returns: {result}" ) assert result == should_show, msg @@ -396,8 +396,8 @@ def test_should_include_by_depth_relative_single_package( msg = ( f"Node {node.root.return_value.name} with max_depth={max_depth} and " - f"specified package {specified_pkg} " - f"{'should show' if should_show else 'should not show'}. " + f"specified package {specified_pkg} should" + f"{'' if should_show else ' not'} show. " f"Generator returns: {result}" ) assert result == should_show, msg @@ -405,7 +405,7 @@ def test_should_include_by_depth_relative_single_package( @pytest.mark.parametrize("max_depth", range(5)) def test_should_include_by_depth_relative_multiple_packages( - caplog: LogCaptureFixture, + capwarn: NotSureaboutTheType, generator_factory: GeneratorFactory, mock_node: Mock, max_depth: int, From 33850fa11e91eeb3a18c37760faca79d362a2868 Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Tue, 25 Feb 2025 19:43:02 +0000 Subject: [PATCH 37/39] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- pylint/pyreverse/diadefslib.py | 1 - tests/pyreverse/test_diadefs.py | 1 - 2 files changed, 2 deletions(-) diff --git a/pylint/pyreverse/diadefslib.py b/pylint/pyreverse/diadefslib.py index 5882642e7f..300eca3885 100644 --- a/pylint/pyreverse/diadefslib.py +++ b/pylint/pyreverse/diadefslib.py @@ -7,7 +7,6 @@ from __future__ import annotations import argparse -import logging from collections.abc import Generator, Sequence from typing import Any diff --git a/tests/pyreverse/test_diadefs.py b/tests/pyreverse/test_diadefs.py index 5728f4ca90..92c88fd531 100644 --- a/tests/pyreverse/test_diadefs.py +++ b/tests/pyreverse/test_diadefs.py @@ -15,7 +15,6 @@ import pytest from astroid import extract_node, nodes -from pytest import LogCaptureFixture from pylint.pyreverse.diadefslib import ( ClassDiadefGenerator, From 918861acb2752a07fde425a7efb4b641c8dd4c65 Mon Sep 17 00:00:00 2001 From: Julfried Date: Tue, 25 Feb 2025 21:39:19 +0100 Subject: [PATCH 38/39] Fix failing tests --- pylint/pyreverse/diadefslib.py | 8 ++++---- tests/pyreverse/test_diadefs.py | 26 ++++++++++++++------------ 2 files changed, 18 insertions(+), 16 deletions(-) diff --git a/pylint/pyreverse/diadefslib.py b/pylint/pyreverse/diadefslib.py index 300eca3885..9017db27f6 100644 --- a/pylint/pyreverse/diadefslib.py +++ b/pylint/pyreverse/diadefslib.py @@ -7,6 +7,7 @@ from __future__ import annotations import argparse +import warnings from collections.abc import Generator, Sequence from typing import Any @@ -42,11 +43,10 @@ def __init__(self, linker: Linker, handler: DiadefsHandler) -> None: if len(diff) > 0: warnings.warn( "Detected nested names within the specified packages. " - "The following packages: %s will be ignored for " - "depth calculations, using only: %s as the base for limiting " + f"The following packages: {sorted(diff)} will be ignored for " + f"depth calculations, using only: {sorted(leaf_nodes)} as the base for limiting " "package depth.", - sorted(diff), - sorted(leaf_nodes), + stacklevel=1, ) self.args_depths = {module: module.count(".") for module in leaf_nodes} diff --git a/tests/pyreverse/test_diadefs.py b/tests/pyreverse/test_diadefs.py index 92c88fd531..1fa173c0ea 100644 --- a/tests/pyreverse/test_diadefs.py +++ b/tests/pyreverse/test_diadefs.py @@ -8,7 +8,7 @@ from __future__ import annotations -import logging +import re from collections.abc import Callable, Iterator, Sequence from pathlib import Path from unittest.mock import Mock @@ -404,14 +404,24 @@ def test_should_include_by_depth_relative_single_package( @pytest.mark.parametrize("max_depth", range(5)) def test_should_include_by_depth_relative_multiple_packages( - capwarn: NotSureaboutTheType, generator_factory: GeneratorFactory, mock_node: Mock, max_depth: int, ) -> None: """Test relative depth filtering when multiple packages are specified.""" specified_pkg = ["pkg", "pkg.subpkg"] - generator = generator_factory(PyreverseConfig(max_depth=max_depth), specified_pkg) + + warning_msg = re.escape( + "Detected nested names within the specified packages. " + "The following packages: ['pkg'] will be ignored for " + "depth calculations, using only: ['pkg.subpkg'] as the base for limiting " + "package depth." + ) + + with pytest.warns(UserWarning, match=warning_msg): + generator = generator_factory( + PyreverseConfig(max_depth=max_depth), specified_pkg + ) test_cases = { "pkg": [False, False, False, False, False], @@ -423,9 +433,7 @@ def test_should_include_by_depth_relative_multiple_packages( for node in nodes: should_show = test_cases[node.root.return_value.name][max_depth] - - with caplog.at_level(logging.WARNING): - result = generator._should_include_by_depth(node) + result = generator._should_include_by_depth(node) msg = ( f"Node {node.root.return_value.name} with max_depth={max_depth} and " @@ -433,13 +441,7 @@ def test_should_include_by_depth_relative_multiple_packages( f"{'should show' if should_show else 'should not show'}. " f"Generator returns: {result}" ) - logging_msg = ( - "Detected nested names within the specified packages. " - "The following packages: ['pkg'] will be ignored for " - "depth calculations, using only: ['pkg.subpkg'] as the base for limiting " - ) assert result == should_show, msg - assert logging_msg in caplog.text def test_get_leaf_nodes(generator_factory: GeneratorFactory) -> None: From 0e629d9d908dc7778159986bb5993fe380790cfe Mon Sep 17 00:00:00 2001 From: Julfried Date: Tue, 25 Feb 2025 21:42:59 +0100 Subject: [PATCH 39/39] Fix stackelevel --- pylint/pyreverse/diadefslib.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pylint/pyreverse/diadefslib.py b/pylint/pyreverse/diadefslib.py index 9017db27f6..c1f14ce727 100644 --- a/pylint/pyreverse/diadefslib.py +++ b/pylint/pyreverse/diadefslib.py @@ -46,7 +46,7 @@ def __init__(self, linker: Linker, handler: DiadefsHandler) -> None: f"The following packages: {sorted(diff)} will be ignored for " f"depth calculations, using only: {sorted(leaf_nodes)} as the base for limiting " "package depth.", - stacklevel=1, + stacklevel=2, ) self.args_depths = {module: module.count(".") for module in leaf_nodes}