From 5d580f3dc45cffe5f4fda27fd1ec1d116a338600 Mon Sep 17 00:00:00 2001 From: Sorin Sbarnea Date: Thu, 1 Sep 2022 15:45:42 +0100 Subject: [PATCH] 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 | 6 ++---- 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, 28 insertions(+), 23 deletions(-) diff --git a/src/ansiblelint/errors.py b/src/ansiblelint/errors.py index ac66b250756..72c0ffcf11d 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 b02e3c32cf7..a70e05add78 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 dafc1619485..be27f5c5f9f 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 af5c3dbe8d3..692f45561d3 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 a03ac861521..b48982e3b0e 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 dbc291047bb..af5670a3b49 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 532fdd72455..27a5dea8b19 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 4072a8620c7..e76d7c51447 100644 --- a/src/ansiblelint/runner.py +++ b/src/ansiblelint/runner.py @@ -119,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(), @@ -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 291125932b8..35776c17736 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 74a06628236..8fb8977d5e9 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 112ccfdb869..e7f18b3aacf 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 17f9b9eb4f9..8937ef79288 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 a66feb17295..3dfec6d32c9 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()),