Support module wildcards everywhere

[fbinz]

Hey,
after stumbling upon this issue, I decided to give the problem a go myself.
I tried to follow the ideas you outlined in the linked PR.

Up until now, I've implemented the following refactorings/features:

• Introduce ModuleExpression (Field and Domain-Object)
• Refactor ImportExpression to re-use ModuleExpressions, to keep the - albeit limited - logic in one place
• Refactor _expression_to_modules so that it works with ModuleExpression
• Make ForbiddenContract accept ModuleExpressionFields instead of ModuleFields - the former being a superset of the latter, so the tests all still pass
• Add two very basic tests for the wildcard behavior of the source_modules and forbidden_module fields of ForbiddenContract

I know, that I'm far from finished, but maybe you could have a look and check if I'm on the right track?

You also raised a concern regarding the semantics of the wildcards:

For example, what would it actually mean if we listed mypackage.** in an independence contract? What about mypackage.*.foo?

Maybe we could solve this similarly to how .gitignore files do it, namely have a !package.name pattern, to remove matches from the set of modules again?

[seddonym]

This is great stuff, thanks for putting the effort in.

I really like where you've got to, and most of my comments are small things. I don't think there is much more. On top of those, this is what I think is left:

• Add docs for the contracts. (You can run tox -edocsand then open dist/docs/index.html if you want to try rendering them locally.)
• Add a line to the CHANGELOG.
• Add yourself to CONTRIBUTORS. 🏅

Can you think of anything else?

PS If you're not confident about writing docs then I am happy to take that on.

[fbinz]

This is great stuff, thanks for putting the effort in.

Great to hear. Was actually really fun to hack on. :)
I'll add the tests you proposed and make the changes.

Can you think of anything else?

What about the layer contracts? Would wildcards make sense here?

[seddonym]

What about the layer contracts? Would wildcards make sense here?

Hmm, I guess they could indicate siblings within the same layer, but I suspect it might add unnecessary complexity. Let's leave it for now.

[fbinz]

I made the changes you proposed and noticed something funny: The feature does not even solve my problem. :D

In my django project, I have a dashboard app, that should be independent of all other apps.
It was at this point, that I thought, that it would be nice to express this "all other apps" using a wildcard.

I guess I should have tried this sooner, but it seemed to be such an obvious thing, that wildcards would solve my problem, that I didn't give it more thought.

So, what I would have liked is the following to work:

[[tool.importlinter.contracts]]
name = "Nothing depends on the dashboard app"
type = "forbidden"
source_modules = ["apps.*"]
forbidden_modules = ["apps.dashboard"]

Running this with the new wildcards support does not crash (before it complained about apps.* not being a valid module), but now it quite correctly says, that "Modules have shared descendants". My guess it, that this is due to the fact that apps.* of course also includes the forbidden module apps.dashboard.

I see two options (for this particular case):

1. Automatically remove forbidden modules from the set of source modules for forbidden contracts. This does seem sensible to some extent.
2. Introduce a way, to remove modules from a set of modules.

Regarding option 2, as I mentioned earlier, maybe something like the .gitignore syntax could work:

[[tool.importlinter.contracts]]
name = "Nothing depends on the dashboard app"
type = "forbidden"
source_modules = ["apps.*", "!apps.dashboard"]
forbidden_modules = ["apps.dashboard"]

What are your thoughts on this?

[GitRon]

Quick 2 cents on the topic:

Automatically remove forbidden modules from the set of source modules for forbidden contracts. This does seem sensible to some extent.

Yes, that's the way to go IMHO. Realised this earlier but still... this will never work. So why bother having it in? Just document that we'll remove the forbidden modules and we have the convenience we desire. Right? 🙂

[seddonym]

Thanks for the latest round of changes. This is nearly there.

I've left a few comments which are not blockers - I appreciate you've already put plenty of time into this and it might be easier for me just to make the changes once the PR is merged. The only blockers are these:

• The changes related to releases.
• The changes in Return set of Modules when resolving module expressions. I don't mind if you just remove that commit, unless you feel like adjusting it as per my comments.

Regarding the question of actually solving your problem, I'll reply separately! I think in its current form it will still be useful to others.

[seddonym]

Not convinced we should be changing the tests - if anything it's a smell that the Import Linter is not behaving deterministically, which I think should at least be an aspiration.

I think a better way to address this is just to move the sort up the stack, so while module_expressions_to_modules returns a set, the check methods that use it sort the results.

[fbinz]

Yeah, good idea!
But I guess as soon as I introduce any kind of ordering, I'll have to adjust the order in the assertions aswell, right? Or what do you mean by "changing the tests"?

[seddonym]

Oh I see what you mean. Yes I don't mind if the assertions change if the ordering becomes alphabetical.

[fbinz]

Hm, I'm not able to reproduce the original issue currently. I.e. all tests now pass on Python 3.12 at least.

If I understand correctly, tox should run the tests etc. for several python versions.
However, if I try to run tox locally, I get the following error:

    eps = importlib_metadata.entry_points().get(self.namespace, ())
          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
