-
Notifications
You must be signed in to change notification settings - Fork 49
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
Standalone contract #222
base: master
Are you sure you want to change the base?
Standalone contract #222
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -318,6 +318,33 @@ Note: you are not allowed to mix different kinds of separators on the same line. | |
mypackage.blue | mypackage.green : mypackage.yellow # Invalid as it mixes separators. | ||
mypackage.low | ||
|
||
Standalone modules | ||
------------------ | ||
|
||
*Type name:* ``standalone`` | ||
|
||
Standalone contracts check that a set of modules are standalone, that is not importing | ||
or imported by any other modules in the graph. | ||
|
||
**Example:** | ||
|
||
.. code-block:: ini | ||
|
||
[importlinter:contract:my-standalone-contract] | ||
name = My standalone contract | ||
type = standalone | ||
modules = | ||
mypackage.bar | ||
mypackage.baz | ||
ignore_imports = | ||
mypackage.bar.green -> mypackage.utils | ||
mypackage.foo.purple -> mypackage.baz.blue | ||
|
||
**Configuration options** | ||
|
||
- ``modules``: A list of modules/subpackages that should be independent of each other. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This line needs to be updated as it's from the independence contract. |
||
- ``ignore_imports``: See :ref:`Shared options`. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should also add |
||
|
||
|
||
Custom contract types | ||
--------------------- | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,64 @@ | ||
from __future__ import annotations | ||
|
||
from grimp import ImportGraph | ||
|
||
from importlinter.application import contract_utils, output | ||
from importlinter.domain import fields | ||
from importlinter.domain.contract import Contract, ContractCheck | ||
|
||
|
||
class StandaloneContract(Contract): | ||
""" | ||
Standalone contracts check that a set of modules are standalone, that is not importing | ||
or imported by any other modules in the graph. | ||
|
||
Configuration options: | ||
|
||
- modules: A list of Modules that should be standalone. | ||
- 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 = "standalone" | ||
|
||
modules = fields.ListField(subfield=fields.ModuleField()) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Any reason not to make this a |
||
ignore_imports = fields.SetField(subfield=fields.ImportExpressionField(), required=False) | ||
|
||
def check(self, graph: ImportGraph, verbose: bool) -> ContractCheck: | ||
warnings = contract_utils.remove_ignored_imports( | ||
graph=graph, | ||
ignore_imports=self.ignore_imports, # type: ignore | ||
unmatched_alerting="none", # type: ignore | ||
) | ||
|
||
self._check_all_modules_exist_in_graph(graph) | ||
|
||
violations = {} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would be helpful to type annotate what type of element this set contains. |
||
for module in self.modules: # type: ignore | ||
imports = graph.find_modules_directly_imported_by(module.name) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Minor suggestion: rename |
||
imported_by = graph.find_modules_that_directly_import(module.name) | ||
if imported_by or imports: | ||
violations[module.name] = [ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I find this block a bit difficult to read. Maybe better to tackle the importeds and the importers in two separate statements? It would also be nice to use the |
||
(module.name, import_expression) for import_expression in imported_by | ||
] + [(import_expression, module.name) for import_expression in imports] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What about calling
What do you think? Would be nice to have line numbers. |
||
|
||
kept = all(len(violation) == 0 for violation in violations.values()) | ||
return ContractCheck( | ||
kept=kept, | ||
warnings=warnings, | ||
metadata={"violations": violations}, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could we put these under a key called |
||
) | ||
|
||
def render_broken_contract(self, check: "ContractCheck") -> None: | ||
for module_name, connections in check.metadata["violations"].items(): | ||
output.print(f"{module_name} must be standalone:") | ||
output.new_line() | ||
for upstream, downstream in connections: | ||
output.print_error(f"- {downstream} is not allowed to import {upstream}") | ||
output.new_line() | ||
|
||
def _check_all_modules_exist_in_graph(self, graph: ImportGraph) -> None: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Now that this is used by two contracts, would you mind moving this to |
||
for module in self.modules: # type: ignore | ||
if module.name not in graph.modules: | ||
raise ValueError(f"Module '{module.name}' does not exist.") |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,162 @@ | ||
from __future__ import annotations | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice tests, thanks! |
||
|
||
import pytest | ||
from grimp.adaptors.graph import ImportGraph | ||
|
||
from importlinter.application.app_config import settings | ||
from importlinter.contracts.standalone import StandaloneContract | ||
from importlinter.domain.contract import ContractCheck | ||
from tests.adapters.printing import FakePrinter | ||
from tests.adapters.timing import FakeTimer | ||
|
||
|
||
@pytest.fixture(scope="module", autouse=True) | ||
def configure(): | ||
settings.configure(TIMER=FakeTimer()) | ||
|
||
|
||
class TestStandaloneContract: | ||
def _build_default_graph(self): | ||
graph = ImportGraph() | ||
for module in ( | ||
"mypackage", | ||
"mypackage.blue", | ||
"mypackage.blue.alpha", | ||
"mypackage.blue.beta", | ||
"mypackage.blue.beta.foo", | ||
"mypackage.blue.foo", | ||
"mypackage.blue.hello", | ||
"mypackage.blue.world", | ||
"mypackage.green", | ||
"mypackage.green.bar", | ||
"mypackage.yellow", | ||
"mypackage.yellow.gamma", | ||
"mypackage.yellow.delta", | ||
"mypackage.other", | ||
"mypackage.other.sub", | ||
"mypackage.other.sub2", | ||
): | ||
graph.add_module(module) | ||
return graph | ||
|
||
def _check_default_contract(self, graph): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not a blocker, but IMO it would be better if we passed in the set of modules that we want to be standalone from the test itself - they're a bit hidden in here. |
||
contract = StandaloneContract( | ||
name="Standalone contract", | ||
session_options={"root_packages": ["mypackage"]}, | ||
contract_options={"modules": ("mypackage.green", "mypackage.yellow")}, | ||
) | ||
return contract.check(graph=graph, verbose=False) | ||
|
||
def test_when_modules_are_standalone(self): | ||
graph = self._build_default_graph() | ||
graph.add_import( | ||
importer="mypackage.blue", | ||
imported="mypackage.other", | ||
line_number=10, | ||
line_contents="-", | ||
) | ||
graph.add_import( | ||
importer="mypackage.other", | ||
imported="mypackage.blue.world", | ||
line_number=11, | ||
line_contents="-", | ||
) | ||
|
||
contract_check = self._check_default_contract(graph) | ||
|
||
assert contract_check.kept, contract_check.metadata | ||
|
||
def test_non_standalone_imported(self): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Mind adding a test case for multiple importeds and importers to/from multiple modules, just to check the logic of handling multiple violations? |
||
graph = self._build_default_graph() | ||
graph.add_import( | ||
importer="mypackage.blue", | ||
imported="mypackage.green", | ||
line_number=10, | ||
line_contents="-", | ||
) | ||
contract_check = self._check_default_contract(graph) | ||
|
||
assert not contract_check.kept | ||
|
||
expected_metadata = { | ||
"violations": {"mypackage.green": [("mypackage.green", "mypackage.blue")]} | ||
} | ||
assert expected_metadata == contract_check.metadata | ||
|
||
def test_non_standalone_imports(self): | ||
graph = self._build_default_graph() | ||
graph.add_import( | ||
importer="mypackage.yellow", | ||
imported="mypackage.other", | ||
line_number=10, | ||
line_contents="-", | ||
) | ||
|
||
contract_check = self._check_default_contract(graph) | ||
|
||
assert not contract_check.kept | ||
|
||
expected_metadata = { | ||
"violations": {"mypackage.yellow": [("mypackage.other", "mypackage.yellow")]} | ||
} | ||
assert expected_metadata == contract_check.metadata | ||
|
||
def test_standalone_ignore(self): | ||
graph = self._build_default_graph() | ||
graph.add_import( | ||
importer="mypackage.yellow", | ||
imported="mypackage.other", | ||
line_number=10, | ||
line_contents="-", | ||
) | ||
|
||
contract = StandaloneContract( | ||
name="Standalone contract", | ||
session_options={"root_packages": ["mypackage"]}, | ||
contract_options={ | ||
"modules": ("mypackage.green", "mypackage.yellow"), | ||
"ignore_imports": ["mypackage.yellow -> mypackage.other"], | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Very minor comment, but we could allow passing in the |
||
}, | ||
) | ||
contract_check = contract.check(graph=graph, verbose=False) | ||
|
||
assert contract_check.kept | ||
|
||
|
||
def test_render_broken_contract(): | ||
settings.configure(PRINTER=FakePrinter()) | ||
contract = StandaloneContract( | ||
name="Standalone contract", | ||
session_options={"root_packages": ["mypackage"]}, | ||
contract_options={"modules": ["mypackage.green"]}, | ||
) | ||
check = ContractCheck( | ||
kept=False, | ||
metadata={ | ||
"violations": { | ||
"mypackage": [ | ||
("mypackage.blue.foo", "mypackage.utils.red"), | ||
("mypackage.blue.red", "mypackage.utils.yellow"), | ||
], | ||
"mypackage.green": [ | ||
("mypackage.green.a.b", "mypackage.green.b.a"), | ||
], | ||
} | ||
}, | ||
) | ||
|
||
contract.render_broken_contract(check) | ||
|
||
settings.PRINTER.pop_and_assert( | ||
""" | ||
mypackage must be standalone: | ||
|
||
- mypackage.utils.red is not allowed to import mypackage.blue.foo | ||
- mypackage.utils.yellow is not allowed to import mypackage.blue.red | ||
|
||
mypackage.green must be standalone: | ||
|
||
- mypackage.green.b.a is not allowed to import mypackage.green.a.b | ||
|
||
""" # noqa | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why the |
||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be better to remove the
ignore_imports
section from the example, just so it's minimal.