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

Pyreverse: Use dashed lines for type-checking imports #8824

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
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
3 changes: 3 additions & 0 deletions doc/whatsnew/fragments/8112.feature
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
In Pyreverse package dependency diagrams, show when a module imports another only for type-checking.

Closes #8112
21 changes: 17 additions & 4 deletions pylint/pyreverse/diagrams.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
import astroid
from astroid import nodes, util

from pylint.checkers.utils import decorated_with_property
from pylint.checkers.utils import decorated_with_property, in_type_checking_block
from pylint.pyreverse.utils import FilterMixIn


Expand Down Expand Up @@ -281,9 +281,15 @@ def get_module(self, name: str, node: nodes.Module) -> PackageEntity:
def add_from_depend(self, node: nodes.ImportFrom, from_module: str) -> None:
"""Add dependencies created by from-imports."""
mod_name = node.root().name
obj = self.module(mod_name)
if from_module not in obj.node.depends:
obj.node.depends.append(from_module)
package = self.module(mod_name).node

if from_module in package.depends:
return

if not in_type_checking_block(node):
package.depends.append(from_module)
elif from_module not in package.type_depends:
package.type_depends.append(from_module)

def extract_relationships(self) -> None:
"""Extract relationships between nodes in the diagram."""
Expand All @@ -304,3 +310,10 @@ def extract_relationships(self) -> None:
except KeyError:
continue
self.add_relationship(package_obj, dep, "depends")

for dep_name in package_obj.node.type_depends:
try:
dep = self.get_module(dep_name, package_obj.node)
except KeyError: # pragma: no cover
continue
self.add_relationship(package_obj, dep, "type_depends")
5 changes: 5 additions & 0 deletions pylint/pyreverse/dot_printer.py
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,11 @@ class HTMLLabels(Enum):
"style": "solid",
},
EdgeType.USES: {"arrowtail": "none", "arrowhead": "open"},
EdgeType.TYPE_DEPENDENCY: {
"arrowtail": "none",
"arrowhead": "open",
"style": "dashed",
},
}


Expand Down
1 change: 1 addition & 0 deletions pylint/pyreverse/inspector.py
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,7 @@ def visit_module(self, node: nodes.Module) -> None:
return
node.locals_type = collections.defaultdict(list)
node.depends = []
node.type_depends = []
if self.tag:
node.uid = self.generate_id()

Expand Down
1 change: 1 addition & 0 deletions pylint/pyreverse/mermaidjs_printer.py
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ class MermaidJSPrinter(Printer):
EdgeType.ASSOCIATION: "--*",
EdgeType.AGGREGATION: "--o",
EdgeType.USES: "-->",
EdgeType.TYPE_DEPENDENCY: "-.->",
Copy link
Contributor

Choose a reason for hiding this comment

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

this is the actual wrong line

Copy link
Member

Choose a reason for hiding this comment

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

Thanks! Would you be willing to provide a PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

yep... just did!
#9031

}

def _open_graph(self) -> None:
Expand Down
1 change: 1 addition & 0 deletions pylint/pyreverse/plantuml_printer.py
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ class PlantUmlPrinter(Printer):
EdgeType.ASSOCIATION: "--*",
EdgeType.AGGREGATION: "--o",
EdgeType.USES: "-->",
EdgeType.TYPE_DEPENDENCY: "..>",
}

def _open_graph(self) -> None:
Expand Down
1 change: 1 addition & 0 deletions pylint/pyreverse/printer.py
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ class EdgeType(Enum):
ASSOCIATION = "association"
AGGREGATION = "aggregation"
USES = "uses"
TYPE_DEPENDENCY = "type_dependency"


class Layout(Enum):
Expand Down
7 changes: 7 additions & 0 deletions pylint/pyreverse/writer.py
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,13 @@ def write_packages(self, diagram: PackageDiagram) -> None:
type_=EdgeType.USES,
)

for rel in diagram.get_relationships("type_depends"):
self.printer.emit_edge(
rel.from_object.fig_id,
rel.to_object.fig_id,
type_=EdgeType.TYPE_DEPENDENCY,
)

