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

nickdrozd · 2023-07-04T20:17:09Z

Type of Changes

	Type
✓	✨ New feature

Description

This PR modifies the Pyreverse package diagram generator so that if module A only imports from module B under the TYPE_CHECKING flag, an empty arrow is used.

Closes #8112

Here is an example of the output:

In this diagram, the instrs module is only imported for type checking, the tape module is sometimes imported for type checking and sometimes imported for real, and the rest of the modules are all imported for real.

There is an interesting interaction with the (still unreleased) --no-standalone option. Namely, a module only imported for type checking appears as a ghost module:

I hadn't considered this in my implementation, but I think it actually works out pretty well.

TODO

Documentation
Testing
Add output for writers other than dot.

Pierre-Sassoulas

Those "empty arrows" have a meaning in UML it means generalization, i.e. "inherit from". The proper UML convention needs to be used, possibly dotted line with the same arrow for "dependency" see (14) here : https://stackoverflow.com/a/2293763/2519059.

codecov · 2023-07-04T20:25:04Z

Codecov Report

Merging #8824 (abcfcc2) into main (f815e9f) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #8824   +/-   ##
=======================================
  Coverage   95.89%   95.90%           
=======================================
  Files         173      173           
  Lines       18511    18523   +12     
=======================================
+ Hits        17752    17765   +13     
+ Misses        759      758    -1

Impacted Files	Coverage Δ
pylint/pyreverse/dot_printer.py	`85.86% <ø> (ø)`
pylint/pyreverse/mermaidjs_printer.py	`98.18% <ø> (ø)`
pylint/pyreverse/plantuml_printer.py	`100.00% <ø> (ø)`
pylint/pyreverse/diagrams.py	`93.10% <100.00%> (+0.93%)`	⬆️
pylint/pyreverse/inspector.py	`79.79% <100.00%> (+0.10%)`	⬆️
pylint/pyreverse/printer.py	`100.00% <100.00%> (ø)`
pylint/pyreverse/writer.py	`100.00% <100.00%> (ø)`

nickdrozd · 2023-07-05T14:49:14Z

Using dashed lines instead for type-checking-only dependencies:

for more information, see https://pre-commit.ci

nickdrozd · 2023-07-05T17:43:33Z

@DudeNr33 Something is off with the tests, but I can't what it is. There's some kind of interference with clientmodule_test, but I don't know where it's coming from.

DudeNr33 · 2023-07-05T18:17:19Z

We currently do not have a equivalent to the functional test of class diagrams for package diagrams.
The current tests are configured to create diagrams for the packages/modules found under tests/data:

@pytest.fixture()
def setup_type_check_imports_dot(
    type_check_imports_dot_config: PyreverseConfig, get_project: GetProjectCallable
) -> Iterator[None]:
    writer = DiagramWriter(type_check_imports_dot_config)
    project = get_project(TEST_DATA_DIR, name="type_check_imports")
    yield from _setup(project, type_check_imports_dot_config, writer)

The first argument to get_project is the directory to analyze.
The content of your packages_type_check_imports.dot however looks like it expects the result of analyzing tests/pyreverse/functional/package_diagrams.

The cleanest solution would probably be to come up with a working functional test harness for package diagrams as well.

for more information, see https://pre-commit.ci

nickdrozd · 2023-07-05T19:33:39Z

pylint/pyreverse/diagrams.py

+                except KeyError:
+                    continue


These two lines are uncovered, and I don't know how to cover them. Any ideas? They could also be cut, or just ignore the coverage.

The check is definitely required. But I can't figure out exactly what triggers it.

Pierre-Sassoulas

I think this look dope, great change (especially in conjunction with the other option) ! Although I would appreciate a second / third opinion regarding the UML symbolic before we commit to it.

DudeNr33 · 2023-07-07T16:43:38Z

I think this look dope, great change (especially in conjunction with the other option) ! Although I would appreciate a second / third opinion regarding the UML symbolic before we commit to it.

I am not an UML expert myself, but the link you provided sounds pretty reasonable to me!

DudeNr33

Thank you Nick for the new feature. I like it.

I left some remarks.

I also think we should try to introduce real functional package diagram tests rather sooner than later after this PR. The functional tests for class diagram work more or less the same as the functional tests for messages - that is, you only need to create the input, output, and optionally config file and they will be picked up automatically in the test run.
This is not yet the case for the files below functional/package_diagram, but the naming convention could make you believe it is.

