Fix #126 improve error messages including classname when possible

Added ViolationDescription, as input for a Violation Added a description to the ViolationDescription - Only for Expressions where it made sense
phparkitect · Aug 16, 2022 · 78b739b · 78b739b
1 parent c76a3cf
commit 78b739b
Show file tree

Hide file tree

Showing 26 changed files with 133 additions and 36 deletions.
diff --git a/src/Expression/ForClasses/DependsOnlyOnTheseNamespaces.php b/src/Expression/ForClasses/DependsOnlyOnTheseNamespaces.php
@@ -9,6 +9,7 @@
 use Arkitect\Expression\Description;
 use Arkitect\Expression\Expression;
 use Arkitect\Rules\Violation;
+use Arkitect\Rules\ViolationMessage;
 use Arkitect\Rules\Violations;
 
 class DependsOnlyOnTheseNamespaces implements Expression
@@ -44,7 +45,10 @@ public function evaluate(ClassDescription $theClass, Violations $violations, str
             if (!$dependency->matchesOneOf(...$this->namespaces)) {
                 $violation = Violation::createWithErrorLine(
                     $theClass->getFQCN(),
-                    $this->describe($theClass, $because)->toString(),
+                    ViolationMessage::withDescription(
+                        $this->describe($theClass, $because),
+                        "depends on {$dependency->getFQCN()->toString()}"
+                    ),
                     $dependency->getLine()
                 );
 

diff --git a/src/Expression/ForClasses/DocBlockContains.php b/src/Expression/ForClasses/DocBlockContains.php
@@ -8,6 +8,7 @@
 use Arkitect\Expression\Description;
 use Arkitect\Expression\Expression;
 use Arkitect\Rules\Violation;
+use Arkitect\Rules\ViolationMessage;
 use Arkitect\Rules\Violations;
 
 class DocBlockContains implements Expression
@@ -30,7 +31,7 @@ public function evaluate(ClassDescription $theClass, Violations $violations, str
         if (!$theClass->containsDocBlock($this->docBlock)) {
             $violation = Violation::create(
                 $theClass->getFQCN(),
-                $this->describe($theClass, $because)->toString()
+                ViolationMessage::selfExplanatory($this->describe($theClass, $because))
             );
             $violations->add($violation);
         }

diff --git a/src/Expression/ForClasses/DocBlockNotContains.php b/src/Expression/ForClasses/DocBlockNotContains.php
@@ -8,6 +8,7 @@
 use Arkitect\Expression\Description;
 use Arkitect\Expression\Expression;
 use Arkitect\Rules\Violation;
+use Arkitect\Rules\ViolationMessage;
 use Arkitect\Rules\Violations;
 
 class DocBlockNotContains implements Expression
@@ -30,7 +31,7 @@ public function evaluate(ClassDescription $theClass, Violations $violations, str
         if ($theClass->containsDocBlock($this->docBlock)) {
             $violation = Violation::create(
                 $theClass->getFQCN(),
-                $this->describe($theClass, $because)->toString()
+                ViolationMessage::selfExplanatory($this->describe($theClass, $because))
             );
             $violations->add($violation);
         }

diff --git a/src/Expression/ForClasses/Extend.php b/src/Expression/ForClasses/Extend.php
@@ -8,6 +8,7 @@
 use Arkitect\Expression\Description;
 use Arkitect\Expression\Expression;
 use Arkitect\Rules\Violation;
+use Arkitect\Rules\ViolationMessage;
 use Arkitect\Rules\Violations;
 
 class Extend implements Expression
@@ -35,7 +36,7 @@ public function evaluate(ClassDescription $theClass, Violations $violations, str
 
         $violation = Violation::create(
             $theClass->getFQCN(),
-            $this->describe($theClass, $because)->toString()
+            ViolationMessage::selfExplanatory($this->describe($theClass, $because))
         );
 
         $violations->add($violation);

diff --git a/src/Expression/ForClasses/HaveAttribute.php b/src/Expression/ForClasses/HaveAttribute.php
@@ -7,6 +7,7 @@
 use Arkitect\Expression\Description;
 use Arkitect\Expression\Expression;
 use Arkitect\Rules\Violation;
+use Arkitect\Rules\ViolationMessage;
 use Arkitect\Rules\Violations;
 
 final class HaveAttribute implements Expression
@@ -33,7 +34,7 @@ public function evaluate(ClassDescription $theClass, Violations $violations, str
         $violations->add(
             Violation::create(
                 $theClass->getFQCN(),
-                $this->describe($theClass, $because)->toString()
+                ViolationMessage::selfExplanatory($this->describe($theClass, $because))
             )
         );
     }

diff --git a/src/Expression/ForClasses/HaveNameMatching.php b/src/Expression/ForClasses/HaveNameMatching.php
@@ -9,6 +9,7 @@
 use Arkitect\Expression\Description;
 use Arkitect\Expression\Expression;
 use Arkitect\Rules\Violation;
+use Arkitect\Rules\ViolationMessage;
 use Arkitect\Rules\Violations;
 
 class HaveNameMatching implements Expression
@@ -32,7 +33,7 @@ public function evaluate(ClassDescription $theClass, Violations $violations, str
         if (!$fqcn->classMatches($this->name)) {
             $violation = Violation::create(
                 $theClass->getFQCN(),
-                $this->describe($theClass, $because)->toString()
+                ViolationMessage::selfExplanatory($this->describe($theClass, $because))
             );
             $violations->add($violation);
         }

diff --git a/src/Expression/ForClasses/Implement.php b/src/Expression/ForClasses/Implement.php
@@ -9,6 +9,7 @@
 use Arkitect\Expression\Description;
 use Arkitect\Expression\Expression;
 use Arkitect\Rules\Violation;
+use Arkitect\Rules\ViolationMessage;
 use Arkitect\Rules\Violations;
 
 class Implement implements Expression
@@ -37,7 +38,7 @@ public function evaluate(ClassDescription $theClass, Violations $violations, str
         if (0 === \count(array_filter($interfaces, $implements))) {
             $violation = Violation::create(
                 $theClass->getFQCN(),
-                $this->describe($theClass, $because)->toString()
+                ViolationMessage::selfExplanatory($this->describe($theClass, $because))
             );
             $violations->add($violation);
         }

diff --git a/src/Expression/ForClasses/IsAbstract.php b/src/Expression/ForClasses/IsAbstract.php
@@ -8,6 +8,7 @@
 use Arkitect\Expression\Description;
 use Arkitect\Expression\Expression;
 use Arkitect\Rules\Violation;
+use Arkitect\Rules\ViolationMessage;
 use Arkitect\Rules\Violations;
 
 class IsAbstract implements Expression
@@ -25,7 +26,7 @@ public function evaluate(ClassDescription $theClass, Violations $violations, str
 
         $violation = Violation::create(
             $theClass->getFQCN(),
-            $this->describe($theClass, $because)->toString()
+            ViolationMessage::selfExplanatory($this->describe($theClass, $because))
         );
 
         $violations->add($violation);

diff --git a/src/Expression/ForClasses/IsFinal.php b/src/Expression/ForClasses/IsFinal.php
@@ -8,6 +8,7 @@
 use Arkitect\Expression\Description;
 use Arkitect\Expression\Expression;
 use Arkitect\Rules\Violation;
+use Arkitect\Rules\ViolationMessage;
 use Arkitect\Rules\Violations;
 
 class IsFinal implements Expression
@@ -25,7 +26,7 @@ public function evaluate(ClassDescription $theClass, Violations $violations, str
 
         $violation = Violation::create(
             $theClass->getFQCN(),
-            $this->describe($theClass, $because)->toString()
+            ViolationMessage::selfExplanatory($this->describe($theClass, $because))
         );
 
         $violations->add($violation);

diff --git a/src/Expression/ForClasses/IsNotAbstract.php b/src/Expression/ForClasses/IsNotAbstract.php
@@ -8,6 +8,7 @@
 use Arkitect\Expression\Description;
 use Arkitect\Expression\Expression;
 use Arkitect\Rules\Violation;
+use Arkitect\Rules\ViolationMessage;
 use Arkitect\Rules\Violations;
 
 class IsNotAbstract implements Expression
@@ -25,7 +26,7 @@ public function evaluate(ClassDescription $theClass, Violations $violations, str
 
         $violation = Violation::create(
             $theClass->getFQCN(),
-            $this->describe($theClass, $because)->toString()
+            ViolationMessage::selfExplanatory($this->describe($theClass, $because))
         );
 
         $violations->add($violation);

diff --git a/src/Expression/ForClasses/IsNotFinal.php b/src/Expression/ForClasses/IsNotFinal.php
@@ -8,6 +8,7 @@
 use Arkitect\Expression\Description;
 use Arkitect\Expression\Expression;
 use Arkitect\Rules\Violation;
+use Arkitect\Rules\ViolationMessage;
 use Arkitect\Rules\Violations;
 
 class IsNotFinal implements Expression
@@ -25,7 +26,7 @@ public function evaluate(ClassDescription $theClass, Violations $violations, str
 
         $violation = Violation::create(
             $theClass->getFQCN(),
-            $this->describe($theClass, $because)->toString()
+            ViolationMessage::selfExplanatory($this->describe($theClass, $because))
         );
 
         $violations->add($violation);

diff --git a/src/Expression/ForClasses/NotDependsOnTheseNamespaces.php b/src/Expression/ForClasses/NotDependsOnTheseNamespaces.php
@@ -9,6 +9,7 @@
 use Arkitect\Expression\Description;
 use Arkitect\Expression\Expression;
 use Arkitect\Rules\Violation;
+use Arkitect\Rules\ViolationMessage;
 use Arkitect\Rules\Violations;
 
 class NotDependsOnTheseNamespaces implements Expression
@@ -41,7 +42,10 @@ public function evaluate(ClassDescription $theClass, Violations $violations, str
             if ($dependency->matchesOneOf(...$this->namespaces)) {
                 $violation = Violation::createWithErrorLine(
                     $theClass->getFQCN(),
-                    $this->describe($theClass, $because)->toString(),
+                    ViolationMessage::withDescription(
+                        $this->describe($theClass, $because),
+                        "depends on {$dependency->getFQCN()->toString()}"
+                    ),
                     $dependency->getLine()
                 );
 

diff --git a/src/Expression/ForClasses/NotExtend.php b/src/Expression/ForClasses/NotExtend.php
@@ -8,6 +8,7 @@
 use Arkitect\Expression\Description;
 use Arkitect\Expression\Expression;
 use Arkitect\Rules\Violation;
+use Arkitect\Rules\ViolationMessage;
 use Arkitect\Rules\Violations;
 
 class NotExtend implements Expression
@@ -39,7 +40,7 @@ public function evaluate(ClassDescription $theClass, Violations $violations, str
 
         $violation = Violation::create(
             $theClass->getFQCN(),
-            $this->describe($theClass, $because)->toString()
+            ViolationMessage::selfExplanatory($this->describe($theClass, $because))
         );
 
         $violations->add($violation);

diff --git a/src/Expression/ForClasses/NotHaveDependencyOutsideNamespace.php b/src/Expression/ForClasses/NotHaveDependencyOutsideNamespace.php
@@ -9,6 +9,7 @@
 use Arkitect\Expression\Description;
 use Arkitect\Expression\Expression;
 use Arkitect\Rules\Violation;
+use Arkitect\Rules\ViolationMessage;
 use Arkitect\Rules\Violations;
 
 class NotHaveDependencyOutsideNamespace implements Expression
@@ -47,7 +48,10 @@ public function evaluate(ClassDescription $theClass, Violations $violations, str
 
             $violation = Violation::createWithErrorLine(
                 $theClass->getFQCN(),
-                $this->describe($theClass, $because)->toString(),
+                ViolationMessage::withDescription(
+                    $this->describe($theClass, $because),
+                    "depends on {$externalDep->getFQCN()->toString()}"
+                ),
                 $externalDep->getLine()
             );
             $violations->add($violation);

diff --git a/src/Expression/ForClasses/NotHaveNameMatching.php b/src/Expression/ForClasses/NotHaveNameMatching.php
@@ -9,6 +9,7 @@
 use Arkitect\Expression\Description;
 use Arkitect\Expression\Expression;
 use Arkitect\Rules\Violation;
+use Arkitect\Rules\ViolationMessage;
 use Arkitect\Rules\Violations;
 
 class NotHaveNameMatching implements Expression
@@ -32,7 +33,7 @@ public function evaluate(ClassDescription $theClass, Violations $violations, str
         if ($fqcn->classMatches($this->name)) {
             $violation = Violation::create(
                 $theClass->getFQCN(),
-                $this->describe($theClass, $because)->toString()
+                ViolationMessage::selfExplanatory($this->describe($theClass, $because))
             );
             $violations->add($violation);
         }

diff --git a/src/Expression/ForClasses/NotImplement.php b/src/Expression/ForClasses/NotImplement.php
@@ -9,6 +9,7 @@
 use Arkitect\Expression\Description;
 use Arkitect\Expression\Expression;
 use Arkitect\Rules\Violation;
+use Arkitect\Rules\ViolationMessage;
 use Arkitect\Rules\Violations;
 
 class NotImplement implements Expression
@@ -37,7 +38,7 @@ public function evaluate(ClassDescription $theClass, Violations $violations, str
         if (\count(array_filter($interfaces, $implements)) > 0) {
             $violation = Violation::create(
                 $theClass->getFQCN(),
-                $this->describe($theClass, $because)->toString()
+                ViolationMessage::selfExplanatory($this->describe($theClass, $because))
             );
             $violations->add($violation);
         }

diff --git a/src/Expression/ForClasses/NotResideInTheseNamespaces.php b/src/Expression/ForClasses/NotResideInTheseNamespaces.php
@@ -8,6 +8,7 @@
 use Arkitect\Expression\Description;
 use Arkitect\Expression\Expression;
 use Arkitect\Rules\Violation;
+use Arkitect\Rules\ViolationMessage;
 use Arkitect\Rules\Violations;
 
 class NotResideInTheseNamespaces implements Expression
@@ -39,7 +40,7 @@ public function evaluate(ClassDescription $theClass, Violations $violations, str
         if ($resideInNamespace) {
             $violation = Violation::create(
                 $theClass->getFQCN(),
-                $this->describe($theClass, $because)->toString()
+                ViolationMessage::selfExplanatory($this->describe($theClass, $because))
             );
             $violations->add($violation);
         }

diff --git a/src/Expression/ForClasses/ResideInOneOfTheseNamespaces.php b/src/Expression/ForClasses/ResideInOneOfTheseNamespaces.php
@@ -8,6 +8,7 @@
 use Arkitect\Expression\Description;
 use Arkitect\Expression\Expression;
 use Arkitect\Rules\Violation;
+use Arkitect\Rules\ViolationMessage;
 use Arkitect\Rules\Violations;
 
 class ResideInOneOfTheseNamespaces implements Expression
@@ -39,7 +40,7 @@ public function evaluate(ClassDescription $theClass, Violations $violations, str
         if (!$resideInNamespace) {
             $violation = Violation::create(
                 $theClass->getFQCN(),
-                $this->describe($theClass, $because)->toString()
+                ViolationMessage::selfExplanatory($this->describe($theClass, $because))
             );
             $violations->add($violation);
         }

diff --git a/src/Rules/Violation.php b/src/Rules/Violation.php
@@ -22,14 +22,14 @@ public function __construct(string $fqcn, string $error, ?int $line = null)
         $this->line = $line;
     }
 
-    public static function create(string $fqcn, string $error): self
+    public static function create(string $fqcn, ViolationMessage $error): self
     {
-        return new self($fqcn, $error);
+        return new self($fqcn, $error->toString());
     }
 
-    public static function createWithErrorLine(string $fqcn, string $error, int $line): self
+    public static function createWithErrorLine(string $fqcn, ViolationMessage $error, int $line): self
     {
-        return new self($fqcn, $error, $line);
+        return new self($fqcn, $error->toString(), $line);
     }
 
     public function getFqcn(): string

diff --git a/src/Rules/ViolationMessage.php b/src/Rules/ViolationMessage.php
@@ -0,0 +1,40 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Arkitect\Rules;
+
+use Arkitect\Expression\Description;
+
+class ViolationMessage
+{
+    /** @var string */
+    private $rule;
+    /** @var string|null */
+    private $violation;
+
+    private function __construct(string $rule, ?string $violation)
+    {
+        $this->rule = $rule;
+        $this->violation = $violation;
+    }
+
+    public static function withDescription(Description $brokenRule, string $description): self
+    {
+        return new self($brokenRule->toString(), $description);
+    }
+
+    public static function selfExplanatory(Description $brokenRule): self
+    {
+        return new self($brokenRule->toString(), null);
+    }
+
+    public function toString(): string
+    {
+        if (null === $this->violation) {
+            return $this->rule;
+        }
+
+        return "$this->violation, but $this->rule";
+    }
+}
diff --git a/tests/E2E/Cli/CheckCommandTest.php b/tests/E2E/Cli/CheckCommandTest.php
@@ -35,8 +35,8 @@ public function test_app_returns_error_with_multiple_violations(): void
   should implement ContainerAwareInterface because all controllers should be container aware
 
 App\Domain\Model violates rules
-  should not depend on classes outside namespace App\Domain because we want protect our domain (on line 14)
-  should not depend on classes outside namespace App\Domain because we want protect our domain (on line 15)';
+  depends on App\Services\UserService, but should not depend on classes outside namespace App\Domain because we want protect our domain (on line 14)
+  depends on App\Services\CartService, but should not depend on classes outside namespace App\Domain because we want protect our domain (on line 15)';
 
         $this->assertCheckHasErrors($cmdTester, $expectedErrors);
     }