def write_classes(self, diagram: ClassDiagram) -> None:
"""Write a class diagram."""
# sorted to get predictable (hence testable) results
Expand Down
4 changes: 4 additions & 0 deletions tests/pyreverse/data/classes_type_check_imports.dot
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
digraph "classes_type_check_imports" {
rankdir=BT
charset="utf-8"
}
13 changes: 13 additions & 0 deletions tests/pyreverse/data/packages_type_check_imports.dot
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
digraph "packages_type_check_imports" {
rankdir=BT
charset="utf-8"
"package_diagrams" [color="black", label=<package_diagrams>, shape="box", style="solid"];
"package_diagrams.type_check_imports" [color="black", label=<package_diagrams.type_check_imports>, shape="box", style="solid"];
"package_diagrams.type_check_imports.mod_a" [color="black", label=<package_diagrams.type_check_imports.mod_a>, shape="box", style="solid"];
"package_diagrams.type_check_imports.mod_b" [color="black", label=<package_diagrams.type_check_imports.mod_b>, shape="box", style="solid"];
"package_diagrams.type_check_imports.mod_c" [color="black", label=<package_diagrams.type_check_imports.mod_c>, shape="box", style="solid"];
"package_diagrams.type_check_imports.mod_d" [color="black", label=<package_diagrams.type_check_imports.mod_d>, shape="box", style="solid"];
"package_diagrams.type_check_imports.mod_b" -> "package_diagrams.type_check_imports.mod_a" [arrowhead="open", arrowtail="none"];
"package_diagrams.type_check_imports.mod_d" -> "package_diagrams.type_check_imports.mod_a" [arrowhead="open", arrowtail="none"];
"package_diagrams.type_check_imports.mod_c" -> "package_diagrams.type_check_imports.mod_a" [arrowhead="open", arrowtail="none", style="dashed"];
}
Empty file.
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
Int = int

List = list
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
from typing import Any

from mod_a import Int


def is_int(x) -> bool:
return isinstance(x, Int)
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
from typing import TYPE_CHECKING

if TYPE_CHECKING:
from mod_a import Int

def some_int() -> Int:
return 5
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
from typing import TYPE_CHECKING

from mod_a import Int

if TYPE_CHECKING:
from typing import Any

from mod_a import List

def list_int(x: Any) -> List[Int]:
return [x] if isinstance(x, Int) else []
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
classDiagram
class type_check_imports {
}
class mod_a {
}
class mod_b {
}
class mod_c {
}
class mod_d {
}
mod_b --> mod_a
mod_d --> mod_a
mod_c -.-> mod_a
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not valid mermaid.

it should be:
mod_c ..> mod_a
as documented here:
https://mermaid.js.org/syntax/classDiagram.html#defining-relationship

github renders mermaid files, and if we try to open this one we get an error, see:
https://github.com/pylint-dev/pylint/blob/ffe1e714a3cfe83b5a627ade6d723489f2e10621/tests/pyreverse/functional/package_diagrams/type_check_imports/packages.mmd

23 changes: 23 additions & 0 deletions tests/pyreverse/test_writer.py
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,10 @@
MMD_FILES = ["packages_No_Name.mmd", "classes_No_Name.mmd"]
HTML_FILES = ["packages_No_Name.html", "classes_No_Name.html"]
NO_STANDALONE_FILES = ["classes_no_standalone.dot", "packages_no_standalone.dot"]
TYPE_CHECK_IMPORTS_FILES = [
"packages_type_check_imports.dot",
"classes_type_check_imports.dot",
]


class Config:
Expand Down Expand Up @@ -105,6 +109,19 @@ def setup_no_standalone_dot(
yield from _setup(project, no_standalone_dot_config, writer)


@pytest.fixture()
def setup_type_check_imports_dot(
default_config: PyreverseConfig, get_project: GetProjectCallable
) -> Iterator[None]:
writer = DiagramWriter(default_config)
project = get_project(
os.path.join(os.path.dirname(__file__), "functional", "package_diagrams"),
name="type_check_imports",
)

yield from _setup(project, default_config, writer)


@pytest.fixture()
def setup_puml(
puml_config: PyreverseConfig, get_project: GetProjectCallable
Expand Down Expand Up @@ -173,6 +190,12 @@ def test_no_standalone_dot_files(generated_file: str) -> None:
_assert_files_are_equal(generated_file)


@pytest.mark.usefixtures("setup_type_check_imports_dot")
@pytest.mark.parametrize("generated_file", TYPE_CHECK_IMPORTS_FILES)
def test_type_check_imports_dot_files(generated_file: str) -> None:
_assert_files_are_equal(generated_file)


@pytest.mark.usefixtures("setup_puml")
@pytest.mark.parametrize("generated_file", PUML_FILES)
def test_puml_files(generated_file: str) -> None:
Expand Down
Loading