Merge pull request #148 from Nyholm/error

Adding an Error object
doctrine · May 16, 2021 · c4ac6f1 · c4ac6f1
2 parents 23a8c7a + 241b83a
commit c4ac6f1
Show file tree

Hide file tree

Showing 14 changed files with 147 additions and 69 deletions.
diff --git a/lib/Environment.php b/lib/Environment.php
@@ -11,7 +11,6 @@
 use Doctrine\RST\References\ResolvedReference;
 use Doctrine\RST\Templates\TemplateRenderer;
 use InvalidArgumentException;
-use Throwable;
 
 use function array_shift;
 use function dirname;
@@ -473,16 +472,6 @@ public function getTitleLetters(): array
         return $this->titleLetters;
     }
 
-    public function addError(string $message, ?Throwable $throwable = null): void
-    {
-        $this->errorManager->error($message, $throwable);
-    }
-
-    public function addWarning(string $message): void
-    {
-        $this->errorManager->warning($message);
-    }
-
     public static function slugify(string $text): string
     {
         // replace non letter or digits by -
@@ -508,10 +497,9 @@ public static function slugify(string $text): string
 
     private function addMissingReferenceSectionError(string $section): void
     {
-        $this->errorManager->error(sprintf(
-            'Unknown reference section "%s"%s',
-            $section,
-            $this->getCurrentFileName() !== '' ? sprintf(' in "%s" ', $this->getCurrentFileName()) : ''
-        ));
+        $this->errorManager->error(
+            sprintf('Unknown reference section "%s"', $section),
+            $this->getCurrentFileName()
+        );
     }
 }
diff --git a/lib/Error.php b/lib/Error.php
@@ -0,0 +1,70 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Doctrine\RST;
+
+use Throwable;
+
+use function sprintf;
+
+final class Error
+{
+    /** @var string */
+    private $message;
+
+    /** @var string|null */
+    private $file;
+
+    /** @var int|null */
+    private $line;
+
+    /** @var Throwable|null */
+    private $throwable;
+
+    public function __construct(string $message, ?string $file = null, ?int $line = null, ?Throwable $throwable = null)
+    {
+        $this->message   = $message;
+        $this->file      = $file;
+        $this->line      = $line;
+        $this->throwable = $throwable;
+    }
+
+    public function asString(): string
+    {
+        $output = $this->message;
+        if ($this->getFile() !== null) {
+            $output .= sprintf(' in file "%s"', $this->file);
+
+            if ($this->line !== null) {
+                $output .= sprintf(' at line %d', $this->line);
+            }
+        }
+
+        return $output;
+    }
+
+    public function getMessage(): string
+    {
+        return $this->message;
+    }
+
+    public function getFile(): ?string
+    {
+        if ($this->file === '') {
+            return null;
+        }
+
+        return $this->file;
+    }
+
+    public function getLine(): ?int
+    {
+        return $this->line;
+    }
+
+    public function getThrowable(): ?Throwable
+    {
+        return $this->throwable;
+    }
+}
diff --git a/lib/ErrorManager.php b/lib/ErrorManager.php
@@ -7,36 +7,48 @@
 use Exception;
 use Throwable;
 
+use function sprintf;
+
 class ErrorManager
 {
     /** @var Configuration */
     private $configuration;
 
-    /** @var string[] */
+    /** @var list<Error> */
     private $errors = [];
 
     public function __construct(Configuration $configuration)
     {
         $this->configuration = $configuration;
     }
 
-    public function error(string $message, ?Throwable $throwable = null): void
+    public function error(string $message, ?string $file = null, ?int $line = null, ?Throwable $throwable = null): void
     {
-        $this->errors[] = $message;
+        $this->errors[] = $error = new Error($message, $file, $line, $throwable);
 
         if (! $this->configuration->isSilentOnError()) {
-            echo '/!\\ ' . $message . "\n";
+            if ($this->configuration->getOutputFormat() === Configuration::OUTPUT_FORMAT_GITHUB) {
+                $file = $error->getFile();
+                echo sprintf(
+                    '::error %s%s::%s',
+                    $file !== null ? 'file=' . $file : '',
+                    $file !== null && $error->getLine() !== null ? ',linefile=' . $error->getLine() : '',
+                    $error->getMessage()
+                );
+            } else {
+                echo '⚠️ ' . $error->asString() . "\n";
+            }
         }
 
         if ($this->configuration->isAbortOnError()) {
-            throw new Exception($message, 0, $throwable);
+            throw new Exception($error->asString(), 0, $error->getThrowable());
         }
     }
 
-    public function warning(string $message): void
+    public function warning(string $message, ?string $file = null, ?int $line = null, ?Throwable $throwable = null): void
     {
         if ($this->configuration->isWarningsAsError()) {
-            $this->error($message);
+            $this->error($message, $file, $line, $throwable);
 
             return;
         }
@@ -45,11 +57,22 @@ public function warning(string $message): void
             return;
         }
 
-        echo $message . "\n";
+        $error = new Error($message, $file, $line, $throwable);
+        if ($this->configuration->getOutputFormat() === Configuration::OUTPUT_FORMAT_GITHUB) {
+            $file = $error->getFile();
+            echo sprintf(
+                '::warning %s%s::%s',
+                $file !== null ? 'file=' . $file : '',
+                $file !== null && $error->getLine() !== null ? ',linefile=' . $error->getLine() : '',
+                $error->getMessage()
+            );
+        } else {
+            echo $error->asString() . "\n";
+        }
     }
 
     /**
-     * @return string[]
+     * @return list<Error>
      */
     public function getErrors(): array
     {

diff --git a/lib/Nodes/DocumentNode.php b/lib/Nodes/DocumentNode.php
@@ -236,11 +236,10 @@ private function postRenderValidate(): void
         $currentFileName = $this->environment->getCurrentFileName();
 
         foreach ($this->environment->getInvalidLinks() as $invalidLink) {
-            $this->errorManager->error(sprintf(
-                'Found invalid reference "%s"%s',
-                $invalidLink->getName(),
-                $currentFileName !== '' ? sprintf(' in file "%s"', $currentFileName) : ''
-            ));
+            $this->errorManager->error(
+                sprintf('Found invalid reference "%s"', $invalidLink->getName()),
+                $currentFileName
+            );
         }
     }
 }
