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

Module wildcard expression in independence contract #220

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
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
11 changes: 10 additions & 1 deletion src/importlinter/contracts/independence.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@

from importlinter.application import contract_utils, output
from importlinter.application.contract_utils import AlertLevel
from importlinter.domain import fields
from importlinter.domain import fields, helpers
from importlinter.domain.contract import Contract, ContractCheck
from importlinter.domain.imports import Module

Expand Down Expand Up @@ -52,6 +52,15 @@ def check(self, graph: ImportGraph, verbose: bool) -> ContractCheck:
unmatched_alerting=self.unmatched_ignore_imports_alerting, # type: ignore
)

# resolve wildcards
modules = set()
for module in self.modules: # type: ignore
if not module.has_wildcard_expression():
modules.add(module)
else:
modules = modules.union(helpers._resolve_wildcards(module.name, graph))
self.modules = list(modules) # type: ignore

self._check_all_modules_exist_in_graph(graph)

dependencies = graph.find_illegal_dependencies_for_layers(
Expand Down
37 changes: 20 additions & 17 deletions src/importlinter/domain/fields.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,21 @@
FieldValue = TypeVar("FieldValue")


def _validate_wildcard(expression: str) -> None:
Copy link
Owner

Choose a reason for hiding this comment

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

I wonder if we should be careful about using the same wildcard syntax used by ignored imports. For example, what would it actually mean if we listed mypackage.** in an independence contract? What about mypackage.*.foo?

I had been anticipating a much more limited set of expressions: i.e. just of the form mypackage.*.

Then again, if the functionality is already there, then perhaps it's counterproductive to special-case it. People may come up with use cases we hadn't thought of. What do you think?

last_wildcard = None
for part in expression.split("."):
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 NotSupplied:
"""Sentinel to use in place of None for a default argument value."""

Expand Down Expand Up @@ -156,7 +171,9 @@ class ModuleField(Field):
"""

def parse(self, raw_data: Union[str, List]) -> Module:
return Module(StringField().parse(raw_data))
module = Module(StringField().parse(raw_data))
_validate_wildcard(module.name)
return module


class ImportExpressionField(Field):
Expand All @@ -181,25 +198,11 @@ def parse(self, raw_data: Union[str, List]) -> ImportExpression:
if not (importer and imported):
raise ValidationError('Must be in the form "package.importer -> package.imported".')

self._validate_wildcard(importer)
self._validate_wildcard(imported)
_validate_wildcard(importer)
Copy link
Owner

Choose a reason for hiding this comment

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

Not a blocker, but if we introduce the concept of ModuleExpression, it might be nice to use them within ImportExpression objects, which could become a pair of ModuleExpressions.

_validate_wildcard(imported)

return ImportExpression(importer=importer, imported=imported)

def _validate_wildcard(self, expression: str) -> None:
last_wildcard = None
for part in expression.split("."):
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
51 changes: 24 additions & 27 deletions src/importlinter/domain/helpers.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
from __future__ import annotations
Copy link
Owner

Choose a reason for hiding this comment

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

👏🏻


import itertools
import re
from typing import Iterable, List, Pattern, Set, Tuple
from typing import Iterable, Pattern

from grimp import DetailedImport

Expand All @@ -12,7 +14,7 @@ class MissingImport(Exception):
pass


def pop_imports(graph: ImportGraph, imports: Iterable[DirectImport]) -> List[DetailedImport]:
def pop_imports(graph: ImportGraph, imports: Iterable[DirectImport]) -> list[DetailedImport]:
"""
Removes the supplied direct imports from the graph.

Expand All @@ -22,7 +24,7 @@ def pop_imports(graph: ImportGraph, imports: Iterable[DirectImport]) -> List[Det
Raises:
MissingImport if an import is not present in the graph.
"""
removed_imports: List[DetailedImport] = []
removed_imports: list[DetailedImport] = []

imports_to_remove = _dedupe_imports(imports)

Expand All @@ -44,18 +46,18 @@ def pop_imports(graph: ImportGraph, imports: Iterable[DirectImport]) -> List[Det

def import_expression_to_imports(
graph: ImportGraph, expression: ImportExpression
) -> List[DirectImport]:
) -> list[DirectImport]:
"""
Returns a list of imports in a graph, given some import expression.

Raises:
MissingImport if an import is not present in the graph. For a wildcarded import expression,
this is raised if there is not at least one match.
"""
imports: Set[DirectImport] = set()
imports: set[DirectImport] = set()
matched = False

for (importer, imported) in _expression_to_modules(expression, graph):
for importer, imported in _expression_to_modules(expression, graph):
import_details = graph.get_import_details(importer=importer.name, imported=imported.name)

if import_details:
Expand All @@ -80,7 +82,7 @@ def import_expression_to_imports(

def import_expressions_to_imports(
graph: ImportGraph, expressions: Iterable[ImportExpression]
) -> List[DirectImport]:
) -> list[DirectImport]:
"""
Returns a list of imports in a graph, given some import expressions.

Expand All @@ -99,7 +101,7 @@ def import_expressions_to_imports(

def resolve_import_expressions(
graph: ImportGraph, expressions: Iterable[ImportExpression]
) -> Tuple[Set[DirectImport], Set[ImportExpression]]:
) -> tuple[set[DirectImport], set[ImportExpression]]:
"""
Find any imports in the graph that match the supplied import expressions.

Expand All @@ -116,12 +118,12 @@ def resolve_import_expressions(
except MissingImport:
unresolved_expressions.add(expression)

return (resolved_imports, unresolved_expressions)
return resolved_imports, unresolved_expressions


def pop_import_expressions(
graph: ImportGraph, expressions: Iterable[ImportExpression]
) -> List[DetailedImport]:
) -> list[DetailedImport]:
"""
Removes any imports matching the supplied import expressions from the graph.

Expand All @@ -135,7 +137,7 @@ def pop_import_expressions(
return pop_imports(graph, imports)


def add_imports(graph: ImportGraph, import_details: List[DetailedImport]) -> None:
def add_imports(graph: ImportGraph, import_details: list[DetailedImport]) -> None:
"""
Adds the supplied import details to the graph.

Expand Down Expand Up @@ -187,7 +189,7 @@ def _dedupe_imports(imports: Iterable[DirectImport]) -> Iterable[DirectImport]:
This is to make it easy for the calling function to remove the set of imports from a graph
without attempting to remove certain imports twice.
"""
deduped_imports: List[DirectImport] = []
deduped_imports: list[DirectImport] = []

# Why don't we use a set here? Because we want to preserve the order (mainly for testability).
imports_without_metadata = [
Expand Down Expand Up @@ -215,23 +217,18 @@ def _to_pattern(expression: str) -> Pattern:
return re.compile(r"^" + r"\.".join(pattern_parts) + r"$")


def _resolve_wildcards(value: str, graph: ImportGraph) -> set[Module]:
pattern = _to_pattern(value)
return {Module(module) for module in graph.modules if pattern.match(module)}


def _expression_to_modules(
expression: ImportExpression, graph: ImportGraph
) -> Iterable[Tuple[Module, Module]]:
) -> 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:

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

return itertools.product(set(importer), set(imported))
return itertools.product(
_resolve_wildcards(expression.importer, graph),
_resolve_wildcards(expression.imported, graph),
)
3 changes: 3 additions & 0 deletions src/importlinter/domain/imports.py
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,9 @@ def __init__(self, name: str) -> None:
"""
self.name = name

def has_wildcard_expression(self) -> bool:
Copy link
Owner

Choose a reason for hiding this comment

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

My concern with this approach is that we're conflating modules with module expressions. I think it's important to keep this distinction, as we have done with the difference between the DirectImport and ImportExpression types.

With that in mind, I think we should add a new type called a ModuleExpression together with a ModuleExpressionField. We should then adjust the independence contract module field to be like so:

class IndependenceContract(Contract):
    modules = fields.ListField(subfield=fields.ModuleExpressionField())
    ...

The IndependenceContract.check method can then call a function in contract_utils (maybe called resolve_module_expressions?) which would turn an iterable of ModuleExpression objects into a set of Module objects. What do you think of that approach?

return "*" in self.name

def __str__(self) -> str:
return self.name

Expand Down
Loading