pylint/pyreverse/printer.py

tests/pyreverse/conftest.py

DudeNr33 · 2023-07-09T07:28:25Z

codecov seems to be stuck. @Pierre-Sassoulas what's the best fix for this? Close/reopen the PR?

Pierre-Sassoulas · 2023-07-09T07:54:31Z

Yeah, let's try !

Pierre-Sassoulas · 2023-07-09T08:19:49Z

There's some trickery with astroid's version but I'm not sure why readthedoc alone fail. Maybe bad caching. I'm half inclined to "just merge it ™️".

Pierre-Sassoulas · 2023-07-09T08:21:19Z

Waiting for #7767 and a rebase might be best.

github-actions · 2023-07-09T08:29:06Z

🤖 According to the primer, this change has no effect on the checked open source code. 🤖🎉

This comment was generated for commit abcfcc2

jacobtylerwalls · 2023-07-10T13:52:56Z

There's some trickery with astroid's version but I'm not sure why readthedoc alone fail.

Yeah, I didn't bother to pin the astroid commit in the docs/requirements.txt, so it was pulling from astroid 3.0.0a7. Should be good now 👍

agusdmb · 2023-09-12T20:18:47Z

tests/pyreverse/functional/package_diagrams/type_check_imports/packages.mmd

+  }
+  mod_b --> mod_a
+  mod_d --> mod_a
+  mod_c -.-> mod_a


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

agusdmb · 2023-09-12T20:26:40Z

pylint/pyreverse/mermaidjs_printer.py

@@ -24,6 +24,7 @@ class MermaidJSPrinter(Printer):
        EdgeType.ASSOCIATION: "--*",
        EdgeType.AGGREGATION: "--o",
        EdgeType.USES: "-->",
+        EdgeType.TYPE_DEPENDENCY: "-.->",


this is the actual wrong line

Thanks! Would you be willing to provide a PR?

yep... just did!
#9031

Pyreverse: Use empty arrows for type-checking imports

bec61ff

nickdrozd requested a review from DudeNr33 as a code owner July 4, 2023 20:17

Pierre-Sassoulas requested changes Jul 4, 2023

View reviewed changes

Pierre-Sassoulas added the pyreverse Related to pyreverse component label Jul 4, 2023

Use dashed lines instead of empty arrows

1a2b998

This comment has been minimized.

Sign in to view

Simplify branch logic

614354e

nickdrozd added the Enhancement ✨ Improvement to a component label Jul 5, 2023

nickdrozd changed the title ~~Pyreverse: Use empty arrows for type-checking imports~~ Pyreverse: Use dashed lines for type-checking imports Jul 5, 2023

This comment has been minimized.

Sign in to view

nickdrozd and others added 5 commits July 5, 2023 11:30

Start test

95663f9

Add mermaid writer

458a737

move test data

3701040

add config fixtures

3bb5b2c

[pre-commit.ci] auto fixes from pre-commit.com hooks

59b5ba1

for more information, see https://pre-commit.ci

This comment has been minimized.

Sign in to view

nickdrozd and others added 2 commits July 5, 2023 14:46

get test data from right place

611a353

[pre-commit.ci] auto fixes from pre-commit.com hooks

dc30497

for more information, see https://pre-commit.ci

This comment has been minimized.

Sign in to view

nickdrozd commented Jul 5, 2023

View reviewed changes

This was referenced Jul 6, 2023

[primer] Flaky results from inference differences #8790

Closed

Upgrade astroid to 3.0.0a6 #8822

Merged

Pierre-Sassoulas reviewed Jul 6, 2023

View reviewed changes

Pierre-Sassoulas added the Needs review 🔍 Needs to be reviewed by one or multiple more persons label Jul 6, 2023

nickdrozd added 2 commits July 6, 2023 09:04

Cut KeyError check

5dd3831

Add news

afb4632

Skip KeyError coverage

20d47fe

This comment has been minimized.

Sign in to view

DudeNr33 requested changes Jul 7, 2023

View reviewed changes

pylint/pyreverse/printer.py Outdated Show resolved Hide resolved

tests/pyreverse/conftest.py Outdated Show resolved Hide resolved

nickdrozd added 2 commits July 7, 2023 14:47

Rename type dependency

4057067

Cut type_check_imports_dot_config

abcfcc2

This comment has been minimized.

Sign in to view

nickdrozd requested review from DudeNr33 and Pierre-Sassoulas July 8, 2023 13:43

nickdrozd mentioned this pull request Jul 8, 2023

New check: variable used only for type checking #8111

Closed

Pierre-Sassoulas approved these changes Jul 8, 2023

View reviewed changes

DudeNr33 approved these changes Jul 8, 2023

View reviewed changes

DudeNr33 removed the Needs review 🔍 Needs to be reviewed by one or multiple more persons label Jul 9, 2023

Pierre-Sassoulas closed this Jul 9, 2023

Pierre-Sassoulas reopened this Jul 9, 2023

Pierre-Sassoulas added the Blocked 🚧 Blocked by a particular issue label Jul 9, 2023

Pierre-Sassoulas added this to the 3.0.0a7 milestone Jul 9, 2023

jacobtylerwalls removed the Blocked 🚧 Blocked by a particular issue label Jul 10, 2023

Pierre-Sassoulas merged commit ffe1e71 into pylint-dev:main Jul 11, 2023

agusdmb reviewed Sep 12, 2023

View reviewed changes

agusdmb mentioned this pull request Sep 12, 2023

Invalid mermaid arrow #9030

Closed

agusdmb reviewed Sep 12, 2023

View reviewed changes

agusdmb mentioned this pull request Sep 12, 2023

fix mermaid dotted arrow #9031

Merged

nickdrozd deleted the pyreverse-package-type-checking branch September 25, 2024 16:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

nickdrozd commented Jul 4, 2023 •

edited

Loading

Pierre-Sassoulas left a comment

codecov bot commented Jul 4, 2023 •

edited

Loading

This comment has been minimized.

nickdrozd commented Jul 5, 2023

This comment has been minimized.

This comment has been minimized.

nickdrozd commented Jul 5, 2023

DudeNr33 commented Jul 5, 2023

This comment has been minimized.

nickdrozd Jul 5, 2023

nickdrozd Jul 6, 2023

Pierre-Sassoulas left a comment

This comment has been minimized.

DudeNr33 commented Jul 7, 2023

DudeNr33 left a comment

This comment has been minimized.

DudeNr33 commented Jul 9, 2023

Pierre-Sassoulas commented Jul 9, 2023

Pierre-Sassoulas commented Jul 9, 2023

Pierre-Sassoulas commented Jul 9, 2023

github-actions bot commented Jul 9, 2023

jacobtylerwalls commented Jul 10, 2023 •

edited

Loading

agusdmb Sep 12, 2023

agusdmb Sep 12, 2023

jacobtylerwalls Sep 12, 2023

agusdmb Sep 12, 2023

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

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

Conversation

nickdrozd commented Jul 4, 2023 • edited Loading

Type of Changes

Description

TODO

Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

codecov bot commented Jul 4, 2023 • edited Loading

Codecov Report

This comment has been minimized.

nickdrozd commented Jul 5, 2023

This comment has been minimized.

This comment has been minimized.

nickdrozd commented Jul 5, 2023

DudeNr33 commented Jul 5, 2023

This comment has been minimized.

nickdrozd Jul 5, 2023

Choose a reason for hiding this comment

nickdrozd Jul 6, 2023

Choose a reason for hiding this comment

Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

This comment has been minimized.

DudeNr33 commented Jul 7, 2023

DudeNr33 left a comment

Choose a reason for hiding this comment

This comment has been minimized.

DudeNr33 commented Jul 9, 2023

Pierre-Sassoulas commented Jul 9, 2023

Pierre-Sassoulas commented Jul 9, 2023

Pierre-Sassoulas commented Jul 9, 2023

github-actions bot commented Jul 9, 2023

jacobtylerwalls commented Jul 10, 2023 • edited Loading

agusdmb Sep 12, 2023

Choose a reason for hiding this comment

agusdmb Sep 12, 2023

Choose a reason for hiding this comment

jacobtylerwalls Sep 12, 2023

Choose a reason for hiding this comment

agusdmb Sep 12, 2023

Choose a reason for hiding this comment

nickdrozd commented Jul 4, 2023 •

edited

Loading

codecov bot commented Jul 4, 2023 •

edited

Loading

jacobtylerwalls commented Jul 10, 2023 •

edited

Loading