diff --git a/lib/Nodes/TableNode.php b/lib/Nodes/TableNode.php
@@ -153,7 +153,7 @@ public function finalize(Parser $parser): void
         if (count($this->errors) > 0) {
             $parser->getEnvironment()
                 ->getErrorManager()
-                ->error(sprintf("%s\nin file %s\n\n%s", $this->errors[0], $parser->getFilename(), $tableAsString));
+                ->error(sprintf("%s\n\n%s", $this->errors[0], $tableAsString), $parser->getFilename());
 
             $this->data    = [];
             $this->headers = [];

diff --git a/lib/Parser/DocumentParser.php b/lib/Parser/DocumentParser.php
@@ -318,11 +318,11 @@ private function parseLine(string $line): bool
             case State::LIST:
                 if (! $this->lineChecker->isListLine($line, $this->listMarker, $this->listOffset) && ! $this->lineChecker->isBlockLine($line, max(1, $this->listOffset))) {
                     if (trim($this->lines->getPreviousLine()) !== '') {
-                        $this->environment->addWarning(sprintf(
-                            'Warning%s%s: List ends without a blank line; unexpected unindent.',
-                            $this->environment->getCurrentFileName() !== '' ? sprintf(' in "%s"', $this->environment->getCurrentFileName()) : '',
-                            $this->currentLineNumber !== null ? ' around line ' . ($this->currentLineNumber - 1) : ''
-                        ));
+                        $this->environment->getErrorManager()->warning(
+                            'List ends without a blank line; unexpected unindent',
+                            $this->environment->getCurrentFileName(),
+                            $this->currentLineNumber !== null ? $this->currentLineNumber - 1 : null
+                        );
                     }
 
                     $this->flush();
@@ -455,7 +455,7 @@ private function parseLine(string $line): bool
                 break;
 
             default:
-                $this->environment->addError('Parser ended in an unexcepted state');
+                $this->environment->getErrorManager()->error('Parser ended in an unexcepted state');
         }
 
         return true;
@@ -589,15 +589,12 @@ private function flush(): void
                         $this->directive->getOptions()
                     );
                 } catch (Throwable $e) {
-                    $message = sprintf(
-                        'Error while processing "%s" directive%s%s: %s',
-                        $currentDirective->getName(),
-                        $this->environment->getCurrentFileName() !== '' ? sprintf(' in "%s"', $this->environment->getCurrentFileName()) : '',
-                        $this->currentLineNumber !== null ? ' around line ' . $this->currentLineNumber : '',
-                        $e->getMessage()
+                    $this->environment->getErrorManager()->error(
+                        sprintf('Error while processing "%s" directive: "%s"', $currentDirective->getName(), $e->getMessage()),
+                        $this->environment->getCurrentFileName(),
+                        $this->currentLineNumber ?? null,
+                        $e
                     );
-
-                    $this->environment->addError($message, $e);
                 }
             }
 
@@ -655,15 +652,11 @@ private function initDirective(string $line): bool
         }
 
         if (! isset($this->directives[$parserDirective->getName()])) {
-            $message = sprintf(
-                'Unknown directive: "%s" %sfor line "%s"',
-                $parserDirective->getName(),
-                $this->environment->getCurrentFileName() !== '' ? sprintf('in "%s" ', $this->environment->getCurrentFileName()) : '',
-                $line
+            $this->environment->getErrorManager()->error(
+                sprintf('Unknown directive "%s" for line "%s"', $parserDirective->getName(), $line),
+                $this->environment->getCurrentFileName()
             );
 
-            $this->environment->addError($message);
-
             return false;
         }
 

diff --git a/tests/BuilderWithErrors/BuilderWithErrorsTest.php b/tests/BuilderWithErrors/BuilderWithErrorsTest.php
@@ -32,8 +32,8 @@ public function testNoContentDirectiveError(): void
             , $bodyHtml);
 
         self::assertEquals(
-            ['Error while processing "note" directive in "no_content_directive" around line 6: Content expected, none found.'],
-            $this->builder->getErrorManager()->getErrors()
+            'Error while processing "note" directive: "Content expected, none found." in file "no_content_directive" at line 6',
+            $this->builder->getErrorManager()->getErrors()[0]->asString()
         );
     }
 

