Enhancement: skip_violations for traits #624
Comments
|
I actually like the current behavior better. But it is a matter of opinion. |
|
I cannot image a case where that would be helpful: you want to exempt the trait but not a class implementing the trait? I guess I'm unfamiliar with how traits are handled during collection - do they count both for themselves and their implementing class? |
|
Yes, they do. For implementing class:I always think of traits as copy-paste into the implementing class. For that reason, I would like to know every class that is not conforming to the set rules. I can imagine cases when some of the classes implementing are allowed by the ruleset, but others are not. In such a scenario, only some of the uses will be violations. I want to see them. For themselvesI also would like to know when I am writing the trait itself, that it is by design not conforming to the defined ruleset. Both pieces of information are useful to me to know. |
|
Hmm troublesome, but I see your point. Maybe allowing wildcards/regex in |
|
That is something people would expect to see dealing with tools like PHPStan and something I could get behind. |
|
I am not too familiar with CodeIgniter, especially in the current generation, but to me these violations are indicators of an actual architectural "flaw", be it classes in the wrong layer, missing abstraction, or something similar. If HTTP and Pager should be independent, then HTTP should not rely on that interface in so many places, not even indirectly via the trait. In fact this is exactly what I like about Deptrac, it makes it visible that both layers are starting to merge and you are missing some isolation between them, a single connection point, e.g. an adapter. If you were to inverse the control and move the logic in the trait to the pager component, then you would only have 1 connection point between the two layers, which is probably what you want. Additionally, using another layer of indirection, e.g. a separate bridge/adapter you could make both layers independent, i.e. you move the violation out to a new point, so both layers do not know about each other. Basically, your violations would probably go away if you followed the todo comment in the ResponseTrait 😉 Another way of getting rid of the violations would be for public function setLink(LinkedResources $linkedResources)
{
$links = [];
foreach($linkedResources as $resource) {
$link = sprintf('<%s>; rel="%s"', $resource->getUri(), $resource->getRel());
if ($resource->hasTitle()) {
$link .= sprintf('; title="%s"', $resource->getTitle());
}
$links[] = $link;
}
$this->setHeader('Link', implode(',', $links));
return $this;
}Not only do you no longer have a violation, the code is now more generalized because anything with a uri (and rel) can now be linked. At the same time, the only place where both layers meet is where the PagerInterface is converted into a I understand that rewriting the logic especially in a framework where you have to be careful about BC and how it is used by applications is not a trivial task, so you probably want to skip the violations until this refactoring can be done. For those cases I would still prefer for deptrac to explicitly warn about each separate violation and force users to mark each of them as skipped instead of providing additional logic, because it will tempt people to build fancy ways of ignoring deptrac instead of actually solving the violations it reports. When it comes to (many) existing violations, I think the BaselineFormatter is a good compromise to automate this step and I am hesitant to provide anything more than that. |
|
@dbrumann you for sure hit the nail on the head! And that note is correct, For now I have added every class to the list but I do still think wildcards would be helpful for circumstances like this. This is "Deptrac at its best": catching and improving existing architectures then enforcing them moving forward. Check out my PR for the framework here. |
|
If wildcards/regex help with introducing deptrac to an existing project I am open to it. My main concern is, that this will introduce some uncertainty which violations are skipped and which ones not, especially since we regularly have issues regarding regexes. For regexes in layers we have some debugging now that helps with this, but I am not sure I want to have that for violations as well. I would prefer to keep that section as simple as possible. |
|
Is there no way to display skipped violations? I kinda assumed I could see them somehow, with |
|
Yes, but only as part of a full analysis. |
|
This issue has not seen activity in a while and will be closed automatically soon. |
|
@github-actions pls no |
|
This issue has not seen activity in a while and will be closed automatically soon. |
|
Fine. |
Currently allowing a violation in a trait:
... will not apply to any of the classes that use the trait, requiring every class to apply the exemption as well.
The text was updated successfully, but these errors were encountered: