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 wildcards in module ignores #103

Merged
merged 27 commits into from
Sep 2, 2021
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
36 changes: 24 additions & 12 deletions docs/contract_types.rst
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,25 @@
Contract types
==============

.. _Shared options:

Shared options
--------------

The following options can be used in all built-in contracts.

- ``ignore_imports``: Optional list of imports, each in the form ``mypackage.foo.importer -> mypackage.bar.imported``.
Copy link
Owner

Choose a reason for hiding this comment

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

(Not related to documentation, but had to write this somewhere!) It would be great to have the following test coverage:

  • Unit tests for wildcarded ignore_imports for each contract type (see tests/unit/contracts).
  • Functional test coverage in tests/functional/test_lint_imports.py. I suggest just adding an illegal import, together with a wildcarded ignore_imports that covers it, to the contract in setup.cfg in that folder (which is used for checking contract adherence).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@seddonym I can work further on the pure functions but I guess I don't have the time for extended tests. I really see the point of them, but it consumes a lot of time. Would you maybe just add them?

These imports will be ignored: if the import would cause a contract to be broken, adding it to the list will cause the
contract be kept instead.

Wildcards are supported as well. These can stand in for a module names, but they do not extend to subpackages.
Examples:

- mypackage.*: matches mypackage.foo but not mypackage.foo.bar.
- mypackage.*.baz: matches mypackage.foo.baz but not mypackage.foo.bar.baz.



Forbidden modules
-----------------

Expand Down Expand Up @@ -62,10 +81,7 @@ Configuration options:
- ``forbidden_modules``: A list of modules that should not be imported by the source modules. These may include
root level external packages (i.e. ``django``, but not ``django.db.models``). If external packages are included,
the top level configuration must have ``internal_external_packages = True``.
- ``ignore_imports``:
A list of imports, each in the form ``mypackage.foo.importer -> mypackage.bar.imported``. These imports
will be ignored: if the import would cause a contract to be broken, adding it to the list will cause the
contract be kept instead. (Optional.)
- ``ignore_imports``: See :ref:`Shared options`
- ``allow_indirect_imports``: If ``True``, allow indirect imports to forbidden modules without interpreting them
as a reason to mark the contract broken. (Optional.)

Expand Down Expand Up @@ -96,10 +112,8 @@ They do this by checking that there are no imports in any direction between the
**Configuration options**

- ``modules``: A list of modules/subpackages that should be independent from each other.
- ``ignore_imports``:
A list of imports, each in the form ``mypackage.foo.importer -> mypackage.bar.imported``. These imports
will be ignored: if the import would cause a contract to be broken, adding it to the list will cause the
contract be kept instead. (Optional.)
- ``ignore_imports``: See :ref:`Shared options`


Layers
------
Expand Down Expand Up @@ -187,10 +201,8 @@ won't complain.
- ``containers``:
List of the parent modules of the layers, as *absolute names* that you could import, such as
``mypackage.foo``. (Optional.)
- ``ignore_imports``:
A list of imports, each in the form ``mypackage.foo.importer -> mypackage.bar.imported``. These imports
will be ignored: if the import would cause a contract to be broken, adding it to the list will cause the
contract be kept instead. (Optional.)
- ``ignore_imports``: See :ref:`Shared options`



Custom contract types
Expand Down
6 changes: 3 additions & 3 deletions src/importlinter/contracts/forbidden.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ class ForbiddenContract(Contract):
Configuration options:
- source_modules: A list of Modules that should not import the forbidden modules.
- forbidden_modules: A list of Modules that should not be imported by the source modules.
- ignore_imports: A set of DirectImports. These imports will be ignored: if the import
- ignore_imports: A set of ImportExpressions. These imports will be ignored if the import
would cause a contract to be broken, adding it to the set will cause
the contract be kept instead. (Optional.)
- allow_indirect_imports: Whether to allow indirect imports to forbidden modules.
Expand All @@ -24,14 +24,14 @@ class ForbiddenContract(Contract):

