Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support recursive wildcards in module ignores #174

Merged
merged 4 commits into from
Jun 7, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions AUTHORS.rst
Original file line number Diff line number Diff line change
Expand Up @@ -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
5 changes: 5 additions & 0 deletions CHANGELOG.rst
Original file line number Diff line number Diff line change
@@ -1,6 +1,11 @@
Changelog
=========

1.10.0 (Unreleased)
-------------------

* Recursive wildcard support for ignored imports.

1.9.0 (2023-05-13)
------------------

Expand Down
8 changes: 7 additions & 1 deletion docs/contract_types.rst
Original file line number Diff line number Diff line change
Expand Up @@ -236,14 +236,20 @@ 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:

- ``mypackage.*``: matches ``mypackage.foo`` but not ``mypackage.foo.bar``.
- ``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``.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be mypackage.foo.bar.baz?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, no, I guess it was highlighting that it'd give the same as mypackage.*.*, but I guess also anything below that too. A bit of a tricky one to express sanely.

- ``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:
Expand Down
2 changes: 1 addition & 1 deletion src/importlinter/contracts/forbidden.py
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ def check(self, graph: ImportGraph, verbose: bool) -> ContractCheck:
)
if chains:
is_kept = False
for chain in chains:
for chain in sorted(chains):
chain_data = []
for importer, imported in [
(chain[i], chain[i + 1]) for i in range(len(chain) - 1)
Expand Down
12 changes: 11 additions & 1 deletion src/importlinter/domain/fields.py
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand All @@ -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):
Expand Down
2 changes: 2 additions & 0 deletions src/importlinter/domain/helpers.py
Original file line number Diff line number Diff line change
Expand Up @@ -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"$")
Expand Down
59 changes: 58 additions & 1 deletion tests/unit/contracts/test_forbidden.py
Original file line number Diff line number Diff line change
Expand Up @@ -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,),
},
],
],
},
{
Expand Down Expand Up @@ -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": [
Expand Down Expand Up @@ -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",
Expand All @@ -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",
Expand Down
3 changes: 3 additions & 0 deletions tests/unit/contracts/test_independence.py
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Expand Down
8 changes: 8 additions & 0 deletions tests/unit/contracts/test_layers.py
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Expand Down
41 changes: 34 additions & 7 deletions tests/unit/domain/test_fields.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import enum
import re
from typing import Any, Dict, List, Optional, Type, Union

import pytest
Expand Down Expand Up @@ -33,11 +34,9 @@ class BaseFieldTest:
def test_field(self, raw_data, expected_value):
field = self.field_class(**self.field_kwargs)

if isinstance(expected_value, ValidationError):
try:
if isinstance(expected_value, Exception):
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, thanks for spotting.

with pytest.raises(expected_value.__class__, match=re.escape(expected_value.message)):
field.parse(raw_data) == expected_value
except ValidationError as e:
assert e.message == expected_value.message
else:
assert field.parse(raw_data) == expected_value

Expand Down Expand Up @@ -116,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
# -------------------
(
Expand All @@ -135,12 +150,24 @@ 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."),
),
(
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👏🏻

"*.*.* -> mypackage.*.foo.*",
ImportExpression(importer="*.*.*", imported="mypackage.*.foo.*"),
"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."),
),
),
)
Expand Down
28 changes: 28 additions & 0 deletions tests/unit/domain/test_helpers.py
Original file line number Diff line number Diff line change
Expand Up @@ -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",
[
Expand Down
4 changes: 4 additions & 0 deletions tests/unit/domain/test_imports.py
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Expand Down