diff --git a/tests/EnvironmentTest.php b/tests/EnvironmentTest.php
@@ -82,7 +82,7 @@ public function getMissingSectionTests(): iterable
         ];
 
         yield 'with_current_filename' => [
-            'Unknown reference section "doc" in "current_doc_filename"',
+            'Unknown reference section "doc" in file "current_doc_filename"',
             'current_doc_filename',
         ];
     }

diff --git a/tests/ErrorManagerTest.php b/tests/ErrorManagerTest.php
@@ -8,9 +8,6 @@
 use Doctrine\RST\ErrorManager;
 use PHPUnit\Framework\TestCase;
 
-use function ob_end_clean;
-use function ob_start;
-
 class ErrorManagerTest extends TestCase
 {
     public function testGetErrors(): void
@@ -19,12 +16,16 @@ public function testGetErrors(): void
         $configuration->expects(self::atLeastOnce())
             ->method('isAbortOnError')
             ->willReturn(false);
+        $configuration->expects(self::atLeastOnce())
+            ->method('isSilentOnError')
+            ->willReturn(true);
 
         $errorManager = new ErrorManager($configuration);
-        ob_start();
         $errorManager->error('ERROR FOO');
         $errorManager->error('ERROR BAR');
-        ob_end_clean();
-        self::assertSame(['ERROR FOO', 'ERROR BAR'], $errorManager->getErrors());
+
+        $errors = $errorManager->getErrors();
+        self::assertSame('ERROR FOO', $errors[0]->asString());
+        self::assertSame('ERROR BAR', $errors[1]->asString());
     }
 }
diff --git a/tests/Functional/tests/main-directive/main-directive.html b/tests/Functional/tests/main-directive/main-directive.html
@@ -1,2 +1,2 @@
 Exception: Exception
-Unknown directive: "latex-main" for line ".. latex-main::"
+Unknown directive "latex-main" for line ".. latex-main::"
diff --git a/tests/Functional/tests/pretty-table-error1/pretty-table-error1.html b/tests/Functional/tests/pretty-table-error1/pretty-table-error1.html
@@ -4,7 +4,6 @@
 forgot a blank line
 
 does not appear to be a complete table row
-in file (unknown)
 
 +-----------+----------------+----------------------------+
 | Type      | Options        | Description                |
@@ -13,4 +12,4 @@
 +-----------+----------------+----------------------------+
 | currency  | currency (m)   | A currency string          |
 +-----------+----------------+----------------------------+
-forgot a blank line
+forgot a blank line in file "(unknown)"
diff --git a/tests/Functional/tests/simple-table-error1/simple-table-error1.html b/tests/Functional/tests/simple-table-error1/simple-table-error1.html
@@ -1,9 +1,8 @@
 Exception: Exception
 Malformed table: content "l" appears in the "gap" on row "Second row  Other colllOther col"
-in file (unknown)
 
 =========== ========== =========
 First col   Second col Third col
 Second row  Other colllOther col
 Third  row  Other col  Last col
-=========== ========== =========
+=========== ========== ========= in file "(unknown)"
diff --git a/tests/Functional/tests/unknown-directive/unknown-directive.html b/tests/Functional/tests/unknown-directive/unknown-directive.html
@@ -1,2 +1,2 @@
 Exception: Exception
-Unknown directive: "unknown-directive" for line ".. unknown-directive::"
+Unknown directive "unknown-directive" for line ".. unknown-directive::"
diff --git a/tests/Parser/DocumentParserTest.php b/tests/Parser/DocumentParserTest.php
@@ -7,6 +7,7 @@
 use Doctrine\Common\EventManager;
 use Doctrine\RST\Directives\Directive;
 use Doctrine\RST\Environment;
+use Doctrine\RST\ErrorManager;
 use Doctrine\RST\NodeFactory\NodeFactory;
 use Doctrine\RST\Parser;
 use Doctrine\RST\Parser\DocumentParser;
@@ -22,6 +23,7 @@ public function testErrorWhenDirectiveThrowsException(): void
         $nodeFactory        = $this->createMock(NodeFactory::class);
         $eventManager       = $this->createMock(EventManager::class);
         $codeBlockDirective = $this->createMock(Directive::class);
+        $errorManager       = $this->createMock(ErrorManager::class);
 
         $docParser = new DocumentParser(
             $parser,
@@ -41,8 +43,12 @@ public function testErrorWhenDirectiveThrowsException(): void
             ->willReturn('code-block-name');
 
         $environment->expects(self::once())
-            ->method('addError')
-            ->with('Error while processing "code-block-name" directive: Invalid something something!');
+            ->method('getErrorManager')
+            ->willReturn($errorManager);
+
+        $errorManager->expects(self::once())
+            ->method('error')
+            ->with('Error while processing "code-block-name" directive: "Invalid something something!"');
 
         $docParser->parse('.. code-block:: php');
     }