Create common `Printer` base class for `pyreverse` and improve typing. #4731

DudeNr33 · 2021-07-20T20:02:53Z

Steps

Write a good description on what the PR does.

Description

This is the first MR which shall split MR #4615 into smaller and more easily reviewable parts.
It involves the necessary refactoring which is needed to prepare for the new features (PlantUML support and colorization).
It also adds type annotations.

Summary of changes:

Rename vcgutils.py to printer.py and introduce a Printer base class which defines the common interface for the different backends.
Adapt VCGPrinter to adhere to the new printer base class
Add a DotPrinter class which essentially implements the features of pylint.graph.DotBackend. The latter will be removed in a future MR, as it is currently also used by the ImportsChecker and I wanted to keep this MR smaller and focused (it is already not very small...).
Add type annotations for the Printer class and its children. This also uncovered some problems where mypy correctly complained, so I had to add specialised PackageEntity and ClassEntity child classes in diagrams.py.
Added tests for VCG output, which was missing before
Updated the test data as the new Printer backends now outputs some more information. The final image produced by Graphviz is still the same.

Type of Changes

	Type
✓	🔨 Refactoring

coveralls · 2021-07-20T20:09:18Z

Coverage increased (+0.3%) to 92.573% when pulling 5f624b7 on DudeNr33:pyreverse-refactoring-printer into 2ad8247 on PyCQA:main.

Pierre-Sassoulas

Just a small comment, and I did not look in detail but the new typing is 🔥 , thanks! I'll review when this is no longer a draft :)

pylint/pyreverse/utils.py

…athlib.Path().suffix``

Pierre-Sassoulas

Hey, thank you for this work on this and sorry for the delay before reviewing. Could we reorganize and move the code around first then do the modification on it ? I'd be more confident if there was a proper diff for the change in the printer.py file, right now everything is considered new by git and it's hard to tell what changed exactly :)

Pierre-Sassoulas · 2021-07-25T09:00:53Z

pylint/pyreverse/printer.py

+    body: Optional[str] = None
+
+
+class Printer:


Suggested change

class Printer:

class Printer(abc.ABC):

It's more explicit than raising NotImplementErrors :)

Pierre-Sassoulas · 2021-07-25T09:02:20Z

pylint/pyreverse/printer.py

+        raise NotImplementedError
+
+
+class VCGPrinter(Printer):


We can create a new file for VCGPrinter and another one for DotPrinter :) (It would also help with the diff and possible future conflict if vcgutils.py is recognized as the old file for it).

Pierre-Sassoulas · 2021-07-25T09:07:57Z

pylint/pyreverse/writer.py

+    def save(self) -> None:
+        """write to disk"""
+        self.printer.generate(self.file_name)
+

 class DotWriter(DiagramWriter):


Maybe we could start with a first merge request that create the following architecture and move relevant part of the code without modifying anything:

pyreverse: writter: diagram_writter.py dot_writter.py vcg_writter.py printer: dot_printer.py vcg_printer.py

What do you think ?

Thank you for reorganizing the code the diff is manageable now :)

DudeNr33 · 2021-07-28T18:37:44Z

Thanks for the feedback. I will implement it when #4745 is merged and I did a rebase.

…nd move ``VCGPrinter`` back into original ``vcgutils.py`` for easier diffing

…ImplementedError``

DudeNr33 · 2021-08-01T13:19:33Z

pylint/pyreverse/diagrams.py

+        super().__init__(title=title, node=node)
+        self.attrs = None
+        self.methods = None
+


Adding the type hints revealed that it was necessary to distinguish between a PackageEntity and a ClassEntity, because the ClassEntity has additional attributes that were dynamically added in the previous code, which confused mypy.

DudeNr33 · 2021-08-01T13:23:13Z

pylint/pyreverse/writer.py

