Skip self-pairs in forbidden contracts so wildcards can include the source#361
Conversation
…ource A forbidden contract with a wildcard forbidden module that also matches one of its source modules (e.g. source_modules = mypackage.core, forbidden_modules = mypackage.*) previously failed with "Modules have shared descendants", because the source/forbidden pair referring to the same package was passed to find_shortest_chains. Skip any pair where the source and forbidden module are the same module, or (when treated as packages) one contains the other. A module cannot be forbidden from importing itself, so skipping is the correct semantics, and it lets a wildcard cover all sibling packages, including ones added later, without listing them or excluding the source by hand. Closes seddonym#360
seddonym
left a comment
There was a problem hiding this comment.
Thanks for your submission, and apologies it has taken a while to get to it.
I agree this is has been a problem and I agree with the way you've solved it - nice work.
The implementation is basically there, I've just requested a couple of small tweaks, and have pushed some alternative documentation. The main thing to add is test coverage for the as_packages code. Let me know if you have any concerns or questions!
| """ | ||
| if source_module == forbidden_module: | ||
| return True | ||
| if self.as_packages: |
There was a problem hiding this comment.
Could we move this method to be a function that has as_packages passed in? Makes it more functional - it's entirely dependent on its inputs.
| """ | ||
| if source_module == forbidden_module: | ||
| return True | ||
| if self.as_packages: |
There was a problem hiding this comment.
If I comment out this if block, the tests still pass. Could we add test coverage for this bit?
|
|
||
| External packages may also be forbidden. | ||
|
|
||
| If a source module and a forbidden module refer to the same module (or, when treated as packages, |
There was a problem hiding this comment.
a module cannot be forbidden from importing itself
I think this is a potentially confusing way to put it - in this context, a module can be a subpackage, and imports do exist within subpackages. For example, importlinter.domain.fields imports importlinter.domain.imports, but we might still want to make importlinter.domain a leaf subpackage.
I've pushed up a commit that rewrites the docs in a way that I hope is clearer, let me know what you think.
|
|
||
| * Improve error message when root package is a single-file module. | ||
| * Alert users with all unmatched ignored imports in the same run. | ||
| * Forbidden contracts: skip any source/forbidden pair that refers to the same module (or, as |
There was a problem hiding this comment.
Suggest replacing this whole thing with just Allow overlapping modules in forbidden contracts. Users can consult the docs for more info.
| Forbidden contracts check that one set of modules are not imported by another set of modules. | ||
| Indirect imports will also be checked. | ||
|
|
||
| Any pair where the source and forbidden module are the same (or, when treated as packages, one |
There was a problem hiding this comment.
We should replace this with whatever ends up in the docs.
| output.verbose_print( | ||
| verbose, | ||
| f"Skipping {source_module} -> {forbidden_module} " | ||
| "(a module cannot be forbidden from importing itself).", |
There was a problem hiding this comment.
a module cannot be forbidden from importing itself
This is misleading - let's just log "Skipping overlapping modules {source_module} and {forbidden_module}."
- Make `_modules_overlap` a module-level function taking `as_packages` as an
argument, rather than a method reading it from the instance.
- Reword the verbose log to "Skipping overlapping modules {a} and {b}.".
- Align the contract docstring with the documentation and drop the misleading
"a module cannot be forbidden from importing itself" phrasing.
- Shorten the release note to "Allow overlapping modules in forbidden contracts.".
- Add test coverage for the as_packages branch: overlapping (containing)
modules are skipped when as_packages is True and still checked when False.
|
Thanks for the review and the docs rewrite. Pushed f40ce3c addressing all points:
The behavior matches your docs: with |
|
That was quick. Thanks - great contribution! |
|
This is now available in the latest release, 2.12. Enjoy! |
|
Thanks for the fast merge and release! |
Closes #360.
Problem
A
forbiddencontract is the natural way to enforce that a package is a leaf, i.e. it may import only itself (and external libraries) and none of its sibling packages, while siblings may still import it. The obvious config is:But
mypackage.*also matchesmypackage.coreitself, and the check fails with:because the
mypackage.core -> mypackage.corepair is passed tofind_shortest_chains.ignore_importsdoes not help: it removes edges from the graph, not the module from the expanded forbidden set, so the self-pair is still evaluated. The only workarounds today are enumerating every sibling by hand (and keeping the list in sync as packages are added) or writing a custom contract type.Change
In
ForbiddenContract.check, skip any (source, forbidden) pair that refers to the same module, or, when treated as packages, where one contains the other. A module cannot be meaningfully forbidden from importing itself, so skipping is the correct semantics rather than an error. This lets a wildcard such asmypackage.*be used as a forbidden module even when a source module is one of the modules it matches, and newly added sibling packages are covered automatically.The guard runs before both the direct and indirect (
allow_indirect_imports) code paths, so neither raises nor reports intra-package imports for the skipped pair.Tests
Added unit tests in
tests/unit/contracts/test_forbidden.py:as_packagesTrue and FalseDocs
Per CONTRIBUTING, this follows up the filed issue #360; happy to adjust the approach (e.g. an opt-in flag, or an exclusion syntax in module expressions) if you'd prefer a different direction.