From 43240cf3d52260402008d21e9d378ca99abf5fd1 Mon Sep 17 00:00:00 2001 From: Richard Shi <63933334+sushimon@users.noreply.github.com> Date: Sun, 9 Jul 2023 21:16:08 -0400 Subject: [PATCH] Fixed PythonTA crash when reporting error messages in config files (#920) --- CHANGELOG.md | 1 + python_ta/__init__.py | 24 +++++++- python_ta/reporters/core.py | 13 ++++- .../test_json_with_errors.pylintrc | 16 ++++++ .../test_plain_with_errors.pylintrc | 16 ++++++ .../file_fixtures/test_with_errors.pylintrc | 16 ++++++ tests/test_config/test_config.py | 57 +++++++++++++++++++ 7 files changed, 139 insertions(+), 4 deletions(-) create mode 100644 tests/test_config/file_fixtures/test_json_with_errors.pylintrc create mode 100644 tests/test_config/file_fixtures/test_plain_with_errors.pylintrc create mode 100644 tests/test_config/file_fixtures/test_with_errors.pylintrc diff --git a/CHANGELOG.md b/CHANGELOG.md index 6cd34c790..ce6f69f23 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -30,6 +30,7 @@ and adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). ### Bug Fixes - Fixed bug where running `python3 -m python_ta --generate-config` yields a `FileNotFoundError`. +- Fixed bug in how PythonTA reports error messages that occur when parsing configuration files. - Fixed bug where the HTML reporter would display all error occurrences of the same type despite stating that only a limited number was being shown. - Fixed bug where the JSON reporter was not limiting the number of error occurrences displayed with respect to `pyta-number-of-messages`. diff --git a/python_ta/__init__.py b/python_ta/__init__.py index dcd53b1bc..444812034 100644 --- a/python_ta/__init__.py +++ b/python_ta/__init__.py @@ -138,8 +138,6 @@ def _check( for file_py in get_file_paths(locations): if not _verify_pre_check(file_py): continue # Check the other files - # The current file can actually be checked so update the flag - is_any_file_checked = True # Load config file in user location. Construct new linter each # time, so config options don't bleed to unintended files. # Reuse the same reporter each time to accumulate the results across different files. @@ -148,7 +146,22 @@ def _check( file_linted=file_py, load_default_config=load_default_config, ) - linter.set_reporter(current_reporter) + + if not is_any_file_checked: + prev_output = current_reporter.out + current_reporter = linter.reporter + current_reporter.out = prev_output + + # At this point, the only possible errors are those from parsing the config file + # so print them, if there are any. + if current_reporter.messages: + current_reporter.print_messages() + else: + linter.set_reporter(current_reporter) + + # The current file was checked so update the flag + is_any_file_checked = True + module_name = os.path.splitext(os.path.basename(file_py))[0] if module_name in MANAGER.astroid_cache: # Remove module from astroid cache del MANAGER.astroid_cache[module_name] @@ -221,6 +234,8 @@ def _override_config(linter: PyLinter, config_location: AnyStr) -> None: Snippets taken from pylint.config.config_initialization. """ + linter.set_current_module(config_location) + # Read the configuration file. config_file_parser = _ConfigurationFileParser(verbose=True, linter=linter) try: @@ -235,6 +250,9 @@ def _override_config(linter: PyLinter, config_location: AnyStr) -> None: except _UnrecognizedOptionError as exc: print(f"Unrecognized options: {', '.join(exc.options)}", file=sys.stderr) + # Everything has been set up already so emit any stashed messages. + linter._emit_stashed_messages() + linter.config_file = config_location diff --git a/python_ta/reporters/core.py b/python_ta/reporters/core.py index 4c94c6073..a5a5c1887 100644 --- a/python_ta/reporters/core.py +++ b/python_ta/reporters/core.py @@ -3,6 +3,7 @@ import os.path import sys from collections import defaultdict +from pathlib import Path from typing import Dict, List, Optional, Tuple from astroid import NodeNG @@ -40,7 +41,10 @@ def to_dict(self) -> Dict: # Messages without a source code line to highlight -NO_SNIPPET = {"invalid-name"} +NO_SNIPPET = { + "invalid-name", + "unknown-option-value", +} class PythonTaReporter(BaseReporter): @@ -215,6 +219,13 @@ def _colourify(cls, colour_class: str, text: str) -> str: # Event callbacks def on_set_current_module(self, module: str, filepath: Optional[str]) -> None: """Hook called when a module starts to be analysed.""" + # First, check if `module` is the name of a config file and if so, make filepath the + # corresponding path to that config file. + possible_config_path = Path(os.path.expandvars(module)).expanduser() + + if possible_config_path.exists() and filepath is None: + filepath = str(possible_config_path) + # Skip if filepath is None if filepath is None: return diff --git a/tests/test_config/file_fixtures/test_json_with_errors.pylintrc b/tests/test_config/file_fixtures/test_json_with_errors.pylintrc new file mode 100644 index 000000000..7341df262 --- /dev/null +++ b/tests/test_config/file_fixtures/test_json_with_errors.pylintrc @@ -0,0 +1,16 @@ +[REPORTS] + +output-format = python_ta.reporters.JSONReporter + +[FORMAT] + +max-line-length = 100 + +ignore-long-lines = ^\s*((# )??)|(>>>.*)$ + +[FORBIDDEN IMPORT] +extra-imports = math, tkinter + +[MESSAGES CONTROL] +disable = + ooga diff --git a/tests/test_config/file_fixtures/test_plain_with_errors.pylintrc b/tests/test_config/file_fixtures/test_plain_with_errors.pylintrc new file mode 100644 index 000000000..a0c914b18 --- /dev/null +++ b/tests/test_config/file_fixtures/test_plain_with_errors.pylintrc @@ -0,0 +1,16 @@ +[REPORTS] + +output-format = python_ta.reporters.PlainReporter + +[FORMAT] + +max-line-length = 100 + +ignore-long-lines = ^\s*((# )??)|(>>>.*)$ + +[FORBIDDEN IMPORT] +extra-imports = math, tkinter + +[MESSAGES CONTROL] +disable = + ooga diff --git a/tests/test_config/file_fixtures/test_with_errors.pylintrc b/tests/test_config/file_fixtures/test_with_errors.pylintrc new file mode 100644 index 000000000..6e9a6fd54 --- /dev/null +++ b/tests/test_config/file_fixtures/test_with_errors.pylintrc @@ -0,0 +1,16 @@ +[REPORTS] + +output-format = python_ta.reporters.HTMLReporter + +[FORMAT] + +max-line-length = 100 + +ignore-long-lines = ^\s*((# )??)|(>>>.*)$ + +[FORBIDDEN IMPORT] +extra-imports = math, tkinter + +[MESSAGES CONTROL] +disable = + ooga diff --git a/tests/test_config/test_config.py b/tests/test_config/test_config.py index 021ca0619..51b4e81a2 100644 --- a/tests/test_config/test_config.py +++ b/tests/test_config/test_config.py @@ -1,6 +1,7 @@ """ Test suite for checking whether configuration worked correctly with user-inputted configurations. """ +import json import os import pytest @@ -121,3 +122,59 @@ def test_default_pylint_checks_in_no_default(configure_linter_no_default) -> Non assert all( check not in disabled_checks for check in previously_disabled_checks if disabled_checks ) + + +def test_unknown_option_value() -> None: + """Test that the configuration options gets overridden without error when there is an unknown + option value.""" + curr_dir = os.path.dirname(__file__) + config = os.path.join(curr_dir, "file_fixtures", "test_with_errors.pylintrc") + reporter = python_ta.reset_linter(config=config).reporter + + # Check if there are any messages with the msg_id `W0012` (the code corresponding to the error + # `unknown-option-value`. + message_ids = [msg.msg_id for message_lis in reporter.messages.values() for msg in message_lis] + + assert "W0012" in message_ids + + +def test_unknown_option_value_no_default() -> None: + """Test that the configuration options gets loaded without error when there is an unknown option + value. + + The default options are not loaded from the PythonTA default config.""" + curr_dir = os.path.dirname(__file__) + config = os.path.join(curr_dir, "file_fixtures", "test_with_errors.pylintrc") + reporter = python_ta.reset_linter(config=config, load_default_config=False).reporter + + # Check if there are any messages with the msg_id `W0012` (the code corresponding to the error + # `unknown-option-value`. + message_ids = [msg.msg_id for message_lis in reporter.messages.values() for msg in message_lis] + + assert "W0012" in message_ids + + +def test_json_config_parsing_error(capsys) -> None: + """Test that the JSONReporter correctly includes the config parsing error in its report.""" + curr_dir = os.path.dirname(__file__) + config = os.path.join(curr_dir, "file_fixtures", "test_json_with_errors.pylintrc") + python_ta.check_all(module_name="examples/nodes/name.py", config=config) + out = capsys.readouterr().out + + reports = json.loads(out) + + assert any(msg["msg_id"] == "W0012" for report in reports for msg in report["msgs"]) + + +def test_print_messages_config_parsing_error(capsys) -> None: + """Test that print_messages correctly prints the config parsing error in its report. + + This tests the functionality of PlainReporter and ColorReporter to verify it correctly includes + the config parsing error in its printed report.""" + curr_dir = os.path.dirname(__file__) + config = os.path.join(curr_dir, "file_fixtures", "test_plain_with_errors.pylintrc") + python_ta.check_all(module_name="examples/nodes/name.py", config=config) + + out = capsys.readouterr().out + + assert "W0012" in out