Skip to content
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

Add "must_any" to BoolCollector #1329

Closed
wants to merge 1 commit into from
Closed

Conversation

brightbyte
Copy link

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.

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
Copy link
Collaborator

Interesting idea.

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

@brightbyte
Copy link
Author

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
Copy link
Collaborator

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
Copy link
Author

brightbyte commented Jan 2, 2024

@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
Copy link
Collaborator

you're looking for something like should?

@brightbyte
Copy link
Author

brightbyte commented Jan 3, 2024

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
Copy link
Collaborator

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.

Copy link
Collaborator

@patrickkusebauch patrickkusebauch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 LGTM

@gennadigennadigennadi
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants