pyreverse: better error messages for unsupported file formats
#5951
Conversation
| @@ -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() | |||
There was a problem hiding this comment.
We already check that in the Run class. No need to do it twice.
| help=( | ||
| f"create a *.<format> 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." | ||
| ), |
There was a problem hiding this comment.
We did not state the directly supported output formats in the help output previously, I think this also makes it a bit clearer what is going on under the hood.
pylint/pyreverse/utils.py
Outdated
| @@ -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." | |||
There was a problem hiding this comment.
The message was outdated as we now support other output formats as well. Reiterating the directly supported output formats here is not necessary I think, so I kept it more general.
There was a problem hiding this comment.
| "Please install 'Graphviz' to have image output formats." | |
| "'Graphviz' need to be installed for your chosen output format." |
| "Pyreverse will continue, but subsequent error messages " | ||
| "regarding the output format may come from Graphviz directly." | ||
| ) | ||
| return |
There was a problem hiding this comment.
mypy forced me to include this. 😉
Relying on the textual output of the dot command to determine the supported output formats is probably not the best solution, but I'd argue the tradeoff for more user friendly error messages and ability to fail fast is worth it.
But if you have concerns or feedback please feel free to address it.
pylint/pyreverse/utils.py
Outdated
| @@ -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." | |||
There was a problem hiding this comment.
| "Please install 'Graphviz' to have image output formats." | |
| "'Graphviz' need to be installed for your chosen output format." |
tests/pyreverse/test_main.py
Outdated
| 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 ( |
There was a problem hiding this comment.
| 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 ( | |
| with pytest.raises(SystemExit) as exc_info: | |
| # 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 exc_info.value.code == 2 # (?) | |
| assert ( |
I'll let you decide if it's worth it, there's 3 similar assert that could be done if you want to assert that the error code is correct.
There was a problem hiding this comment.
I included the check.
CONTRIBUTORS.txtif you are a new contributor.doc/whatsnew/<current release.rst>.Type of Changes
Description
When supplying any format not directly known to
pyreverse, we will try to convert it using Graphviz (if available).Until now we just called the
dotcommand and presented any error messages from it directly to the user, which can be confusing if it complains about unsupported file formats.This PR aims to make the error messages clearer and to fail fast if we can already tell that the format is not supported by Graphviz.
Closes #5950