Refacto

VincentLanglet · Jan 10, 2024 · 2a2da6b · 2a2da6b
1 parent 7d6a135
commit 2a2da6b
Show file tree

Hide file tree

Showing 18 changed files with 363 additions and 79 deletions.
diff --git a/src/Command/TwigCsFixerCommand.php b/src/Command/TwigCsFixerCommand.php
@@ -70,6 +70,12 @@ protected function configure(): void
                     InputOption::VALUE_NONE,
                     'Disable cache while running the fixer'
                 ),
+                new InputOption(
+                    'debug',
+                    '',
+                    InputOption::VALUE_NONE,
+                    'Display more technical informations',
+                ),
             ])
         ;
     }
@@ -129,7 +135,7 @@ private function runLinter(Config $config, InputInterface $input, OutputInterfac
 
         $reporterFactory = new ReporterFactory();
         $reporter = $reporterFactory->getReporter($input->getOption('report'));
-        $reporter->display($output, $report, $input->getOption('level'));
+        $reporter->display($output, $report, $input->getOption('level'), $input->getOption('debug'));
 
         return $report;
     }

diff --git a/src/Report/Reporter/CheckstyleReporter.php b/src/Report/Reporter/CheckstyleReporter.php
@@ -12,8 +12,12 @@ final class CheckstyleReporter implements ReporterInterface
 {
     public const NAME = 'checkstyle';
 
-    public function display(OutputInterface $output, Report $report, ?string $level = null): void
-    {
+    public function display(
+        OutputInterface $output,
+        Report $report,
+        ?string $level,
+        bool $debug
+    ): void {
         $text = '<?xml version="1.0" encoding="UTF-8"?>'."\n";
 
         $text .= '<checkstyle>'."\n";
@@ -38,7 +42,7 @@ public function display(OutputInterface $output, Report $report, ?string $level
                     $text .= ' column="'.$linePosition.'"';
                 }
                 $text .= ' severity="'.strtolower(Violation::getLevelAsString($violation->getLevel())).'"';
-                $text .= ' message="'.$this->xmlEncode($violation->getMessage()).'"';
+                $text .= ' message="'.$this->xmlEncode($violation->getDebugMessage($debug)).'"';
                 if (null !== $ruleName) {
                     $text .= ' source="'.$ruleName.'"';
                 }

diff --git a/src/Report/Reporter/GithubReporter.php b/src/Report/Reporter/GithubReporter.php
@@ -17,8 +17,12 @@ final class GithubReporter implements ReporterInterface
 {
     public const NAME = 'github';
 
-    public function display(OutputInterface $output, Report $report, ?string $level = null): void
-    {
+    public function display(
+        OutputInterface $output,
+        Report $report,
+        ?string $level,
+        bool $debug
+    ): void {
         $violations = $report->getViolations($level);
         foreach ($violations as $violation) {
             $text = match ($violation->getLevel()) {
@@ -40,7 +44,7 @@ public function display(OutputInterface $output, Report $report, ?string $level
 
             // newlines need to be encoded
             // see https://github.com/actions/starter-workflows/issues/68#issuecomment-581479448
-            $text .= '::'.str_replace("\n", '%0A', $violation->getMessage());
+            $text .= '::'.str_replace("\n", '%0A', $violation->getDebugMessage($debug));
 
             $output->writeln($text);
         }

diff --git a/src/Report/Reporter/JUnitReporter.php b/src/Report/Reporter/JUnitReporter.php
@@ -12,8 +12,12 @@ final class JUnitReporter implements ReporterInterface
 {
     public const NAME = 'junit';
 
-    public function display(OutputInterface $output, Report $report, ?string $level = null): void
-    {
+    public function display(
+        OutputInterface $output,
+        Report $report,
+        ?string $level,
+        bool $debug
+    ): void {
         $violations = $report->getViolations($level);
         $count = \count($violations);
 
@@ -30,7 +34,7 @@ public function display(OutputInterface $output, Report $report, ?string $level
                 $text .= $this->createTestCase(
                     sprintf('%s:%s', $violation->getFilename(), $violation->getLine() ?? 0),
                     strtolower(Violation::getLevelAsString($violation->getLevel())),
-                    $violation->getMessage()
+                    $violation->getDebugMessage($debug)
                 );
             }
         } else {

diff --git a/src/Report/Reporter/NullReporter.php b/src/Report/Reporter/NullReporter.php
@@ -14,7 +14,11 @@ final class NullReporter implements ReporterInterface
 {
     public const NAME = 'null';
 
-    public function display(OutputInterface $output, Report $report, ?string $level = null): void
-    {
+    public function display(
+        OutputInterface $output,
+        Report $report,
+        ?string $level,
+        bool $debug
+    ): void {
     }
 }
diff --git a/src/Report/Reporter/ReporterInterface.php b/src/Report/Reporter/ReporterInterface.php
@@ -9,5 +9,10 @@
 
 interface ReporterInterface
 {
-    public function display(OutputInterface $output, Report $report, ?string $level = null): void;
+    public function display(
+        OutputInterface $output,
+        Report $report,
+        ?string $level,
+        bool $debug
+    ): void;
 }
diff --git a/src/Report/Reporter/TextReporter.php b/src/Report/Reporter/TextReporter.php
@@ -23,8 +23,12 @@ final class TextReporter implements ReporterInterface
     private const ERROR_LINE_FORMAT = '%-5s| %s';
     private const ERROR_LINE_WIDTH = 120;
 
-    public function display(OutputInterface $output, Report $report, ?string $level = null): void
-    {
+    public function display(
+        OutputInterface $output,
+        Report $report,
+        ?string $level,
+        bool $debug
+    ): void {
         $io = new SymfonyStyle(new ArrayInput([]), $output);
 
         if (
@@ -48,7 +52,7 @@ public function display(OutputInterface $output, Report $report, ?string $level
                 $line = $violation->getLine();
 
                 if (null === $line || false === $content) {
-                    $formattedText[] = $this->formatErrorMessage($violation);
+                    $formattedText[] = $this->formatErrorMessage($violation, $debug);
                 } else {
                     $lines = $this->getContext($content, $line);
                     foreach ($lines as $no => $code) {
@@ -59,7 +63,7 @@ public function display(OutputInterface $output, Report $report, ?string $level
                         );
 
                         if ($no === $violation->getLine()) {
-                            $formattedText[] = $this->formatErrorMessage($violation);
+                            $formattedText[] = $this->formatErrorMessage($violation, $debug);
                         }
                     }
                 }
@@ -119,12 +123,12 @@ private function getContext(string $template, int $line): array
         return array_map(fn (string $code): string => substr($code, min($indents)), $result);
     }
 
-    private function formatErrorMessage(Violation $message): string
+    private function formatErrorMessage(Violation $message, bool $debug): string
     {
         return sprintf(
             sprintf('<fg=red>%s</fg=red>', self::ERROR_LINE_FORMAT),
             self::ERROR_CURSOR_CHAR,
-            wordwrap($message->getMessage(), self::ERROR_LINE_WIDTH)
+            wordwrap($message->getDebugMessage($debug), self::ERROR_LINE_WIDTH)
         );
     }
 }
diff --git a/src/Report/Violation.php b/src/Report/Violation.php
@@ -20,8 +20,6 @@ public function __construct(
         private int $level,
         private string $message,
         private string $filename,
-        private ?int $line = null,
-        private ?int $linePosition = null,
         private ?string $ruleName = null,
         private ?ViolationId $identifier = null,
     ) {
@@ -63,21 +61,20 @@ public function getMessage(): string
         return $this->message;
     }
 
-    public function getLine(): ?int
+    public function getDebugMessage(bool $debug): string
     {
-        return $this->line;
+        if (!$debug) {
+            return $this->message;
+        }
+
+        return $this->identifier?->toString() ?? $this->message;
     }
 
     public function getFilename(): string
     {
         return $this->filename;
     }
 
-    public function getLinePosition(): ?int
-    {
-        return $this->linePosition;
-    }
-
     public function getRuleName(): ?string
     {
         return $this->ruleName;
@@ -87,4 +84,14 @@ public function getIdentifier(): ?ViolationId
     {
         return $this->identifier;
     }
+
+    public function getLine(): ?int
+    {
+        return $this->identifier?->getLine();
+    }
+
+    public function getLinePosition(): ?int
+    {
+        return $this->identifier?->getLinePosition();
+    }
 }
diff --git a/src/Report/ViolationId.php b/src/Report/ViolationId.php
@@ -7,13 +7,23 @@
 final class ViolationId
 {
     public function __construct(
-        private ?string $ruleShortName = null,
-        private ?string $identifier = null,
+        private ?string $ruleIdentifier = null,
+        private ?string $messageIdentifier = null,
         private ?int $line = null,
         private ?int $linePosition = null,
     ) {
     }
 
+    public function getLine(): ?int
+    {
+        return $this->line;
+    }
+
+    public function getLinePosition(): ?int
+    {
+        return $this->linePosition;
+    }
+
     public static function fromString(string $string, ?int $line = null): self
     {
         $exploded = explode(':', $string);
@@ -35,8 +45,8 @@ public function toString(): string
     {
         $name = rtrim(sprintf(
             '%s.%s',
-            $this->ruleShortName ?? '',
-            $this->identifier ?? '',
+            $this->ruleIdentifier ?? '',
+            $this->messageIdentifier ?? '',
         ), '.');
 
         return rtrim(sprintf(
@@ -49,8 +59,8 @@ public function toString(): string
 
     public function match(self $violationId): bool
     {
-        return $this->matchValue($this->ruleShortName, $violationId->ruleShortName)
-            && $this->matchValue($this->identifier, $violationId->identifier)
+        return $this->matchValue($this->ruleIdentifier, $violationId->ruleIdentifier)
+            && $this->matchValue($this->messageIdentifier, $violationId->messageIdentifier)
             && $this->matchValue($this->line, $violationId->line)
             && $this->matchValue($this->linePosition, $violationId->linePosition);
     }

diff --git a/src/Rules/AbstractRule.php b/src/Rules/AbstractRule.php
@@ -172,10 +172,8 @@ private function addMessage(int $messageType, string $message, Token $token, ?st
                 $messageType,
                 $message,
                 $token->getFilename(),
-                $token->getLine(),
-                $token->getPosition(),
                 $this->getName(),
-                $id
+                $id,
             );
 
             $report->addViolation($violation);

diff --git a/src/Runner/Linter.php b/src/Runner/Linter.php
@@ -14,6 +14,7 @@
 use TwigCsFixer\Exception\CannotTokenizeException;
 use TwigCsFixer\Report\Report;
 use TwigCsFixer\Report\Violation;
+use TwigCsFixer\Report\ViolationId;
 use TwigCsFixer\Ruleset\Ruleset;
 use TwigCsFixer\Token\TokenizerInterface;
 
@@ -68,7 +69,8 @@ public function run(iterable $files, Ruleset $ruleset, ?FixerInterface $fixer =
                     Violation::LEVEL_FATAL,
                     sprintf('File is invalid: %s', $error->getRawMessage()),
                     $filePath,
-                    $error->getTemplateLine()
+                    null,
+                    new ViolationId(line: $error->getTemplateLine())
                 );
 
                 $report->addViolation($violation);

diff --git a/tests/Command/TwigCsFixerCommandTest.php b/tests/Command/TwigCsFixerCommandTest.php
@@ -61,6 +61,28 @@ public function testExecuteWithReportErrors(): void
         $display = $commandTester->getDisplay();
         static::assertStringContainsString('directory/subdirectory/file.twig', $display);
         static::assertStringContainsString('directory/file.twig', $display);
+        static::assertStringNotContainsString('DelimiterSpacing.After', $display);
+        static::assertStringContainsString(
+            '[ERROR] Files linted: 3, notices: 0, warnings: 0, errors: 3',
+            $display
+        );
+        static::assertSame(Command::FAILURE, $commandTester->getStatusCode());
+    }
+
+    public function testExecuteWithReportErrorsAndDebug(): void
+    {
+        $command = new TwigCsFixerCommand();
+
+        $commandTester = new CommandTester($command);
+        $commandTester->execute([
+            'paths'   => [$this->getTmpPath(__DIR__.'/Fixtures')],
+            '--debug' => true,
+        ]);
+
+        $display = $commandTester->getDisplay();
+        static::assertStringContainsString('directory/subdirectory/file.twig', $display);
+        static::assertStringContainsString('directory/file.twig', $display);
+        static::assertStringContainsString('DelimiterSpacing.After', $display);
         static::assertStringContainsString(
             '[ERROR] Files linted: 3, notices: 0, warnings: 0, errors: 3',
             $display
@@ -152,7 +174,7 @@ public function testExecuteWithError(): void
             '--config' => $this->getTmpPath(__DIR__.'/Fixtures/.config-not-found.php'),
         ]);
 
-        static::assertStringStartsWith('Error: ', $commandTester->getDisplay());
+        static::assertStringStartsWith('Error: Cannot find the config file', $commandTester->getDisplay());
         static::assertSame(Command::INVALID, $commandTester->getStatusCode());
     }