Pyreverse autocolor #4521

DudeNr33 · 2021-05-29T09:59:04Z

Steps

Add a ChangeLog entry describing what your PR does.
If it's a new feature or an important bug fix, add a What's New entry in
doc/whatsnew/<current release.rst>.
Write a good description on what the PR does.

Description

Running pyreverse on large projects can lead to diagrams with low clarity. Classes or modules might be scattered across the diagram.
By coloring the diagrams, modules or classes of the same package can be easily spotted visually. For an example, see issue #4488.

This new feature is optional and can be activated by passing the --colorized option.
The granularity can be influenced by passing --max-color-depth.
Classes/modules from the standard library are always colored in grey.
However, this seems to not work reliably, at least it did not work inside the tox test environments (I therefore had to delete the unittest for it). It worked great in my venv that I use for development.

This feature only works with dot output. I could not find a suitable program that correctly displays the vcg files.
As there were no unittests for the VCG output as well (are VCG graphs even still a thing? Google finds relatively few discussions for it...), I chose to not implement it for this output format.
I added a regression test which ensures that the VCG files produced with the current Pylint release and after this PR are the same.

Type of Changes

	Type
✓	✨ New feature
✓	🔨 Refactoring

Related Issue

Closes #4488

Distinguish modules and packages.

…ibute in VCGPrinter

…=y option

…modules is checked

… test does not work in tox env

…hod ``get_package_properties``. Rename ``get_values`` to ``get_class_properties``. VCGWriter and VCGPrinter thus no longer need special handling.

Add ChangeLog and whatsnew entry

coveralls · 2021-05-29T10:04:25Z

Coverage increased (+0.4%) to 92.227% when pulling 2dcfdb0 on DudeNr33:pyreverse-autocolor into 4f8ab69 on PyCQA:master.

Pierre-Sassoulas

Hey sorry for the delay in order to review. This looks nice and well tested, I think the pytest fixture were probably not used before (I see import from unittest_pyreverse_writer that would not happen if there were proper fixtures already). You can do a small refactor to move to pytest first if you have time for that or simply introduce a default_config fixture instead of the new _DEFAULT import.

Pierre-Sassoulas · 2021-06-14T07:32:24Z

pylint/pyreverse/writer.py

@@ -123,7 +153,41 @@ def get_title(self, obj):
        """get project title"""
        return obj.title

-    def get_values(self, obj):
+    def get_style(self):


Suggested change

def get_style(self):

def get_style(self) -> str:

Could you add typing on new functions please ? :)

Pierre-Sassoulas · 2021-06-14T07:33:52Z

pylint/pyreverse/writer.py

-    def get_values(self, obj):
+    def get_package_properties(self, obj):
+        """get label and shape for packages."""
+        return dict(


I think it would be better to use NamedTuple (or a child class) here, so we can have proper typing and not use dict everywhere.

I agree that a NamedTuple would be the cleaner solution. However, the current (as in: this PR) implementation of the DiagramWriter class and implementations of DotBackend and VCGPrinter do not allow for this. DotBackend and VCGPrinter rely on the data returned by the get_package_properties and get_class_properties methods, and the data structures they require are not the same.

That being said: I have already started working on the PlantUML backend, and while doing so I extracted a common interface for the Printer classes. Using a NamedTuple should be possible then.

I agree with your other comment, it your implementation of plantuml make you upgrade the code with NamedTuple and proper typing it will be nice to rebase on it once it's merged.

Pierre-Sassoulas · 2021-06-14T07:34:58Z

tests/unittest_pyreverse_diadefs.py

@@ -21,7 +21,7 @@

 import astroid
 import pytest
-from unittest_pyreverse_writer import Config, get_project
+from unittest_pyreverse_writer import _DEFAULTS, Config, get_project


Shouldn't this be defined in conftest.py and used as a pytest fixtures?

Yes, that would be a good idea to keep the dependencies of the test modules as minimal as possible. I will do that.

DudeNr33 · 2021-06-15T16:27:21Z

No problem, I fully understand that pyreverse is not the most important part of pylint. ;)

As I noted above, I already started working on the PlantUML implementation, and introducing NamedTuple in favor of dicts should be way easier there. My branch for PlantUML also already includes the changes of this PR.
My suggestion would be:
We close this PR, and I will implement your review feedback in the PlantUML branch.
I can then submit a new PR with both the coloring and the PlantUML support.

DudeNr33 · 2021-06-20T16:49:38Z

@Pierre-Sassoulas quick question - I extracted the tests into a separate subdirectory and added a new conftest.py there.
Now mypy reports an error:

tests/conftest.py: error: Duplicate module named 'conftest' (also at 'tests/pyreverse/conftest.py')

As conftest.py modules may very well exist at different levels, I would go ahead and apply the suggested fix with the --exclude option.
But before I change anything of pylints static analysis rules, I'd like to double check if that's OK.

Pierre-Sassoulas · 2021-06-20T16:55:29Z

It's a known problem with mypy, I don't know if you can exclude the file, if you can that's great, and I did not think of that because last time I put all my fixtures tests/conftests.py, to make mypy happy 😄

DudeNr33 · 2021-06-20T19:53:47Z

Yes, excluding works fine via the exclude option at the end of the mypy block in the pre-commit-config.yaml:

  - repo: https://github.com/pre-commit/mirrors-mypy
    rev: v0.812
    hooks:
      - id: mypy
        name: mypy
        entry: mypy
        language: python
        types: [python]
        args: ["--ignore-missing-imports", "--scripts-are-modules"]
        require_serial: true
        additional_dependencies: []
        exclude: tests/functional/|tests/input|tests/extensions/data|tests/regrtest_data/|conftest.py|doc/|bin/

Pierre-Sassoulas · 2021-06-20T20:18:59Z

Ha good to know, thanks ! Maybe we can ignore only some of them and keep analyzing test/conftests.py ? (Ie: Change |conftest.py| by |tests/pyreverse/conftest.py| ?)

DudeNr33 · 2021-06-21T18:03:40Z

I see, being able to at least type check one conftest.py would still be better than ignoring all of them.
I would not like to add every new conftest.py by hand, but I guess using tests(/.*)+/conftest.py should work.
This would still check the tests/conftest.py, but ignore alle conftest.py which are in subdirectories of test.

Pierre-Sassoulas · 2021-06-26T18:37:52Z

Hey, just a heads up, we just merged #4551 so you'll probably want to rebase :)

DudeNr33 · 2021-06-27T12:54:59Z

Thanks Pierre, I just submitted the new PR which also includes this feature as well as the requested changes in the review.
This PR can be closed.

DudeNr33 added 15 commits May 24, 2021 11:17

WIP: automatically color package diagrams in pyreverse

53e99b4

Add command line options --colorized and -max-color-depth.

832202b

Distinguish modules and packages.

Dummy implementation of new methods for VCG writer; ignore style attr…

7010985

…ibute in VCGPrinter

Add simple unit test for colored output

72e2d01

Add test files for colorized output

71d6d29

Apply consistent definition of dictionaries

bc0febf

Fix unittest that also relied on the Config class of another test module

128ea44

Enable coloring of class diagrams without relying on the module-names…

d2763be

…=y option

Modify test data and config so that colorization of standard library …

738e7b9

…modules is checked

Add regression tests for vcg output

36c3688

Merge branch 'master' into pyreverse-autocolor

a32ca13

Delete class inheriting from standard library in test data, since the…

225ef45

… test does not work in tox env

Refactoring: extract logic what attributes to set for packages in met…

e8e898f

…hod ``get_package_properties``. Rename ``get_values`` to ``get_class_properties``. VCGWriter and VCGPrinter thus no longer need special handling.

Add ChangeLog and whatsnew entrAdd ChangeLog and whatsnew entryy

e70c135

Add ChangeLog and whatsnew entry

Merge branch 'master' into pyreverse-autocolor

2dcfdb0

DudeNr33 marked this pull request as ready for review May 29, 2021 10:06

Pierre-Sassoulas mentioned this pull request Jun 11, 2021

Use Python type hints for UML generation #1548

Closed

Pierre-Sassoulas requested changes Jun 14, 2021

View reviewed changes

Pierre-Sassoulas added Enhancement ✨ Improvement to a component pyreverse Related to pyreverse component labels Jun 26, 2021

DudeNr33 mentioned this pull request Jun 27, 2021

Pyreverse: PlantUML support + coloring (from PR #4521) #4615

Closed

3 tasks

DudeNr33 closed this Jun 27, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pyreverse autocolor #4521

Pyreverse autocolor #4521

DudeNr33 commented May 29, 2021

coveralls commented May 29, 2021

Pierre-Sassoulas left a comment

Pierre-Sassoulas Jun 14, 2021

DudeNr33 Jun 15, 2021

Pierre-Sassoulas Jun 14, 2021

DudeNr33 Jun 15, 2021

Pierre-Sassoulas Jun 15, 2021

Pierre-Sassoulas Jun 14, 2021

DudeNr33 Jun 15, 2021

DudeNr33 commented Jun 15, 2021

DudeNr33 commented Jun 20, 2021

Pierre-Sassoulas commented Jun 20, 2021

DudeNr33 commented Jun 20, 2021

Pierre-Sassoulas commented Jun 20, 2021

DudeNr33 commented Jun 21, 2021

Pierre-Sassoulas commented Jun 26, 2021

DudeNr33 commented Jun 27, 2021

Pyreverse autocolor #4521

Pyreverse autocolor #4521

Conversation

DudeNr33 commented May 29, 2021

Steps

Description

Type of Changes

Related Issue

coveralls commented May 29, 2021

Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

Pierre-Sassoulas Jun 14, 2021

Choose a reason for hiding this comment

DudeNr33 Jun 15, 2021

Choose a reason for hiding this comment

Pierre-Sassoulas Jun 14, 2021

Choose a reason for hiding this comment

DudeNr33 Jun 15, 2021

Choose a reason for hiding this comment

Pierre-Sassoulas Jun 15, 2021

Choose a reason for hiding this comment

Pierre-Sassoulas Jun 14, 2021

Choose a reason for hiding this comment

DudeNr33 Jun 15, 2021

Choose a reason for hiding this comment

DudeNr33 commented Jun 15, 2021

DudeNr33 commented Jun 20, 2021

Pierre-Sassoulas commented Jun 20, 2021

DudeNr33 commented Jun 20, 2021

Pierre-Sassoulas commented Jun 20, 2021

DudeNr33 commented Jun 21, 2021

Pierre-Sassoulas commented Jun 26, 2021

DudeNr33 commented Jun 27, 2021