+            color="black",
+        )
+
+    def get_class_properties(self, obj: ClassEntity) -> NodeProperties:
        """get label and shape for classes.


get_values is now get_class_properties.

get_package_properties is currently trivial and the color is hardcoded, but this will change when the new feature for automatic coloring are merged.

DudeNr33 · 2021-08-01T13:31:41Z

pylint/pyreverse/writer.py

-            dict(arrowstyle="solid", backarrowstyle="none", textcolor="green"),
-        ]
-        DiagramWriter.__init__(self, config, styles)
-


The __init__ is now obsolete. It formerly defined the styles to apply to the different types of edges for displaying relationships.
These are now defined in the ARROWS dictionary in the module of the corresponding printer class.

DudeNr33 · 2021-08-01T13:35:58Z

I implemented most of the feedback.
I reintroduced the module vcgutils.py, which should make it now easier to diff.
As soon as the review is finished, I would suggest to make a last commit where this file is simply renamed to vcg_printer.py.

Regarding the last suggestion (doing a first MR to implement the structure

pyreverse:
    writter:
        diagram_writter.py
        dot_writter.py
        vcg_writter.py
    printer:
        dot_printer.py
        vcg_printer.py

):

If the current cleanup is not enough to properly review the changes, let me know and I can do that.

Pierre-Sassoulas

Thank you for your work on this, very impressive ! Ad thank you for making the diff manageable. I left some minor comment. We can still do the architecture change later if you feel like it (probably after merging the other MR as the rebase will be hard to do if we do it before).

Pierre-Sassoulas · 2021-08-01T18:10:03Z

pylint/pyreverse/vcgutils.py

                )

    def _inc_indent(self):
        """increment indentation"""
-        self._indent = "  %s" % self._indent
+        self._indent += "  "


Pierre-Sassoulas · 2021-08-01T18:12:09Z

pylint/pyreverse/writer.py

@@ -140,12 +158,14 @@ def get_values(self, obj):
                )

                if func.args.args:
-                    args = [arg for arg in func.args.args if arg.name != "self"]
+                    argument_list = [


Suggested change

argument_list = [

arguments: List[str] = [

Personal preference but when naming an iterable I like a plural word so I can do for argument in arguments later on.

I agree, changed it.

Pierre-Sassoulas · 2021-08-01T18:14:48Z

pylint/pyreverse/dot_printer.py

+    EdgeType.ASSOCIATION: dict(
+        fontcolor="green", arrowtail="none", arrowhead="diamond", style="solid"
+    ),
+    EdgeType.USES: dict(arrowtail="none", arrowhead="open"),


Maybe we could create enums for fontcolor, arrowtail, style and arrowhead ?

pylint/pyreverse/dot_printer.py

pylint/pyreverse/vcgutils.py

pylint/pyreverse/writer.py

Pierre-Sassoulas · 2021-08-01T18:23:55Z

pylint/pyreverse/writer.py

+    def save(self) -> None:
+        """write to disk"""
+        self.printer.generate(self.file_name)
+

 class DotWriter(DiagramWriter):


Thank you for reorganizing the code the diff is manageable now :)

DudeNr33 · 2021-08-01T19:14:22Z

Thank you very much for your review!
The only part I missed in the last push is the request for creating Enums for fontcolor, arrowhead etc.

I also need to rename the vcgutils.py into vcg_printer.py, so please don't merge yet, will do that in the next few days!

Pierre-Sassoulas · 2021-08-01T19:19:45Z

No problem, just request a new review when the code is ready :)

DudeNr33 · 2021-08-03T04:29:06Z

I thought about implementing enums for arrowstyle, arrowhead and so on a little longer.
In my opinion there is not much benefit from it in this case. We would have to create those enums for each printer separately, as the names and values for the arguments are different. And then they would only be used in the ARROWS dictionary, of which the items are not really individually processed:

# in vcgutils.py
attributes = ARROWS[type_]
if label:
    attributes["label"] = label
self._write_attributes(
    EDGE_ATTRS,
    **attributes,
)

# in dot_printer.py
arrowstyle = ARROWS[type_]
attrs = [f'{prop}="{value}"' for prop, value in arrowstyle.items()]

I think we would just clutter the modules with enum definitions, without gaining much in terms of readability or reusability.

vcgutils.py has been renamed to vcg_printer.py.

Pierre-Sassoulas

Thank you for making a first step easier to review :) !

Create common Printer base class for Pyreverse, and improve typing.

649b6e7

Pierre-Sassoulas reviewed Jul 20, 2021

View reviewed changes

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

Implement review suggestion - replace get_file_extension with ``p…

30a1c25

…athlib.Path().suffix``

DudeNr33 marked this pull request as ready for review July 21, 2021 03:08

Pierre-Sassoulas added Enhancement ✨ Improvement to a component Maintenance Discussion or action around maintaining pylint or the dev workflow labels Jul 25, 2021

Pierre-Sassoulas requested changes Jul 25, 2021

View reviewed changes

DudeNr33 added 4 commits August 1, 2021 14:43

Merge branch 'main' into pyreverse-refactoring-printer

f3c3bcb

Create separate file for Printer base class and DotPrinter, a…

d223220

…nd move ``VCGPrinter`` back into original ``vcgutils.py`` for easier diffing

Use abc.ABC as metaclass for Printer instead of raising ``Not…

7fff703

…ImplementedError``

Move some methods back to original position to make diffing easier

1672353

DudeNr33 commented Aug 1, 2021

View reviewed changes

Pierre-Sassoulas requested changes Aug 1, 2021

View reviewed changes

Implement suggested changes from review.

e299c64

Rename vcgutils.py to vcg_printer.py

5f624b7

DudeNr33 requested a review from Pierre-Sassoulas August 3, 2021 04:40

Pierre-Sassoulas approved these changes Aug 3, 2021

View reviewed changes

Pierre-Sassoulas merged commit 7bb5043 into pylint-dev:main Aug 3, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create common `Printer` base class for `pyreverse` and improve typing. #4731

Create common `Printer` base class for `pyreverse` and improve typing. #4731

DudeNr33 commented Jul 20, 2021

coveralls commented Jul 20, 2021 •

edited

Pierre-Sassoulas left a comment

Pierre-Sassoulas left a comment

Pierre-Sassoulas Jul 25, 2021

Pierre-Sassoulas Jul 25, 2021

Pierre-Sassoulas Jul 25, 2021

Pierre-Sassoulas Aug 1, 2021

DudeNr33 commented Jul 28, 2021

DudeNr33 Aug 1, 2021

DudeNr33 Aug 1, 2021

DudeNr33 Aug 1, 2021

DudeNr33 commented Aug 1, 2021

Pierre-Sassoulas left a comment

Pierre-Sassoulas Aug 1, 2021

Pierre-Sassoulas Aug 1, 2021

DudeNr33 Aug 1, 2021

Pierre-Sassoulas Aug 1, 2021

Pierre-Sassoulas Aug 1, 2021

DudeNr33 commented Aug 1, 2021

Pierre-Sassoulas commented Aug 1, 2021

DudeNr33 commented Aug 3, 2021 •

edited

Pierre-Sassoulas left a comment

Create common Printer base class for pyreverse and improve typing. #4731

Create common Printer base class for pyreverse and improve typing. #4731

Conversation

DudeNr33 commented Jul 20, 2021

Steps

Description

Type of Changes

coveralls commented Jul 20, 2021 • edited

Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DudeNr33 commented Jul 28, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DudeNr33 commented Aug 1, 2021

Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DudeNr33 commented Aug 1, 2021

Pierre-Sassoulas commented Aug 1, 2021

DudeNr33 commented Aug 3, 2021 • edited

Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

Create common `Printer` base class for `pyreverse` and improve typing. #4731

Create common `Printer` base class for `pyreverse` and improve typing. #4731

coveralls commented Jul 20, 2021 •

edited

DudeNr33 commented Aug 3, 2021 •

edited