From 07c14663f55e600ad0efbe10a9acdef9d3249b36 Mon Sep 17 00:00:00 2001 From: DudeNr33 <3929834+DudeNr33@users.noreply.github.com> Date: Tue, 22 Mar 2022 09:23:08 +0100 Subject: [PATCH 1/2] Pyreverse: better error messages for unsupported file formats --- ChangeLog | 4 ++ doc/whatsnew/2.13.rst | 4 ++ pylint/pyreverse/dot_printer.py | 3 +- pylint/pyreverse/main.py | 33 +++++++++---- pylint/pyreverse/utils.py | 32 ++++++++++++- tests/pyreverse/test_main.py | 83 +++++++++++++++++++++++++++++++++ 6 files changed, 145 insertions(+), 14 deletions(-) diff --git a/ChangeLog b/ChangeLog index c1917c330c..b82e345bb0 100644 --- a/ChangeLog +++ b/ChangeLog @@ -17,6 +17,10 @@ Release date: TBA Ref PyCQA/astroid#1360 Closes #4826 +* Output better error message if unsupported file formats are used with ``pyreverse``. + + Closes #5950 + * Fix pyreverse diagrams type hinting for classmethods and staticmethods. * Fix pyreverse diagrams type hinting for methods returning None. diff --git a/doc/whatsnew/2.13.rst b/doc/whatsnew/2.13.rst index 7b2f181653..a47516e528 100644 --- a/doc/whatsnew/2.13.rst +++ b/doc/whatsnew/2.13.rst @@ -515,3 +515,7 @@ Other Changes Closes #5333 * Fix type hints in class diagrams generated by pyreverse for class methods and methods returning None. + +* Output better error message if unsupported file formats are used with ``pyreverse``. + + Closes #5950 diff --git a/pylint/pyreverse/dot_printer.py b/pylint/pyreverse/dot_printer.py index fccdcf5579..21f41d765c 100644 --- a/pylint/pyreverse/dot_printer.py +++ b/pylint/pyreverse/dot_printer.py @@ -19,7 +19,7 @@ from astroid import nodes from pylint.pyreverse.printer import EdgeType, Layout, NodeProperties, NodeType, Printer -from pylint.pyreverse.utils import check_graphviz_availability, get_annotation_label +from pylint.pyreverse.utils import get_annotation_label ALLOWED_CHARSETS: FrozenSet[str] = frozenset(("utf-8", "iso-8859-1", "latin1")) SHAPES: Dict[NodeType, str] = { @@ -152,7 +152,6 @@ def generate(self, outputfile: str) -> None: with open(dot_sourcepath, "w", encoding="utf8") as outfile: outfile.writelines(self.lines) if target not in graphviz_extensions: - check_graphviz_availability() use_shell = sys.platform == "win32" subprocess.call( ["dot", "-T", target, dot_sourcepath, "-o", outputfile], diff --git a/pylint/pyreverse/main.py b/pylint/pyreverse/main.py index e955388cb0..aefffb315d 100644 --- a/pylint/pyreverse/main.py +++ b/pylint/pyreverse/main.py @@ -32,7 +32,20 @@ from pylint.pyreverse import writer from pylint.pyreverse.diadefslib import DiadefsHandler from pylint.pyreverse.inspector import Linker, project_from_files -from pylint.pyreverse.utils import check_graphviz_availability, insert_default_options +from pylint.pyreverse.utils import ( + check_graphviz_availability, + check_if_graphviz_supports_format, + insert_default_options, +) + +DIRECTLY_SUPPORTED_FORMATS = ( + "dot", + "vcg", + "puml", + "plantuml", + "mmd", + "html", +) OPTIONS = ( ( @@ -139,7 +152,10 @@ action="store", default="dot", metavar="", - help="create a *. output file if format available.", + help=( + f"create a *. output file if format is available. Available formats are: {', '.join(DIRECTLY_SUPPORTED_FORMATS)}. " + f"Any other format will be tried to create by means of the 'dot' command line tool, which requires a graphviz installation." + ), ), ), ( @@ -205,15 +221,12 @@ def __init__(self, args: Iterable[str]): super().__init__(usage=__doc__) insert_default_options() args = self.load_command_line_configuration(args) - if self.config.output_format not in ( - "dot", - "vcg", - "puml", - "plantuml", - "mmd", - "html", - ): + if self.config.output_format not in DIRECTLY_SUPPORTED_FORMATS: check_graphviz_availability() + print( + f"Format {self.config.output_format} is not supported natively. Pyreverse will try to generate it using Graphviz..." + ) + check_if_graphviz_supports_format(self.config.output_format) sys.exit(self.run(args)) diff --git a/pylint/pyreverse/utils.py b/pylint/pyreverse/utils.py index 7fde44225b..76a78bdcee 100644 --- a/pylint/pyreverse/utils.py +++ b/pylint/pyreverse/utils.py @@ -23,6 +23,7 @@ import os import re import shutil +import subprocess import sys from typing import Optional, Union @@ -292,7 +293,34 @@ def check_graphviz_availability(): if shutil.which("dot") is None: print( "The requested output format is currently not available.\n" - "Please install 'Graphviz' to have other output formats " - "than 'dot' or 'vcg'." + "Please install 'Graphviz' to have image output formats." + ) + sys.exit(32) + + +def check_if_graphviz_supports_format(output_format: str) -> None: + """Check if the ``dot`` command supports the requested output format. + + This is needed if image output is desired and ``dot`` is used to convert + from *.gv into the final output format. + """ + dot_output = subprocess.run( + ["dot", "-T?"], capture_output=True, check=False, encoding="utf-8" + ) + match = re.match( + pattern=r".*Use one of: (?P(\S*\s?)+)", + string=dot_output.stderr.strip(), + ) + if not match: + print( + "Unable to determine Graphviz supported output formats. " + "Pyreverse will continue, but subsequent error messages " + "regarding the output format may come from Graphviz directly." + ) + return + supported_formats = match.group("formats") + if output_format not in supported_formats.split(): + print( + f"Format {output_format} is not supported by Graphviz. It supports: {supported_formats}" ) sys.exit(32) diff --git a/tests/pyreverse/test_main.py b/tests/pyreverse/test_main.py index 01f3ccc099..84ec34c88a 100644 --- a/tests/pyreverse/test_main.py +++ b/tests/pyreverse/test_main.py @@ -2,15 +2,39 @@ import os import sys from typing import Iterator +from unittest import mock import pytest from pylint.lint import fix_import_path +from pylint.pyreverse import main TEST_DATA_DIR = os.path.abspath(os.path.join(os.path.dirname(__file__), "..", "data")) PROJECT_ROOT_DIR = os.path.abspath(os.path.join(TEST_DATA_DIR, "..")) +@pytest.fixture(name="mock_subprocess") +def mock_utils_subprocess(): + with mock.patch("pylint.pyreverse.utils.subprocess") as mock_subprocess: + yield mock_subprocess + + +@pytest.fixture +def mock_graphviz(mock_subprocess): + mock_subprocess.run.return_value = mock.Mock( + stderr=( + 'Format: "XYZ" not recognized. Use one of: ' + "bmp canon cgimage cmap cmapx cmapx_np dot dot_json eps exr fig gd " + "gd2 gif gv icns ico imap imap_np ismap jp2 jpe jpeg jpg json json0 " + "mp pct pdf pic pict plain plain-ext png pov ps ps2 psd sgi svg svgz " + "tga tif tiff tk vdx vml vmlz vrml wbmp webp xdot xdot1.2 xdot1.4 xdot_json" + ) + ) + with mock.patch("pylint.pyreverse.utils.shutil") as mock_shutil: + mock_shutil.which.return_value = "/usr/bin/dot" + yield + + @pytest.fixture(params=[PROJECT_ROOT_DIR, TEST_DATA_DIR]) def setup_path(request) -> Iterator: current_sys_path = list(sys.path) @@ -29,3 +53,62 @@ def test_project_root_in_sys_path(): """ with fix_import_path([TEST_DATA_DIR]): assert sys.path == [PROJECT_ROOT_DIR] + + +@mock.patch("pylint.pyreverse.main.Linker", new=mock.MagicMock()) +@mock.patch("pylint.pyreverse.main.DiadefsHandler", new=mock.MagicMock()) +@mock.patch("pylint.pyreverse.main.writer") +@pytest.mark.usefixtures("mock_graphviz") +def test_graphviz_supported_image_format(mock_writer, capsys): + """Test that Graphviz is used if the image format is supported.""" + with pytest.raises(SystemExit): + # we have to catch the SystemExit so the test execution does not stop + main.Run(["-o", "png", TEST_DATA_DIR]) + # Check that the right info message is shown to the user + assert ( + "Format png is not supported natively. Pyreverse will try to generate it using Graphviz..." + in capsys.readouterr().out + ) + # Check that pyreverse actually made the call to create the diagram + mock_writer.DiagramWriter().write.assert_called_once() + + +@mock.patch("pylint.pyreverse.main.Linker", new=mock.MagicMock()) +@mock.patch("pylint.pyreverse.main.DiadefsHandler", new=mock.MagicMock()) +@mock.patch("pylint.pyreverse.main.writer") +@pytest.mark.usefixtures("mock_graphviz") +def test_graphviz_cant_determine_supported_formats( + mock_writer, mock_subprocess, capsys +): + """Test that Graphviz is used if the image format is supported.""" + mock_subprocess.run.return_value.stderr = "..." + with pytest.raises(SystemExit): + # we have to catch the SystemExit so the test execution does not stop + main.Run(["-o", "png", TEST_DATA_DIR]) + # Check that the right info message is shown to the user + assert ( + "Unable to determine Graphviz supported output formats." + in capsys.readouterr().out + ) + # Check that pyreverse actually made the call to create the diagram + mock_writer.DiagramWriter().write.assert_called_once() + + +@mock.patch("pylint.pyreverse.main.Linker", new=mock.MagicMock()) +@mock.patch("pylint.pyreverse.main.DiadefsHandler", new=mock.MagicMock()) +@mock.patch("pylint.pyreverse.main.writer", new=mock.MagicMock()) +@pytest.mark.usefixtures("mock_graphviz") +def test_graphviz_unsupported_image_format(capsys): + """Test that Graphviz is used if the image format is supported.""" + with pytest.raises(SystemExit) as wrapped_sysexit: + # we have to catch the SystemExit so the test execution does not stop + main.Run(["-o", "somethingElse", TEST_DATA_DIR]) + # Check that the right info messages are shown to the user + stdout = capsys.readouterr().out + assert ( + "Format somethingElse is not supported natively. Pyreverse will try to generate it using Graphviz..." + in stdout + ) + assert "Format somethingElse is not supported by Graphviz. It supports:" in stdout + # Check that we exited with the expected error code + assert wrapped_sysexit.value.code == 32 From b0b408663c26fcb0cf59e79956eca5f6e0125881 Mon Sep 17 00:00:00 2001 From: DudeNr33 <3929834+DudeNr33@users.noreply.github.com> Date: Tue, 22 Mar 2022 19:32:18 +0100 Subject: [PATCH 2/2] Apply suggestions from code review. --- pylint/pyreverse/utils.py | 5 +---- tests/pyreverse/test_main.py | 10 ++++++---- 2 files changed, 7 insertions(+), 8 deletions(-) diff --git a/pylint/pyreverse/utils.py b/pylint/pyreverse/utils.py index 76a78bdcee..f7eff0ee0b 100644 --- a/pylint/pyreverse/utils.py +++ b/pylint/pyreverse/utils.py @@ -291,10 +291,7 @@ def check_graphviz_availability(): from *.dot or *.gv into the final output format. """ if shutil.which("dot") is None: - print( - "The requested output format is currently not available.\n" - "Please install 'Graphviz' to have image output formats." - ) + print("'Graphviz' needs to be installed for your chosen output format.") sys.exit(32) diff --git a/tests/pyreverse/test_main.py b/tests/pyreverse/test_main.py index 84ec34c88a..2cc422c4ac 100644 --- a/tests/pyreverse/test_main.py +++ b/tests/pyreverse/test_main.py @@ -61,7 +61,7 @@ def test_project_root_in_sys_path(): @pytest.mark.usefixtures("mock_graphviz") def test_graphviz_supported_image_format(mock_writer, capsys): """Test that Graphviz is used if the image format is supported.""" - with pytest.raises(SystemExit): + with pytest.raises(SystemExit) as wrapped_sysexit: # we have to catch the SystemExit so the test execution does not stop main.Run(["-o", "png", TEST_DATA_DIR]) # Check that the right info message is shown to the user @@ -69,8 +69,9 @@ def test_graphviz_supported_image_format(mock_writer, capsys): "Format png is not supported natively. Pyreverse will try to generate it using Graphviz..." in capsys.readouterr().out ) - # Check that pyreverse actually made the call to create the diagram + # Check that pyreverse actually made the call to create the diagram and we exit cleanly mock_writer.DiagramWriter().write.assert_called_once() + assert wrapped_sysexit.value.code == 0 @mock.patch("pylint.pyreverse.main.Linker", new=mock.MagicMock()) @@ -82,7 +83,7 @@ def test_graphviz_cant_determine_supported_formats( ): """Test that Graphviz is used if the image format is supported.""" mock_subprocess.run.return_value.stderr = "..." - with pytest.raises(SystemExit): + with pytest.raises(SystemExit) as wrapped_sysexit: # we have to catch the SystemExit so the test execution does not stop main.Run(["-o", "png", TEST_DATA_DIR]) # Check that the right info message is shown to the user @@ -90,8 +91,9 @@ def test_graphviz_cant_determine_supported_formats( "Unable to determine Graphviz supported output formats." in capsys.readouterr().out ) - # Check that pyreverse actually made the call to create the diagram + # Check that pyreverse actually made the call to create the diagram and we exit cleanly mock_writer.DiagramWriter().write.assert_called_once() + assert wrapped_sysexit.value.code == 0 @mock.patch("pylint.pyreverse.main.Linker", new=mock.MagicMock())