From 36e557ea09d8dc96e5a946315f941a0d36a3e34a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?St=C3=A9phane=20Brunner?= Date: Sun, 19 Jan 2025 11:00:12 +0100 Subject: [PATCH] Better noqa interpretation - To completely ignore a line the `# noqa` should be alone - To ignore a code we can use `# noqa: ` ad ruff do - To ignore a code from a certain source use `# noqa: .`, this will also create an error if the ignore is not used. --- docs/profiles.rst | 9 +- docs/suppression.rst | 9 +- prospector/formatters/pylint.py | 4 +- prospector/postfilter.py | 33 ++++++- prospector/suppression.py | 87 ++++++++++++++++--- tests/suppression/test_suppression.py | 29 ++++++- .../testdata/test_ignore_lines/test.py | 6 ++ 7 files changed, 148 insertions(+), 29 deletions(-) diff --git a/docs/profiles.rst b/docs/profiles.rst index 59273f2b..cc726c25 100644 --- a/docs/profiles.rst +++ b/docs/profiles.rst @@ -1,7 +1,7 @@ Profiles / Configuration ======================== -The behaviour of prospector can be configured by creating a profile. A profile is +The behavior of prospector can be configured by creating a profile. A profile is a YAML file containing several sections as described below. Prospector will search for a ``.prospector.yaml`` file (and `several others`_) in the path that it is checking. @@ -120,8 +120,8 @@ If creating your own profile, you can use the ``strictness`` like so:: strictness: medium -Valid values are 'verylow', 'low', 'medium' (the default), 'high' and 'veryhigh'. If you don't specify a -strictness value, then the default of 'medium' will be used. To avoid using any of Prospector's default +Valid values are ``verylow``, ``low``, ``medium`` (the default), ``high`` and ``veryhigh``. If you don't specify a +strictness value, then the default of ``medium`` will be used. To avoid using any of Prospector's default strictness profiles, set ``strictness: none``. @@ -172,7 +172,7 @@ but you can turn it on using the ``--member-warnings`` flag or in a profile:: Libraries Used and Autodetect ............................. -Prospector will adjust the behaviour of the underlying tools based on the libraries that your project +Prospector will adjust the behavior of the underlying tools based on the libraries that your project uses. If you use Django, for example, the `pylint-django `_ plugin will be loaded. This will happen automatically. @@ -276,7 +276,6 @@ This general option, provides a way to select maximum line length allowed. .. Note:: This general option overrides and takes precedence over same option in a particular tool (pycodestyle or pylint) - Individual Configuration Options -------------------------------- diff --git a/docs/suppression.rst b/docs/suppression.rst index 10e6d98e..6fdec798 100644 --- a/docs/suppression.rst +++ b/docs/suppression.rst @@ -38,5 +38,12 @@ prospector:: ---------- A comment of ``noqa`` is used by `pycodestyle` and `pyflakes` when ignoring all errors on a certain -line. If Prospector encounters a ``# noqa`` comment it will suppress any error from any tool +line. + +If Prospector encounters a ``# noqa`` comment it will suppress any error from any tool including ``pylint`` and others such as ``dodgy``. + +If Prospector encounters a ``# noqa: `` comment it will suppress the error with the given code. + +If Prospector encounters a ``# noqa: .`` comment it will suppress the error with the given code from the given source. +This will also create an `useless-suppression` warning to inform you that the suppression is not needed. diff --git a/prospector/formatters/pylint.py b/prospector/formatters/pylint.py index 456c73cc..2173b51c 100644 --- a/prospector/formatters/pylint.py +++ b/prospector/formatters/pylint.py @@ -39,9 +39,9 @@ def render_messages(self) -> list[str]: template_code = ( "(%(source)s)" if message.code is None - else "%(code)s(%(source)s)" + else "%(source)s.%(code)s" if message.location.function is None - else "[%(code)s(%(source)s), %(function)s]" + else "[%(source)s.%(code)s, %(function)s]" ) template = ( f"{template_location}: {template_code}: %(message)s" diff --git a/prospector/postfilter.py b/prospector/postfilter.py index 51d5a0cb..04831eb7 100644 --- a/prospector/postfilter.py +++ b/prospector/postfilter.py @@ -1,6 +1,6 @@ from pathlib import Path -from prospector.message import Message +from prospector.message import Location, Message from prospector.suppression import get_suppressions @@ -48,11 +48,38 @@ def filter_messages(filepaths: list[Path], messages: list[Message]) -> list[Mess if ( relative_message_path in messages_to_ignore and message.location.line in messages_to_ignore[relative_message_path] - and message.code in messages_to_ignore[relative_message_path][message.location.line] ): - continue + matched = False + for ignore in messages_to_ignore[relative_message_path][message.location.line]: + if (ignore.source is None or message.source == ignore.source) and message.code in ignore.code: + ignore.used = True + matched = True + continue + if matched: + continue # otherwise this message was not filtered filtered.append(message) + for path, lines_ignores in messages_to_ignore.items(): # noqa: test.B007 + for line, ignores in lines_ignores.items(): + for ignore in ignores: + if ignore.should_be_used and not ignore.used: + formatted_code = ignore.code if ignore.source is None else f"{ignore.source}.{ignore.code}" + filtered.append( + Message( + source="prospector", + code="useless-suppression", + location=Location( + path=path, + module=None, + function=None, + line=line, + character=ignore.character_start, + character_end=ignore.character_end, + ), + message=f"This comment didn't suppress any message of {formatted_code}.", + ) + ) + return filtered diff --git a/prospector/suppression.py b/prospector/suppression.py index b94f7754..84a177c6 100644 --- a/prospector/suppression.py +++ b/prospector/suppression.py @@ -27,15 +27,45 @@ from typing import Optional from prospector import encoding +from prospector.config import ProspectorConfig from prospector.exceptions import FatalProspectorException from prospector.message import Message _FLAKE8_IGNORE_FILE = re.compile(r"flake8[:=]\s*noqa", re.IGNORECASE) -_PEP8_IGNORE_LINE = re.compile(r"#\s+noqa", re.IGNORECASE) +_PEP8_IGNORE_LINE = re.compile(r"#\s+noqa(\s+#.*)?$", re.IGNORECASE) +_PEP8_IGNORE_LINE_CODE = re.compile(r"#\s+noqa:([^#]+)(\s+#.*)?$", re.IGNORECASE) _PYLINT_SUPPRESSED_MESSAGE = re.compile(r"^Suppressed \'([a-z0-9-]+)\' \(from line \d+\)$") -def get_noqa_suppressions(file_contents: list[str]) -> tuple[bool, set[int]]: +class Ignore: + source: Optional[str] + code: str + # The not used create an error message + should_be_used: bool + used: bool + character_start: Optional[int] + character_end: Optional[int] + + def __init__( + self, + source: Optional[str], + code: str, + character_start: Optional[int] = None, + character_end: Optional[int] = None, + should_be_used: bool = False, + ) -> None: + self.source = source + self.code = code + self.should_be_used = should_be_used + self.used = False + self.character_start = character_start + self.character_end = character_end + + def __str__(self) -> str: + return self.code if self.source is None else f"{self.source}.{self.code}" + + +def get_noqa_suppressions(file_contents: list[str]) -> tuple[bool, set[int], dict[int, set[Ignore]]]: """ Finds all pep8/flake8 suppression messages @@ -47,12 +77,30 @@ def get_noqa_suppressions(file_contents: list[str]) -> tuple[bool, set[int]]: """ ignore_whole_file = False ignore_lines = set() + messages_to_ignore: dict[int, set[Ignore]] = defaultdict(set) for line_number, line in enumerate(file_contents): if _FLAKE8_IGNORE_FILE.search(line): ignore_whole_file = True if _PEP8_IGNORE_LINE.search(line): ignore_lines.add(line_number + 1) - return ignore_whole_file, ignore_lines + else: + noqa_match = _PEP8_IGNORE_LINE_CODE.search(line) + if noqa_match: + prospector_ignore = noqa_match.group(1).strip().split(",") + prospector_ignore = [elem.strip() for elem in prospector_ignore] + for full_code in prospector_ignore: + if "." in full_code: + tool, code = full_code.split(".", 1) + to_be_used = True + else: + tool = None + code = full_code + to_be_used = False + messages_to_ignore[line_number + 1].add( + Ignore(tool, code, noqa_match.start(), noqa_match.start(2) - 1, to_be_used) + ) + + return ignore_whole_file, ignore_lines, messages_to_ignore _PYLINT_EQUIVALENTS = { @@ -88,8 +136,8 @@ def _parse_pylint_informational( def get_suppressions( - filepaths: list[Path], messages: list[Message] -) -> tuple[set[Optional[Path]], dict[Path, set[int]], dict[Optional[Path], dict[int, set[tuple[str, str]]]]]: + filepaths: list[Path], messages: list[Message], config: ProspectorConfig +) -> tuple[set[Optional[Path]], dict[Path, set[int]], dict[Optional[Path], dict[int, set[Ignore]]]]: """ Given every message which was emitted by the tools, and the list of files to inspect, create a list of files to ignore, @@ -97,9 +145,10 @@ def get_suppressions( """ paths_to_ignore: set[Optional[Path]] = set() lines_to_ignore: dict[Path, set[int]] = defaultdict(set) - messages_to_ignore: dict[Optional[Path], dict[int, set[tuple[str, str]]]] = defaultdict(lambda: defaultdict(set)) + messages_to_ignore: dict[Optional[Path], dict[int, set[Ignore]]] = defaultdict(lambda: defaultdict(set)) - # first deal with 'noqa' style messages + conf = config.profile.suppression + # First deal with 'noqa' style messages for filepath in filepaths: try: file_contents = encoding.read_py_file(filepath).split("\n") @@ -108,20 +157,30 @@ def get_suppressions( warnings.warn(f"{err.path}: {err.__cause__}", ImportWarning, stacklevel=2) continue - ignore_file, ignore_lines = get_noqa_suppressions(file_contents) + ignore_file, ignore_lines, file_messages_to_ignore = get_noqa_suppressions( + file_contents, conf.get("noqa", True), conf.get("flake8", True), conf.get("prospector", True) + ) if ignore_file: paths_to_ignore.add(filepath) lines_to_ignore[filepath] |= ignore_lines + for line, codes_ignore in file_messages_to_ignore.items(): + messages_to_ignore[filepath][line] |= codes_ignore - # now figure out which messages were suppressed by pylint + # Now figure out which messages were suppressed by pylint pylint_ignore_files, pylint_ignore_messages = _parse_pylint_informational(messages) paths_to_ignore |= pylint_ignore_files - for pylint_filepath, line in pylint_ignore_messages.items(): - for line_number, codes in line.items(): + for pylint_filepath, line_codes in pylint_ignore_messages.items(): + for line_number, codes in line_codes.items(): for code in codes: - messages_to_ignore[pylint_filepath][line_number].add(("pylint", code)) + ignore = Ignore("pylint", code) + # Never display a unused error + ignore.used = True + messages_to_ignore[pylint_filepath][line_number].add(ignore) if code in _PYLINT_EQUIVALENTS: - for equivalent in _PYLINT_EQUIVALENTS[code]: - messages_to_ignore[pylint_filepath][line_number].add(equivalent) + for ignore_source, ignore_code in _PYLINT_EQUIVALENTS[code]: + ignore = Ignore(ignore_source, ignore_code) + # Never display a unused error + ignore.used = True + messages_to_ignore[pylint_filepath][line_number].add(ignore) return paths_to_ignore, lines_to_ignore, messages_to_ignore diff --git a/tests/suppression/test_suppression.py b/tests/suppression/test_suppression.py index cc2635c6..35768b0c 100644 --- a/tests/suppression/test_suppression.py +++ b/tests/suppression/test_suppression.py @@ -14,17 +14,38 @@ def _get_file_contents(self, name): def test_ignore_file(self): file_contents = self._get_file_contents("test_ignore_file/test.py") - whole_file, _ = get_noqa_suppressions(file_contents) + whole_file, _, _ = get_noqa_suppressions(file_contents) self.assertTrue(whole_file) def test_ignore_lines(self): file_contents = self._get_file_contents("test_ignore_lines/test.py") - _, lines = get_noqa_suppressions(file_contents) - self.assertSetEqual({1, 2, 3}, lines) + _, lines, messages_to_ignore = get_noqa_suppressions(file_contents) + self.assertSetEqual({1, 2, 3, 4}, lines) + + assert set(messages_to_ignore.keys()) == {6, 7, 8, 9} + l6 = messages_to_ignore[6].pop() + assert l6.source is None + assert l6.code == "code" + l7 = messages_to_ignore[7].pop() + assert l7.source is None + assert l7.code == "code" + l8a = messages_to_ignore[8].pop() + assert l8a.source is None + assert l8a.code == "code1" + l8a = messages_to_ignore[8].pop() + assert l8a.source is None + assert l8a.code == "code2" + assert l8a.character_start == 14 + assert l8a.character_end == 32 + l9 = messages_to_ignore[9].pop() + assert l9.source == "source" + assert l9.code == "code" + assert l9.character_start == 14 + assert l9.character_end == 32 def test_ignore_enum_error(self): file_contents = self._get_file_contents("test_ignore_enum/test.py") - _, lines = get_noqa_suppressions(file_contents) + _, lines, _ = get_noqa_suppressions(file_contents) self.assertSetEqual({5}, lines) def test_filter_messages(self): diff --git a/tests/suppression/testdata/test_ignore_lines/test.py b/tests/suppression/testdata/test_ignore_lines/test.py index 024daba1..83815f52 100644 --- a/tests/suppression/testdata/test_ignore_lines/test.py +++ b/tests/suppression/testdata/test_ignore_lines/test.py @@ -1,3 +1,9 @@ import collections # NOQA import os # noqa import tempfile # noqa +import test # noqa # test +import test # noqa test # test +import test # noqa: code +import test # noqa: code # test +import test # noqa: code1,code2 # test +import test # noqa: source.code# test