Support module wildcards everywhere

fbinz commented 2 months ago

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?

fbinz commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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):

Automatically remove forbidden modules from the set of source modules for forbidden contracts. This does seem sensible to some extent.
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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

@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 / import-linter