Fix #126 improve error messages including classname when possible

[annervisser]

• Added ViolationDescription, as input for a Violation
• Added a description to the ViolationDescription
  • Only for Expressions where it made sense

[annervisser]

I was trying out Arkitect, and it looks like a great tool 😄. The php config is great, so much better than Yaml.

I bumped into these messages not being quite as descriptive as I'd like. So I picked up this issue.

Let me know if this is something you'd like to merge and if anything needs to be changed 👍

[AlessandroMinoccheri]

Hi @annervisser, thanks for this PR.
Before starting the review, can you please fix the coding standards 😄?
Thanks.

[annervisser]

@AlessandroMinoccheri done!

[annervisser]

• reverted the rename of Description -> ExpressionDescription
• moved ViolationDescription from Arkitect\Expression -> Arkitect\Rules (to be next to Violation)
• renamed ViolationDescription to ViolationMessage to avoid duplicating the term description in classname and method

[fain182]

Thank you @annervisser for your PR! I think that is absolutely necessary to explicit which dependency is raising the error.

I am not sure about the wording:

App\Domain\Model violates rules
depends on App\Services\UserService, but should not depend on classes outside namespace App\Domain because we want protect our domain (on line 14)

It seems that "depends on..." is part of the rule, but it's the opposite, it's the violation 🤔

Maybe we could write something like this? Other ideas?

App\Domain\Model violates rules
should not depend on classes outside namespace App\Domain because we want protect our domain (it depends on App\Services\UserService on line 14)

[annervisser]

@fain182 I agree the wording is a bit weird right now.

I would be okay with your suggestion of putting it at the end with the line number

An alternative would be to change the wording to make it clear that they're violations (change <fqcn> violates rules to for example <fqcn> has <n> violations
Maybe we could even group the messages per violation? like this: (though this is probably out of scope for this PR)

App\Domain\Model has 2 violations:
  rule: should not depend on classes outside namespace App\Domain because we want protect our domain
    - App\Domain\Model depends on App\Services\UserService (on line 14)
    - App\Domain\Model depends on App\Services\OtherService (on line 26)

As you said the current wording implies we're outputting rules, despite actually outputting violations. Currently this is already noticeable through the fact that a rule can be listed multiple times per file (when a rule has multiple violations)

What do you think?
My suggestion would be to change the wording to make it clear we're listing violations rather than rules, and follow up in a future PR to group violations per rule

[fain182]

I agree with your idea @annervisser, we are showing violations, not rules right now 👍

[annervisser]

@fain182 done in c9b3be0 (#281)

[annervisser]

This is why you write tests 🙈
Updated the tests and realized I implemented it wrong, fixed now

[codecov-commenter]

Codecov Report

Merging #281 (6817886) into main (db9c74b) will increase coverage by 0.11%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##               main     #281      +/-   ##
============================================
+ Coverage     93.62%   93.73%   +0.11%     
- Complexity      370      376       +6     
============================================
  Files            57       58       +1     
  Lines           988     1006      +18     
============================================
+ Hits            925      943      +18     
  Misses           63       63              

Impacted Files	Coverage Δ
...ession/ForClasses/DependsOnlyOnTheseNamespaces.php	100.00% <100.00%> (ø)
src/Expression/ForClasses/DocBlockContains.php	100.00% <100.00%> (ø)
src/Expression/ForClasses/DocBlockNotContains.php	100.00% <100.00%> (ø)
src/Expression/ForClasses/Extend.php	91.66% <100.00%> (ø)
src/Expression/ForClasses/HaveAttribute.php	100.00% <100.00%> (ø)
src/Expression/ForClasses/HaveNameMatching.php	100.00% <100.00%> (ø)
src/Expression/ForClasses/Implement.php	100.00% <100.00%> (ø)
src/Expression/ForClasses/IsAbstract.php	100.00% <100.00%> (ø)
src/Expression/ForClasses/IsFinal.php	100.00% <100.00%> (ø)
src/Expression/ForClasses/IsNotAbstract.php	100.00% <100.00%> (ø)
... and 11 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[AlessandroMinoccheri]

LGTM 👍