From 203f7df08e1e03f5c0591ca87bb8bcb9fb416ad5 Mon Sep 17 00:00:00 2001 From: Sorin Sbarnea Date: Thu, 1 Sep 2022 16:14:06 +0100 Subject: [PATCH] refactored runner.is_exclude and MatchError to use Lintable (#2379) * refactored runner.is_exclude use Lintable * refactoring: make MatchError use only lintable This refactoring will help us simplify the codebase and enable us to make use of some extra data that is specific to each file, like implementing per file (lintable) skip lists. --- src/ansiblelint/errors.py | 2 +- src/ansiblelint/rules/__init__.py | 4 ++-- src/ansiblelint/rules/partial_become.py | 2 +- src/ansiblelint/rules/playbook_extension.py | 2 +- src/ansiblelint/rules/role_name.py | 2 +- src/ansiblelint/rules/syntax_check.py | 6 ++--- src/ansiblelint/rules/yaml_rule.py | 2 +- src/ansiblelint/runner.py | 26 ++++++++++----------- src/ansiblelint/utils.py | 2 +- test/test_formatter.py | 7 +++--- test/test_formatter_json.py | 5 ++-- test/test_formatter_sarif.py | 5 ++-- test/test_matcherrror.py | 6 ++++- 13 files changed, 38 insertions(+), 33 deletions(-) diff --git a/src/ansiblelint/errors.py b/src/ansiblelint/errors.py index ac66b25075..72c0ffcf11 100644 --- a/src/ansiblelint/errors.py +++ b/src/ansiblelint/errors.py @@ -34,7 +34,7 @@ def __init__( linenumber: int = 1, column: int | None = None, details: str = "", - filename: str | Lintable | None = None, + filename: Lintable | None = None, rule: BaseRule = RuntimeErrorRule(), tag: str | None = None, # optional fine-graded tag ) -> None: diff --git a/src/ansiblelint/rules/__init__.py b/src/ansiblelint/rules/__init__.py index b02e3c32cf..a70e05add7 100644 --- a/src/ansiblelint/rules/__init__.py +++ b/src/ansiblelint/rules/__init__.py @@ -86,7 +86,7 @@ def create_matcherror( message: str | None = None, linenumber: int = 1, details: str = "", - filename: str | Lintable | None = None, + filename: Lintable | None = None, tag: str = "", ) -> MatchError: """Instantiate a new MatchError.""" @@ -207,7 +207,7 @@ def matchyaml(self, file: Lintable) -> list[MatchError]: if isinstance(yaml, str): if yaml.startswith("$ANSIBLE_VAULT"): return [] - return [MatchError(filename=str(file.path), rule=LoadingFailureRule())] + return [MatchError(filename=file, rule=LoadingFailureRule())] if not yaml: return matches diff --git a/src/ansiblelint/rules/partial_become.py b/src/ansiblelint/rules/partial_become.py index dafc161948..be27f5c5f9 100644 --- a/src/ansiblelint/rules/partial_become.py +++ b/src/ansiblelint/rules/partial_become.py @@ -104,7 +104,7 @@ def matchplay(self, file: Lintable, data: odict[str, Any]) -> list[MatchError]: return [ self.create_matcherror( message=self.shortdesc, - filename=str(file.path), + filename=file, linenumber=data[LINE_NUMBER_KEY], ) ] diff --git a/src/ansiblelint/rules/playbook_extension.py b/src/ansiblelint/rules/playbook_extension.py index af5c3dbe8d..692f45561d 100644 --- a/src/ansiblelint/rules/playbook_extension.py +++ b/src/ansiblelint/rules/playbook_extension.py @@ -28,5 +28,5 @@ def matchyaml(self, file: Lintable) -> list[MatchError]: ext = os.path.splitext(path) if ext[1] not in [".yml", ".yaml"] and path not in self.done: self.done.append(path) - result.append(self.create_matcherror(filename=path)) + result.append(self.create_matcherror(filename=file)) return result diff --git a/src/ansiblelint/rules/role_name.py b/src/ansiblelint/rules/role_name.py index a03ac86152..b48982e3b0 100644 --- a/src/ansiblelint/rules/role_name.py +++ b/src/ansiblelint/rules/role_name.py @@ -82,7 +82,7 @@ def matchyaml(self, file: Lintable) -> list[MatchError]: if role_name and not self._re.match(role_name): result.append( self.create_matcherror( - filename=str(file.path), + filename=file, message=self.shortdesc.format(role_name), ) ) diff --git a/src/ansiblelint/rules/syntax_check.py b/src/ansiblelint/rules/syntax_check.py index dbc291047b..af5670a3b4 100644 --- a/src/ansiblelint/rules/syntax_check.py +++ b/src/ansiblelint/rules/syntax_check.py @@ -75,7 +75,7 @@ def _get_ansible_syntax_check_matches(lintable: Lintable) -> list[MatchError]: result = [] if run.returncode != 0: message = None - filename = str(lintable.path) + filename = lintable linenumber = 1 column = None tag = None @@ -93,12 +93,12 @@ def _get_ansible_syntax_check_matches(lintable: Lintable) -> list[MatchError]: if match: message = match.groupdict()["title"] # Ansible returns absolute paths - filename = match.groupdict()["filename"] + filename = Lintable(match.groupdict()["filename"]) linenumber = int(match.groupdict()["line"]) column = int(match.groupdict()["column"]) elif _empty_playbook_re.search(stderr): message = "Empty playbook, nothing to do" - filename = str(lintable.path) + filename = lintable tag = "empty-playbook" if run.returncode == 4: diff --git a/src/ansiblelint/rules/yaml_rule.py b/src/ansiblelint/rules/yaml_rule.py index 532fdd7245..27a5dea8b1 100644 --- a/src/ansiblelint/rules/yaml_rule.py +++ b/src/ansiblelint/rules/yaml_rule.py @@ -51,7 +51,7 @@ def matchyaml(self, file: Lintable) -> list[MatchError]: message=problem.desc, linenumber=problem.line, details="", - filename=str(file.path), + filename=file, tag=f"yaml[{problem.rule}]", ) ) diff --git a/src/ansiblelint/runner.py b/src/ansiblelint/runner.py index edc64c77a2..e76d7c5144 100644 --- a/src/ansiblelint/runner.py +++ b/src/ansiblelint/runner.py @@ -7,7 +7,6 @@ import os from dataclasses import dataclass from fnmatch import fnmatch -from pathlib import Path from typing import TYPE_CHECKING, Any, Generator import ansiblelint.skip_utils @@ -84,7 +83,7 @@ def _update_exclude_paths(self, exclude_paths: list[str]) -> None: else: self.exclude_paths = [] - def is_excluded(self, file_path: str) -> bool: + def is_excluded(self, lintable: Lintable) -> bool: """Verify if a file path should be excluded.""" # Any will short-circuit as soon as something returns True, but will # be poor performance for the case where the path under question is @@ -92,17 +91,16 @@ def is_excluded(self, file_path: str) -> bool: # Exclusions should be evaluated only using absolute paths in order # to work correctly. - if not file_path: + if not lintable: return False - abs_path = os.path.abspath(file_path) - _file_path = Path(file_path) + abs_path = str(lintable.abspath) return any( abs_path.startswith(path) - or _file_path.match(path) + or lintable.path.match(path) or fnmatch(str(abs_path), path) - or fnmatch(str(_file_path), path) + or fnmatch(str(lintable), path) for path in self.exclude_paths ) @@ -113,7 +111,7 @@ def run(self) -> list[MatchError]: # noqa: C901 # remove exclusions for lintable in self.lintables.copy(): - if self.is_excluded(str(lintable.path.resolve())): + if self.is_excluded(lintable): _logger.debug("Excluded %s", lintable) self.lintables.remove(lintable) try: @@ -121,7 +119,7 @@ def run(self) -> list[MatchError]: # noqa: C901 except RuntimeError as exc: matches.append( MatchError( - filename=str(lintable.path), + filename=lintable, message=str(exc), details=str(exc.__cause__), rule=LoadingFailureRule(), @@ -180,7 +178,9 @@ def worker(lintable: Lintable) -> list[MatchError]: # remove any matches made inside excluded files matches = list( - filter(lambda match: not self.is_excluded(match.filename), matches) + filter( + lambda match: not self.is_excluded(Lintable(match.filename)), matches + ) ) return sorted(set(matches)) @@ -191,7 +191,7 @@ def _emit_matches(self, files: list[Lintable]) -> Generator[MatchError, None, No for lintable in self.lintables - visited: try: for child in ansiblelint.utils.find_children(lintable): - if self.is_excluded(str(child.path)): + if self.is_excluded(child): continue self.lintables.add(child) files.append(child) @@ -201,9 +201,7 @@ def _emit_matches(self, files: list[Lintable]) -> Generator[MatchError, None, No exc.rule = LoadingFailureRule() yield exc except AttributeError: - yield MatchError( - filename=str(lintable.path), rule=LoadingFailureRule() - ) + yield MatchError(filename=lintable, rule=LoadingFailureRule()) visited.add(lintable) diff --git a/src/ansiblelint/utils.py b/src/ansiblelint/utils.py index 291125932b..35776c1773 100644 --- a/src/ansiblelint/utils.py +++ b/src/ansiblelint/utils.py @@ -185,7 +185,7 @@ def find_children(lintable: Lintable) -> list[Lintable]: # noqa: C901 basedir = os.path.dirname(str(lintable.path)) # playbook_ds can be an AnsibleUnicode string, which we consider invalid if isinstance(playbook_ds, str): - raise MatchError(filename=str(lintable.path), rule=LoadingFailureRule()) + raise MatchError(filename=lintable, rule=LoadingFailureRule()) for item in _playbook_items(playbook_ds): # if lintable.kind not in ["playbook"]: # continue diff --git a/test/test_formatter.py b/test/test_formatter.py index 74a0662823..8fb8977d5e 100644 --- a/test/test_formatter.py +++ b/test/test_formatter.py @@ -21,6 +21,7 @@ import pathlib from ansiblelint.errors import MatchError +from ansiblelint.file_utils import Lintable from ansiblelint.formatters import Formatter from ansiblelint.rules import AnsibleLintRule @@ -35,7 +36,7 @@ def test_format_coloured_string() -> None: message="message", linenumber=1, details="hello", - filename="filename.yml", + filename=Lintable("filename.yml"), rule=rule, ) formatter.format(match) @@ -47,7 +48,7 @@ def test_unicode_format_string() -> None: message="\U0001f427", linenumber=1, details="hello", - filename="filename.yml", + filename=Lintable("filename.yml"), rule=rule, ) formatter.format(match) @@ -59,7 +60,7 @@ def test_dict_format_line() -> None: message="xyz", linenumber=1, details={"hello": "world"}, # type: ignore - filename="filename.yml", + filename=Lintable("filename.yml"), rule=rule, ) formatter.format(match) diff --git a/test/test_formatter_json.py b/test/test_formatter_json.py index 112ccfdb86..e7f18b3aac 100644 --- a/test/test_formatter_json.py +++ b/test/test_formatter_json.py @@ -9,6 +9,7 @@ import pytest from ansiblelint.errors import MatchError +from ansiblelint.file_utils import Lintable from ansiblelint.formatters import CodeclimateJSONFormatter from ansiblelint.rules import AnsibleLintRule @@ -31,7 +32,7 @@ def setup_class(self) -> None: message="message", linenumber=1, details="hello", - filename="filename.yml", + filename=Lintable("filename.yml"), rule=self.rule, ) ) @@ -40,7 +41,7 @@ def setup_class(self) -> None: message="message", linenumber=2, details="hello", - filename="filename.yml", + filename=Lintable("filename.yml"), rule=self.rule, ) ) diff --git a/test/test_formatter_sarif.py b/test/test_formatter_sarif.py index 17f9b9eb4f..8937ef7928 100644 --- a/test/test_formatter_sarif.py +++ b/test/test_formatter_sarif.py @@ -9,6 +9,7 @@ import pytest from ansiblelint.errors import MatchError +from ansiblelint.file_utils import Lintable from ansiblelint.formatters import SarifFormatter from ansiblelint.rules import AnsibleLintRule @@ -35,7 +36,7 @@ def setup_class(self) -> None: linenumber=1, column=10, details="hello", - filename="filename.yml", + filename=Lintable("filename.yml"), rule=self.rule, ) ) @@ -44,7 +45,7 @@ def setup_class(self) -> None: message="message", linenumber=2, details="hello", - filename="filename.yml", + filename=Lintable("filename.yml"), rule=self.rule, ) ) diff --git a/test/test_matcherrror.py b/test/test_matcherrror.py index a66feb1729..3dfec6d32c 100644 --- a/test/test_matcherrror.py +++ b/test/test_matcherrror.py @@ -6,6 +6,7 @@ import pytest from ansiblelint.errors import MatchError +from ansiblelint.file_utils import Lintable from ansiblelint.rules.no_changed_when import CommandHasChangesCheckRule from ansiblelint.rules.partial_become import BecomeUserWithoutBecomeRule @@ -75,7 +76,10 @@ def test_matcherror_invalid() -> None: # sorting by message (MatchError("z"), MatchError("a")), # filenames takes priority in sorting - (MatchError("a", filename="b"), MatchError("a", filename="a")), + ( + MatchError("a", filename=Lintable("b")), + MatchError("a", filename=Lintable("a")), + ), # rule id partial-become > rule id no-changed-when ( MatchError(rule=BecomeUserWithoutBecomeRule()),