From 806a5987a1b856a04cd0b4e425a5731fc7be2249 Mon Sep 17 00:00:00 2001 From: Nick Pope Date: Sat, 3 Jun 2023 20:07:51 +0100 Subject: [PATCH] Support recursive wildcards in module ignores --- AUTHORS.rst | 1 + CHANGELOG.rst | 5 ++ docs/contract_types.rst | 8 ++- src/importlinter/domain/fields.py | 12 ++++- src/importlinter/domain/helpers.py | 2 + tests/unit/contracts/test_forbidden.py | 59 ++++++++++++++++++++++- tests/unit/contracts/test_independence.py | 3 ++ tests/unit/contracts/test_layers.py | 8 +++ tests/unit/domain/test_fields.py | 34 ++++++++++++- tests/unit/domain/test_helpers.py | 28 +++++++++++ tests/unit/domain/test_imports.py | 4 ++ 11 files changed, 160 insertions(+), 4 deletions(-) diff --git a/AUTHORS.rst b/AUTHORS.rst index 599ea833..ecacae4b 100644 --- a/AUTHORS.rst +++ b/AUTHORS.rst @@ -18,3 +18,4 @@ Contributors * Petter Friberg - https://github.com/flaeppe * James Owen - https://github.com/leamingrad * Matthew Gamble - https://github.com/mwgamble +* Nick Pope - https://github.com/ngnpope diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 22d2a68f..e358235a 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -1,6 +1,11 @@ Changelog ========= +1.10.0 (Unreleased) +------------------- + +* Recursive wildcard support for ignored imports. + 1.9.0 (2023-05-13) ------------------ diff --git a/docs/contract_types.rst b/docs/contract_types.rst index 678977db..be3cd425 100644 --- a/docs/contract_types.rst +++ b/docs/contract_types.rst @@ -236,7 +236,7 @@ Options used by multiple contracts contract be kept instead. Wildcards (in the form of ``*``) are supported. These can stand in for a module names, but they do not extend to - subpackages. + subpackages. A recursive wildcard (in the form of ``**``) is also supported. Examples: @@ -244,6 +244,12 @@ Options used by multiple contracts - ``mypackage.*.baz``: matches ``mypackage.foo.baz`` but not ``mypackage.foo.bar.baz``. - ``mypackage.*.*``: matches ``mypackage.foo.bar`` and ``mypackage.foobar.baz``. - ``mypackage.foo*``: not a valid expression. (The wildcard must replace a whole module name.) + - ``mypackage.**``: matches ``mypackage.foo.bar`` and ``mypackage.foobar.baz``. + - ``mypackage.**.qux``: matches ``mypackage.foo.bar.qux`` and ``mypackage.foobar.baz.qux``. + - ``mypackage.foo**``: not a valid expression. (The wildcard must replace a whole module name.) + - ``mypackage.**.*.qux``: not a valid expression. (A recursive wildcard cannot be followed by a wildcard.) + - ``mypackage.**.**.qux``: not a valid expression. (A recursive wildcard cannot be followed by a wildcard.) + - ``mypackage.*.**.qux``: not a valid expression. (A wildcard cannot be followed by a recursive wildcard.) - ``unmatched_ignore_imports_alerting``: The alerting level for handling expressions supplied in ``ignore_imports`` that do not match any imports in the graph. Choices are: diff --git a/src/importlinter/domain/fields.py b/src/importlinter/domain/fields.py index a5f25c86..4f7d7f34 100644 --- a/src/importlinter/domain/fields.py +++ b/src/importlinter/domain/fields.py @@ -165,6 +165,7 @@ class ImportExpressionField(Field): In addition, it handles wildcards: "mypackage.*.importer -> mypackage.bar.*" + "mypackage.**.importer -> mypackage.bar.**" """ def parse(self, raw_data: Union[str, List]) -> ImportExpression: @@ -180,9 +181,18 @@ def parse(self, raw_data: Union[str, List]) -> ImportExpression: return ImportExpression(importer=importer, imported=imported) def _validate_wildcard(self, expression: str) -> None: + last_wildcard = None for part in expression.split("."): - if len(part) > 1 and "*" in part: + if "**" == last_wildcard and ("*" == part or "**" == part): + raise ValidationError("A recursive wildcard cannot be followed by a wildcard.") + if "*" == last_wildcard and "**" == part: + raise ValidationError("A wildcard cannot be followed by a recursive wildcard.") + if "*" == part or "**" == part: + last_wildcard = part + continue + if "*" in part: raise ValidationError("A wildcard can only replace a whole module.") + last_wildcard = None class EnumField(Field): diff --git a/src/importlinter/domain/helpers.py b/src/importlinter/domain/helpers.py index 243de0c3..06c9169c 100644 --- a/src/importlinter/domain/helpers.py +++ b/src/importlinter/domain/helpers.py @@ -208,6 +208,8 @@ def _to_pattern(expression: str) -> Pattern: for part in expression.split("."): if "*" == part: pattern_parts.append(part.replace("*", r"[^\.]+")) + elif "**" == part: + pattern_parts.append(part.replace("*", r"[^\.]+(?:\.[^\.]+)*?")) else: pattern_parts.append(part) return re.compile(r"^" + r"\.".join(pattern_parts) + r"$") diff --git a/tests/unit/contracts/test_forbidden.py b/tests/unit/contracts/test_forbidden.py index 6d43af34..58efb419 100644 --- a/tests/unit/contracts/test_forbidden.py +++ b/tests/unit/contracts/test_forbidden.py @@ -49,7 +49,14 @@ def test_is_broken_when_forbidden_modules_imported(self): "imported": "mypackage.green.beta", "line_numbers": (3,), } - ] + ], + [ + { + "importer": "mypackage.one.alpha.circle", + "imported": "mypackage.green.beta.sphere", + "line_numbers": (8,), + }, + ], ], }, { @@ -153,6 +160,48 @@ def test_ignore_imports_with_wildcards(self): ignore_imports=("mypackage.*.alpha -> mypackage.*.beta",), ) + check = contract.check(graph=graph, verbose=False) + assert check.metadata == { + "invalid_chains": [ + { + "upstream_module": "mypackage.green", + "downstream_module": "mypackage.one", + "chains": [ + [ + { + "importer": "mypackage.one.alpha.circle", + "imported": "mypackage.green.beta.sphere", + "line_numbers": (8,), + }, + ] + ], + }, + { + "upstream_module": "mypackage.green", + "downstream_module": "mypackage.three", + "chains": [ + [ + { + "importer": "mypackage.three", + "imported": "mypackage.green", + "line_numbers": (4,), + }, + ] + ], + }, + ], + } + + def test_ignore_imports_with_recursive_wildcards(self): + graph = self._build_graph() + contract = self._build_contract( + forbidden_modules=("mypackage.green",), + ignore_imports=( + "mypackage.**.alpha -> mypackage.**.beta", + "mypackage.**.circle -> mypackage.**.sphere", + ), + ) + check = contract.check(graph=graph, verbose=False) assert check.metadata == { "invalid_chains": [ @@ -251,11 +300,13 @@ def _build_graph(self): for module in ( "one", "one.alpha", + "one.alpha.circle", "two", "three", "blue", "green", "green.beta", + "green.beta.sphere", "yellow", "purple", "utils", @@ -269,6 +320,12 @@ def _build_graph(self): line_number=3, line_contents="foo", ) + graph.add_import( + importer="mypackage.one.alpha.circle", + imported="mypackage.green.beta.sphere", + line_number=8, + line_contents="foo", + ) graph.add_import( importer="mypackage.three", imported="mypackage.green", diff --git a/tests/unit/contracts/test_independence.py b/tests/unit/contracts/test_independence.py index 55750299..772f19fe 100644 --- a/tests/unit/contracts/test_independence.py +++ b/tests/unit/contracts/test_independence.py @@ -422,8 +422,11 @@ def test_extra_firsts_and_lasts(self): (["mypackage.indirect -> mypackage.b"], True), # Wildcards (["mypackage.a -> *.irrelevant"], False), + (["mypackage.a -> **.irrelevant"], False), (["*.a -> *.indirect"], True), + (["**.a -> **.indirect"], True), (["mypackage.* -> mypackage.b"], True), + (["mypackage.** -> mypackage.b"], True), ), ) def test_ignore_imports(ignore_imports, is_kept): diff --git a/tests/unit/contracts/test_layers.py b/tests/unit/contracts/test_layers.py index ed3b4ed5..82df25f3 100644 --- a/tests/unit/contracts/test_layers.py +++ b/tests/unit/contracts/test_layers.py @@ -776,13 +776,21 @@ class TestIgnoreImports: "mypackage.low.black -> mypackage.medium.orange", # Wildcards. "*.low.black -> mypackage.medium.orange", + "**.low.black -> mypackage.medium.orange", "mypackage.*.black -> mypackage.medium.orange", + "mypackage.**.black -> mypackage.medium.orange", "mypackage.low.* -> mypackage.medium.orange", + "mypackage.low.** -> mypackage.medium.orange", "mypackage.low.black -> *.medium.orange", + "mypackage.low.black -> **.medium.orange", "mypackage.low.black -> mypackage.*.orange", + "mypackage.low.black -> mypackage.**.orange", "mypackage.low.black -> mypackage.medium.*", + "mypackage.low.black -> mypackage.medium.**", "mypackage.*.black -> mypackage.*.orange", + "mypackage.**.black -> mypackage.**.orange", "mypackage.*.* -> mypackage.*.*", + "mypackage.** -> mypackage.**", ], ) def test_one_ignored_from_each_chain_means_contract_is_kept(self, expression): diff --git a/tests/unit/domain/test_fields.py b/tests/unit/domain/test_fields.py index ac79e497..eb52970f 100644 --- a/tests/unit/domain/test_fields.py +++ b/tests/unit/domain/test_fields.py @@ -115,6 +115,22 @@ class TestModuleField(BaseFieldTest): "*.*.* -> mypackage.*.foo.*", ImportExpression(importer="*.*.*", imported="mypackage.*.foo.*"), ), + ( + "mypackage.foo.** -> mypackage.bar", + ImportExpression(importer="mypackage.foo.**", imported="mypackage.bar"), + ), + ( + "mypackage.foo.**.baz -> mypackage.bar", + ImportExpression(importer="mypackage.foo.**.baz", imported="mypackage.bar"), + ), + ( + "mypackage.foo -> mypackage.bar.**", + ImportExpression(importer="mypackage.foo", imported="mypackage.bar.**"), + ), + ( + "** -> mypackage.**.foo.*", + ImportExpression(importer="**", imported="mypackage.**.foo.*"), + ), # Invalid expressions # ------------------- ( @@ -134,7 +150,23 @@ class TestModuleField(BaseFieldTest): ValidationError("A wildcard can only replace a whole module."), ), ( - "mypackage.**.bar -> mypackage.baz", + "mypackage.foo.bar** -> mypackage.bar", + ValidationError("A wildcard can only replace a whole module."), + ), + ( + "mypackage.**.*.foo -> mypackage.bar", + ValidationError("A recursive wildcard cannot be followed by a wildcard."), + ), + ( + "mypackage.**.**.foo -> mypackage.bar", + ValidationError("A recursive wildcard cannot be followed by a wildcard."), + ), + ( + "mypackage.*.**.foo -> mypackage.bar", + ValidationError("A wildcard cannot be followed by a recursive wildcard."), + ), + ( + "mypackage.foo.b**z -> mypackage.bar", ValidationError("A wildcard can only replace a whole module."), ), ), diff --git a/tests/unit/domain/test_helpers.py b/tests/unit/domain/test_helpers.py index 40098ce5..4f571fed 100644 --- a/tests/unit/domain/test_helpers.py +++ b/tests/unit/domain/test_helpers.py @@ -368,6 +368,34 @@ class TestResolveImportExpressions: ], set(DIRECT_IMPORTS[3:5]), ), + ( + "Importer recursive wildcard", + [ + ImportExpression(importer="mypackage.**", imported="mypackage.blue"), + ], + {DIRECT_IMPORTS[1]}, + ), + ( + "Imported recursive wildcard", + [ + ImportExpression(importer="mypackage.green", imported="mypackage.**"), + ], + set(DIRECT_IMPORTS[0:2]), + ), + ( + "Importer and imported recursive wildcards", + [ + ImportExpression(importer="mypackage.**", imported="mypackage.**"), + ], + set(DIRECT_IMPORTS[0:6]), + ), + ( + "Inner recursive wildcard", + [ + ImportExpression(importer="mypackage.**.cats", imported="mypackage.**.dogs"), + ], + set(DIRECT_IMPORTS[3:5]), + ), ( "Multiple expressions, non-overlapping", [ diff --git a/tests/unit/domain/test_imports.py b/tests/unit/domain/test_imports.py index b532a831..cc718f03 100644 --- a/tests/unit/domain/test_imports.py +++ b/tests/unit/domain/test_imports.py @@ -141,6 +141,10 @@ def test_equality(self, first, second, expected): ("mypackage.foo", "mypackage.*", True), ("mypackage.*", "mypackage.*", True), ("mypackage.*.foo", "mypackage.*.bar", True), + ("mypackage.**", "mypackage.bar", True), + ("mypackage.foo", "mypackage.**", True), + ("mypackage.**", "mypackage.**", True), + ("mypackage.**.foo", "mypackage.**.bar", True), ], ) def test_has_wildcard_expression(self, importer, imported, has_wildcard_expression):