qossmic / deptrac

Keep your architecture clean.
https://qossmic.github.io/deptrac
MIT License
2.61k stars 135 forks source link

Add "must_any" to BoolCollector #1329

Closed brightbyte closed 6 months ago

brightbyte commented 8 months ago

Use case: Assume we have a layer X that consists of classes that have either "Impl" in the class name, or that habe the "@impl" tag. It uses two collectors, and any class matching either one will be part of the layer.

No we want to exclude things that are in layer Y from layer X. This is not possible with the available "must" and "must_not" options in the BoolCollector, since we have no way to keep the idea that classes with "Impl" in the name OR the "@impl" tag should be part of the layer (unless they are already part of layer Y).

The introduction of the "must_any" attribute allows to address this use case.

patrickkusebauch commented 8 months ago

Interesting idea.

You could use the layer collector in your must clause in the use case you described.

brightbyte commented 8 months ago

You could use the layer collector in your must clause in the use case you described.

You mean, define layer X with classes that have "Impl" in the class name or that habe the "@impl" tag, then Define layer X_without_Y using the bool collector? Yea, but it feels wrong to define a layer just for the sake of arithmetics. And I think it's harder to read.

I think this would be especially useful if there is something that you want to exclude from a lot of layers. E.g. define all your layers to exclude deprecated classes. To do that without must_any, you'd have to double the number of layers.

But I also see the value in keeping things simple. I hope you don't mind me making PRs just to try out ideas :)

patrickkusebauch commented 8 months ago

No, I don't mind. If you don't mind potentially easing code it it gets rejected.

I think it would help me to give a real life use case you have in mind instead of a simple example.

brightbyte commented 8 months ago

@patrickkusebauch Here's the exampel that tripped me up when trying to make a basic deptrac config for MediaWiki.

I want to define a layer of "special pages", which implement formed based user interaction. Nothing should depend on these classes.

  layers:
    - name: SpecialPages
      collectors:
            - type: directory
              value: includes/specials
            - type: directory
              value: includes/specialpage

However, other code needs access to the SpecialPageFactory. So I try to exclude it from the layer:

  layers:
    - name: SpecialPages
      collectors:
        - type: bool
          must:
            - type: directory
              value: includes/specials
            - type: directory
              value: includes/specialpage
          must_not:
            - type: class
              value: MediaWiki\\SpecialPage\\SpecialPageFactory

This of course doesn't work, since the "must" section will never be satisfied. There are various way to work around this, including defining intermediate layers or using a single collector with a more compelx regex for the "must" section. But it just seemed more natural to allow an existing list of collectors to me moved into a bool collector. With this patch, onme would be able to achieve the need to exclude the factory in a straight forward way:

  layers:
    - name: SpecialPages
      collectors:
        - type: bool
          must_any:
            - type: directory
              value: includes/specials
            - type: directory
              value: includes/specialpage
          must_not:
            - type: class
              value: MediaWiki\\SpecialPage\\SpecialPageFactory
timglabisch commented 8 months ago

you're looking for something like should?

brightbyte commented 8 months ago

you're looking for something like should?

That indeed looks like it would produce similar behavior. But I wouldn't want to use the term "should". Searches produce open ended results with a well defined ranking, that's not what we want here. We are explicitly in the land of boolean (or set) algebra, "should" has no place there.

Basically, the bool collector can currently be used to implement the AND, NOT, and NOR operators. I want to add a way to implement OR, without resorting to DeMorgan.

patrickkusebauch commented 8 months ago

I see, makes sense, my idea was:

  layers:
    - name: SpecialPages
      collectors:
        - type: bool
          must:
            - type: directory
              value: includes/specials
          must_not:
            - type: class
              value: MediaWiki\\SpecialPage\\SpecialPageFactory
        - type: bool
          must:
            - type: directory
              value: includes/specialpage
          must_not:
            - type: class
              value: MediaWiki\\SpecialPage\\SpecialPageFactory

which is DeMorgan. And yeah, I do not like the duplication and lack of expressiveness.

gennadigennadigennadi commented 6 months ago

Please update the git origin for development to https://github.com/qossmic/deptrac-src and reopen the PR. Thank you very much!