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

Baseline should still work when allowed namespaces are changed #377

Open
dbu opened this issue Apr 4, 2023 · 4 comments
Open

Baseline should still work when allowed namespaces are changed #377

dbu opened this issue Apr 4, 2023 · 4 comments

Comments

@dbu
Copy link
Contributor

dbu commented Apr 4, 2023

Feature Request

Q A
New Feature yes
RFC no
BC Break no

Summary

The baseline stores the full error message, which in case of namespace rules contains the list of all allowed namespaces. When the configuration is changed, all baseline entries no longer match.

The part that explains what is allowed ...but should depend only on classes in one of these namespaces ... should not be part of the baseline and not be required to match.

My idea would be to split the exception message into the actual error and a hint part. The hint should not be stored in the baseline and not be used when comparing with the baseline.

@dbu
Copy link
Contributor Author

dbu commented Apr 4, 2023

@LuigiCardamone in #365 you proposed to store the rule name and rule parameters for the baseline. the problem with using the name is when we have name clashes. and i think storing the rule parameters means that a baseline entry will still not match any longer if e.g. an additional namespace is added, or one is removed. imo the baseline should not care about that - if the scan finds a problem that we have previously seen and is thus stored in the baseline, we want to ignore it.

@LuigiCardamone
Copy link
Contributor

@dbu you are right, since in #365 I wrote the wrong name. Please, consider violationArguments in place of ruleParameters. The baseline structure should be like this:

{
    "violations": [
        {
            "ruleName": "Domain protection rule",
            "violationArguments": ["AppBundle\\Infra\\Book"],
            "errorMessage": "Domain\\Book depends on AppBundle\\Infra\\Book, but should depend only on classes in one of these namespaces: AppBundle\\Domain"
        }
    ]
}

Like you, I was thinking that the line number in the baseline is useless for many rules .
If a Class A has a wrong dependency on Class B we don't care about the lines in which these violations happens.
If violation A --> B is in baseline, every new code added to Class A should not be checked for violation A --> B.

P.S.
I don't get your comment about "name clashes". Rules should have unique names and this can be checked easily.

@dbu
Copy link
Contributor Author

dbu commented Apr 4, 2023

i thought you meant the reason that the user gives when setting the rules. when users provide that, i expect some people copy-paste and don't notice that they could adjust the name.

i don't see explicit rule names in the code. e.g. DependsOnlyOnTheseNamespaces creates a violation and just creates the message on the fly.

if you mean to add a rule identifier name to every rule, i think that is the best approach to this, as it keeps the freedom to tweak the message that is printed without breaking the baseline. the identifier could even be the class name i think.

@LuigiCardamone
Copy link
Contributor

Yes, the class name of the rule (e.g. DependsOnlyOnTheseNamespaces) can be a good starting point.

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

No branches or pull requests

2 participants