source_modules = fields.ListField(subfield=fields.ModuleField())
forbidden_modules = fields.ListField(subfield=fields.ModuleField())
ignore_imports = fields.SetField(subfield=fields.DirectImportField(), required=False)
ignore_imports = fields.SetField(subfield=fields.ImportExpressionField(), required=False)
allow_indirect_imports = fields.StringField(required=False)

def check(self, graph: ImportGraph) -> ContractCheck:
is_kept = True
invalid_chains = []

helpers.pop_imports(
helpers.pop_import_expressions(
graph, self.ignore_imports if self.ignore_imports else [] # type: ignore
)

Expand Down
6 changes: 3 additions & 3 deletions src/importlinter/contracts/independence.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,21 +16,21 @@ class IndependenceContract(Contract):
Configuration options:

- modules: A list of Modules that should be independent from each other.
- ignore_imports: A set of DirectImports. These imports will be ignored: if the import
- ignore_imports: A set of ImportExpressions. These imports will be ignored: if the import
would cause a contract to be broken, adding it to the set will cause
the contract be kept instead. (Optional.)
"""

type_name = "independence"

modules = fields.ListField(subfield=fields.ModuleField())
ignore_imports = fields.SetField(subfield=fields.DirectImportField(), required=False)
ignore_imports = fields.SetField(subfield=fields.ImportExpressionField(), required=False)

def check(self, graph: ImportGraph) -> ContractCheck:
is_kept = True
invalid_chains = []

helpers.pop_imports(
helpers.pop_import_expressions(
graph, self.ignore_imports if self.ignore_imports else [] # type: ignore
)

Expand Down
6 changes: 3 additions & 3 deletions src/importlinter/contracts/layers.py
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ class LayersContract(Contract):
- layers: An ordered list of layers. Each layer is the name of a module relative
to its parent package. The order is from higher to lower level layers.
- containers: A list of the parent Modules of the layers (optional).
- ignore_imports: A set of DirectImports. These imports will be ignored: if the import
- ignore_imports: A set of ImportExpressions. These imports will be ignored: if the import
would cause a contract to be broken, adding it to the set will cause
the contract be kept instead. (Optional.)
"""
Expand All @@ -51,14 +51,14 @@ class LayersContract(Contract):

layers = fields.ListField(subfield=LayerField())
containers = fields.ListField(subfield=fields.StringField(), required=False)
ignore_imports = fields.SetField(subfield=fields.DirectImportField(), required=False)
ignore_imports = fields.SetField(subfield=fields.ImportExpressionField(), required=False)

def check(self, graph: ImportGraph) -> ContractCheck:
is_kept = True
invalid_chains = []

direct_imports_to_ignore = self.ignore_imports if self.ignore_imports else []
helpers.pop_imports(graph, direct_imports_to_ignore) # type: ignore
helpers.pop_import_expressions(graph, direct_imports_to_ignore) # type: ignore

if self.containers:
self._validate_containers(graph)
Expand Down
26 changes: 20 additions & 6 deletions src/importlinter/domain/fields.py
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import abc
from typing import Generic, Iterable, List, Set, TypeVar, Union

from importlinter.domain.imports import DirectImport, Module
from importlinter.domain.imports import Module, ImportExpression

FieldValue = TypeVar("FieldValue")

Expand Down Expand Up @@ -109,16 +109,30 @@ def parse(self, raw_data: Union[str, List]) -> Module:
return Module(StringField().parse(raw_data))


class DirectImportField(Field):
class ImportExpressionField(Field):
"""
A field for DirectImports.
A field for ImportExpressions.

Expects raw data in the form: "mypackage.foo.importer -> mypackage.bar.imported".
kasium marked this conversation as resolved.
Show resolved Hide resolved
Expects raw data in the form:
"mypackage.foo.importer -> mypackage.bar.imported".

In addition, it handles wildcards:
"mypackage.*.importer -> mypackage.bar.*"
"""

def parse(self, raw_data: Union[str, List]) -> DirectImport:
def parse(self, raw_data: Union[str, List]) -> ImportExpression:
string = StringField().parse(raw_data)
importer, _, imported = string.partition(" -> ")

if not (importer and imported):
raise ValidationError('Must be in the form "package.importer -> package.imported".')
return DirectImport(importer=Module(importer), imported=Module(imported))

self._validate_wildcard(importer)
self._validate_wildcard(imported)

return ImportExpression(importer=importer, imported=imported)
kasium marked this conversation as resolved.
Show resolved Hide resolved

def _validate_wildcard(self, expression: str) -> None:
for part in expression.split("."):
if len(part) > 1 and "*" in part:
raise ValidationError("A wildcard can only replace a whole module")
70 changes: 66 additions & 4 deletions src/importlinter/domain/helpers.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
from typing import Dict, Iterable, List, Union
import itertools
from typing import Dict, Iterable, List, Tuple, Union, Pattern
import re

from importlinter.domain.imports import DirectImport
from importlinter.domain.imports import ImportExpression, Module, DirectImport
from importlinter.domain.ports.graph import ImportGraph


Expand All @@ -13,10 +15,8 @@ def pop_imports(
) -> List[Dict[str, Union[str, int]]]:
"""
Removes the supplied direct imports from the graph.

Returns:
The list of import details that were removed, including any additional metadata.

Raises:
MissingImport if the import is not present in the graph.
"""
Expand All @@ -34,6 +34,33 @@ def pop_imports(
return removed_imports


def import_expressions_to_imports(
graph: ImportGraph, expressions: Iterable[ImportExpression]
) -> List[DirectImport]:
imports: List[DirectImport] = []
for expression in expressions:
Copy link
Owner

Choose a reason for hiding this comment

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

Nice. I think it would be worth extracting lines 41 - 53 into a separate import_expressions_to_imports function, which would be a side-effect free function so easier to test.

It would be great to have unit test coverage too. If I were you I would focus the unit testing on the pure function import_expressions_to_imports - possibly don't need one for pop_import_expressions if we have coverage on the contract types instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added some basic tests

matched = False
for (importer, imported) in _expression_to_modules(expression, graph):
import_details = graph.get_import_details(
importer=importer.name, imported=imported.name
)
if import_details:
imports.append(DirectImport(importer=importer, imported=imported))
matched = True
if not matched:
raise MissingImport(
f"Ignored import expression {expression} didn't match anything in the graph."
)
return imports


def pop_import_expressions(
graph: ImportGraph, expressions: Iterable[ImportExpression]
) -> List[Dict[str, Union[str, int]]]:
imports = import_expressions_to_imports(graph, expressions)
return pop_imports(graph, imports)


def add_imports(graph: ImportGraph, import_details: List[Dict[str, Union[str, int]]]) -> None:
"""
Adds the supplied import details to the graph.
Expand All @@ -55,3 +82,38 @@ def add_imports(graph: ImportGraph, import_details: List[Dict[str, Union[str, in
line_number=details["line_number"],
line_contents=details["line_contents"],
)


def _to_pattern(expression: str) -> Pattern:
kasium marked this conversation as resolved.
Show resolved Hide resolved
"""
Function which translates an import expression into a regex pattern.
"""
pattern_parts = []
for part in expression.split("."):
if "*" == part:
pattern_parts.append(part.replace("*", r"[^\.]+"))
else:
pattern_parts.append(part)
return re.compile(r"\.".join(pattern_parts))


def _expression_to_modules(
expression: ImportExpression, graph: ImportGraph
) -> Iterable[Tuple[Module, Module]]:
if not expression.has_wildcard_expression():
return [(Module(expression.importer), Module(expression.imported))]

importer = []
imported = []

importer_pattern = _to_pattern(expression.importer)
imported_expression = _to_pattern(expression.imported)

for module in graph.modules:
kasium marked this conversation as resolved.
Show resolved Hide resolved

if importer_pattern.match(module):
importer.append(Module(module))
if imported_expression.match(module):
imported.append(Module(module))

return itertools.product(importer, imported)
26 changes: 26 additions & 0 deletions src/importlinter/domain/imports.py
Original file line number Diff line number Diff line change
Expand Up @@ -86,3 +86,29 @@ def __str__(self) -> str:

def __hash__(self) -> int:
return hash((str(self), self.line_contents))


class ImportExpression(ValueObject):
"""
A user-submitted expression describing an import or set of imports.

Sets of imports are notated using * wildcards.
kasium marked this conversation as resolved.
Show resolved Hide resolved
These wildcards can stand in for a module name or part of a name, but they do
not extend to subpackages.

For example, "mypackage.*" refers to every child subpackage of mypackage.
It does not, however, include more distant descendants such as mypackage.foo.bar.
"""

def __init__(self, importer: str, imported: str) -> None:
self.importer = importer
self.imported = imported
Copy link
Owner

Choose a reason for hiding this comment

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

Perhaps we should raise a ValueError if we get a malformed import expression, what do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess, then it make sense to extract the import expression validationto a util function or NOT to validate in ImportExpressionField


def has_wildcard_expression(self) -> bool:
return "*" in self.imported or "*" in self.importer

def __str__(self) -> str:
return "{} -> {}".format(self.importer, self.imported)

def __hash__(self) -> int:
return hash((self.importer, self.imported))
2 changes: 1 addition & 1 deletion tests/helpers/contracts.py
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ def render_broken_contract(self, check: "ContractCheck") -> None:
class FieldsContract(Contract):
single_field = fields.StringField()
multiple_field = fields.ListField(subfield=fields.StringField())
import_field = fields.DirectImportField()
import_field = fields.ImportExpressionField()
required_field = fields.StringField() # Fields are required by default.

def check(self, graph: ImportGraph) -> ContractCheck:
Expand Down
32 changes: 26 additions & 6 deletions tests/unit/domain/test_fields.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,15 +3,15 @@
import pytest

from importlinter.domain.fields import (
DirectImportField,
ImportExpressionField,
Field,
ListField,
ModuleField,
SetField,
StringField,
ValidationError,
)
from importlinter.domain.imports import DirectImport, Module
from importlinter.domain.imports import ImportExpression, Module


class BaseFieldTest:
Expand Down Expand Up @@ -64,7 +64,19 @@ class TestModuleField(BaseFieldTest):
(
(
"mypackage.foo -> mypackage.bar",
DirectImport(importer=Module("mypackage.foo"), imported=Module("mypackage.bar")),
ImportExpression(importer="mypackage.foo", imported="mypackage.bar"),
),
(
"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.*"),
),
(
["one", "two", "three"],
Expand All @@ -76,12 +88,20 @@ class TestModuleField(BaseFieldTest):
),
(
"my-package.foo -> my-package.bar",
DirectImport(importer=Module("my-package.foo"), imported=Module("my-package.bar")),
ImportExpression(importer="my-package.foo", imported="my-package.bar"),
),
(
"mypackage.foo.bar* -> mypackage.bar",
ValidationError("A wildcard can only replace a whole module"),
),
(
"mypackage.foo.b*z -> mypackage.bar",
ValidationError("A wildcard can only replace a whole module"),
),
),
)
class TestDirectImportField(BaseFieldTest):
field_class = DirectImportField
class TestImportExpressionField(BaseFieldTest):
kasium marked this conversation as resolved.
Show resolved Hide resolved
field_class = ImportExpressionField


@pytest.mark.parametrize(
Expand Down
Loading