AttributeError: 'EntryPoints' object has no attribute 'get'
check: exit 1 (0.11 seconds) /home/binz/Repositories/import-linter> flake8 src tests pid=763946

This seems to be related to importlib_metadata changing stuff, but installing a different version (the proposed solution found by googling) does not work.

How did you setup tox locally?

[fbinz]

While I have still not run tox locally, I fixed the typing error related to Set[...] vs set[...]. So hopefully, everything passes now

[seddonym]

Sorry for the delay in replying. If you create a new virtual environment and then pip install tox into it, you should be able to then run it. The only thing is you'll need to make sure you have Python versions accessible for each version of Python you want to run it under. I would suggest just picking one and running that (I use pyenv for that).

E.g. running tox on 3.10 you can do tox -epy310.

[fbinz]

Ah, I see. I somehow expected tox to switch python versions for. No idea why. :D
So, now the output should be stable, since I'm ordering both source and forbidden modules of a forbidden contract.

[seddonym]

The feature does not even solve my problem.

😭 Oh well - it's still a good contribution. In the interests of working incrementally, I don't think we should try to tackle this as part of this PR, better to figure it out as a separate one once we've decided what to do.

Let's take each option in turn.

Automatic exclusion from forbidden modules

My concern with assuming that source module wildcards don't include forbidden modules is, what about the other way around? Maybe someone would want to do this:

[[tool.importlinter.contracts]]
name = "Nothing depends on the dashboard app"
type = "forbidden"
source_modules = ["apps.utils"]
forbidden_modules = ["apps.*"]

Perhaps there is an easy-to-explain rule that we could implement that covers both use cases, but instinctively I worry that we're opening up a world of complexity there. Feel free to suggest something though.

Exclusion expressions

Re. providing a syntax to ignore certain modules - I think it would make more sense to incorporate these into individual module expressions - heading down the route to regular expressions I guess. But for me there's a risk that it will impair readability, so I'm not convinced.

Layers

The way I would solve this problem would be not to use a forbidden module at all, but instead use a layers contract:

[importlinter:contract:layers]
name=Nothing depends on the dashboard app
type=layers
exhaustive = True
containers=apps
layers=
    dashboard
    blue : green : red

For me, that's more expressive of your architecture, though it does have the down side of needing to populate the layers below explicitly. (Still, because it's marked as exhaustive the contract will fail if someone adds a new one without listing it here.)

Interesting to know anyone's thoughts.

[fbinz]

The feature does not even solve my problem.

😭 Oh well - it's still a good contribution. In the interests of working incrementally, I don't think we should try to tackle this as part of this PR, better to figure it out as a separate one once we've decided what to do.

I agree!

Perhaps there is an easy-to-explain rule that we could implement that covers both use cases, but instinctively I worry that we're opening up a world of complexity there. Feel free to suggest something though.

We could introduce another flag of sorts, that specifies how the two sets (source_modules and forbidden_modules) relate to each other. But that seems to be a very ad-hoc solution.

Re. providing a syntax to ignore certain modules - I think it would make more sense to incorporate these into individual module expressions - heading down the route to regular expressions I guess. But for me there's a risk that it will impair readability, so I'm not convinced.

While the syntax I proposed would be part of the module expression ("!mypackage.blue" would exclude that specific package from the set), I'd also rather not add it. So I'll try how layers fit my use case. Maybe I'll add wildcards there too. ;)

[seddonym]

So I'll try how layers fit my use case.

Cool, let me know how it goes.

Maybe I'll add wildcards there too. ;)

Yeah that might be a nice addition. If we did that, we'd need to figure out how to specify whether the siblings represented by a wildcard are independent or dependent (currently denoted by | versus : separators).

[jstriebel]

Awesome PR, thank you @fbinz & @seddonym!

Just adding one more thought to the "shared descendants problem": This can also be an issue without using wildcards, e.g.

[[tool.importlinter.contracts]]
name = "Nothing depends on the dashboard app"
type = "forbidden"
source_modules = ["apps.utils"]
forbidden_modules = ["apps"]

In this case one could say that apps.utils is more specific than apps and could be excluded automatically, but that's much harder to argue for wildcard-expressions.
If the ! syntax would be introduced I'm wondering whether forbidden_modules = ["apps", "!apps.dashboard"] would also work, so it would be similar to forbidden_modules = ["apps.*", "!apps.dashboard"].

[fbinz]

@seddonym

So I'll try how layers fit my use case.

Cool, let me know how it goes.

So, I tried layers and they indeed work quite well. The "exhaustive" part is a nice touch and at least won't let me forget to extend the list.

Regarding wildcard support in layer contracts, we would have the same problems wrt to excluding modules from the set defined by a wildcard expression.
And, as you've mentioned, we'd need to find a way to express the : and | syntax in a wildcard.

I'll think about this some more. :)

[seddonym]

Great PR! Thanks for all your hard work.