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 8 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
33 changes: 21 additions & 12 deletions docs/contract_types.rst
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,22 @@
Contract types
==============

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

The following options can be used in all contracts.
kasium marked this conversation as resolved.
Show resolved Hide resolved

- ``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.
kasium marked this conversation as resolved.
Show resolved Hide resolved
These 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.



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

Expand Down Expand Up @@ -62,10 +78,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 above
kasium marked this conversation as resolved.
Show resolved Hide resolved
- ``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 +109,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 above


Layers
------
Expand Down Expand Up @@ -187,10 +198,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 above



Custom contract types
Expand Down
4 changes: 2 additions & 2 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,7 +24,7 @@ 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:
Expand Down
4 changes: 2 additions & 2 deletions src/importlinter/contracts/independence.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,15 +16,15 @@ 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
Expand Down
4 changes: 2 additions & 2 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,7 +51,7 @@ 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
Expand Down
16 changes: 10 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,20 @@ 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 handle wildcards:
kasium marked this conversation as resolved.
Show resolved Hide resolved
"mypackage.*.importer -> mypackage.bar.imp*"
kasium marked this conversation as resolved.
Show resolved Hide resolved
"""

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))
return ImportExpression(importer=importer, imported=imported)
kasium marked this conversation as resolved.
Show resolved Hide resolved
57 changes: 46 additions & 11 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
from importlinter.domain.ports.graph import ImportGraph


Expand All @@ -9,7 +11,7 @@ class MissingImport(Exception):


def pop_imports(
graph: ImportGraph, imports: Iterable[DirectImport]
graph: ImportGraph, imports: Iterable[ImportExpression]
kasium marked this conversation as resolved.
Show resolved Hide resolved
) -> List[Dict[str, Union[str, int]]]:
"""
Removes the supplied direct imports from the graph.
Expand All @@ -22,15 +24,19 @@ def pop_imports(
"""
removed_imports: List[Dict[str, Union[str, int]]] = []
for import_to_remove in imports:
import_details = graph.get_import_details(
importer=import_to_remove.importer.name, imported=import_to_remove.imported.name
)
if not import_details:
was_any_removed = False
kasium marked this conversation as resolved.
Show resolved Hide resolved

for (importer, imported) in to_modules(import_to_remove, graph):
import_details = graph.get_import_details(
importer=importer.name, imported=imported.name
)
if import_details:
was_any_removed = True
removed_imports.extend(import_details)
graph.remove_import(importer=importer.name, imported=imported.name)
if not was_any_removed:
raise MissingImport(f"Ignored import {import_to_remove} not present in the graph.")
removed_imports.extend(import_details)
graph.remove_import(
importer=import_to_remove.importer.name, imported=import_to_remove.imported.name
)

return removed_imports


Expand All @@ -55,3 +61,32 @@ 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
pattern_parts = []
for part in expression.split("."):
if "*" in part:
pattern_parts.append(part.replace("*", r"[^\.]+"))
else:
pattern_parts.append(part)
return re.compile(r"\.".join(pattern_parts))


def to_modules(
expression: ImportExpression, graph: ImportGraph
) -> Iterable[Tuple[Module, Module]]:
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)
21 changes: 21 additions & 0 deletions src/importlinter/domain/imports.py
Original file line number Diff line number Diff line change
Expand Up @@ -86,3 +86,24 @@ def __str__(self) -> str:

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


class ImportExpression(ValueObject):
"""
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 __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
12 changes: 6 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,7 @@ class TestModuleField(BaseFieldTest):
(
(
"mypackage.foo -> mypackage.bar",
DirectImport(importer=Module("mypackage.foo"), imported=Module("mypackage.bar")),
ImportExpression(importer="mypackage.foo", imported="mypackage.bar"),
),
(
["one", "two", "three"],
Expand All @@ -76,12 +76,12 @@ 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"),
),
),
)
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
12 changes: 11 additions & 1 deletion tests/unit/domain/test_imports.py
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
from contextlib import contextmanager

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


@contextmanager
Expand Down Expand Up @@ -76,3 +76,13 @@ def test_object_representation(self):
)
def test_string_object_representation(self, test_object, expected_string):
assert str(test_object) == expected_string


class TestImportExpression:
kasium marked this conversation as resolved.
Show resolved Hide resolved
def test_object_representation(self):
test_object = ImportExpression(importer="mypackage.foo", imported="mypackage.bar")
assert repr(test_object) == "<ImportExpression: mypackage.foo -> mypackage.bar>"

def test_string_object_representation(self):
expression = ImportExpression(importer="mypackage.foo", imported="mypackage.bar")
assert str(expression) == "mypackage.foo -> mypackage.bar"