From 2ef80c48384abc19f041f77f4e7e5ee116ecd3fa Mon Sep 17 00:00:00 2001 From: NickSdot Date: Tue, 21 Jul 2026 01:49:04 +0700 Subject: [PATCH 1/3] refactor: model file changes as value objects --- src/Application.php | 6 +-- src/Diff/Diff.php | 31 ++++++++++++ src/Diff/DiffParser.php | 20 +++++--- src/Diff/FileChange.php | 15 ++++++ src/Runner/SniffRunner.php | 71 +++------------------------ tests/Unit/Diff/DiffParserTest.php | 38 +++++++++----- tests/Unit/Runner/SniffRunnerTest.php | 18 ++++--- 7 files changed, 106 insertions(+), 93 deletions(-) create mode 100644 src/Diff/Diff.php create mode 100644 src/Diff/FileChange.php diff --git a/src/Application.php b/src/Application.php index 45a0a0a8..9827b560 100644 --- a/src/Application.php +++ b/src/Application.php @@ -83,12 +83,12 @@ public function run(): int $overridePaths = $this->resolveOverridePaths($overridePaths); } - $diffLines = null; + $diff = null; if ($options['diff'] !== null) { try { $diffContent = $this->readDiff($options['diff']); - $diffLines = (new DiffParser())->parse($diffContent); + $diff = (new DiffParser())->parse($diffContent); } catch (\Throwable $e) { $this->writeError('Error reading diff: ' . $e->getMessage() . PHP_EOL); @@ -100,7 +100,7 @@ public function run(): int try { $runner = new SniffRunner($progress); - $report = $runner->run($config, $overridePaths, $diffLines); + $report = $runner->run($config, $overridePaths, $diff); } catch (\Throwable $e) { $this->writeError('Runtime error: ' . $e->getMessage() . PHP_EOL); diff --git a/src/Diff/Diff.php b/src/Diff/Diff.php new file mode 100644 index 00000000..83b3bd9d --- /dev/null +++ b/src/Diff/Diff.php @@ -0,0 +1,31 @@ + $fileChanges */ + public function __construct(public array $fileChanges) + { + } + + public function changeFor(string $filePath): ?FileChange + { + $normalisedPath = str_replace('\\', '/', $filePath); + + foreach ($this->fileChanges as $fileChange) { + $normalisedDiffPath = str_replace('\\', '/', $fileChange->filePath); + + if ( + $normalisedPath === $normalisedDiffPath + || str_ends_with($normalisedPath, '/' . ltrim($normalisedDiffPath, '/')) + ) { + return $fileChange; + } + } + + return null; + } +} diff --git a/src/Diff/DiffParser.php b/src/Diff/DiffParser.php index fc29ef29..9dbd2fe1 100644 --- a/src/Diff/DiffParser.php +++ b/src/Diff/DiffParser.php @@ -8,10 +8,10 @@ final class DiffParser { private const string NO_FINAL_LINE_MARKER = '\ No newline at end of file'; - /** @return array> */ - public function parse(string $diff): array + public function parse(string $diff): Diff { - $result = []; + /** @var array> $changedLinesByFile */ + $changedLinesByFile = []; $currentFile = null; $deleted = false; $newLineNumber = 0; @@ -41,8 +41,8 @@ public function parse(string $diff): array } $currentFile = $path !== '/dev/null' ? $path : null; $inHunk = false; - if ($currentFile !== null && !isset($result[$currentFile])) { - $result[$currentFile] = []; + if ($currentFile !== null && !isset($changedLinesByFile[$currentFile])) { + $changedLinesByFile[$currentFile] = []; } continue; } @@ -67,7 +67,7 @@ public function parse(string $diff): array } if (str_starts_with($line, '+')) { - $result[$currentFile][] = $newLineNumber; + $changedLinesByFile[$currentFile][] = $newLineNumber; $newLineNumber++; $newLinesRemaining--; } elseif (str_starts_with($line, '-')) { @@ -84,6 +84,12 @@ public function parse(string $diff): array } } - return $result; + $fileChanges = []; + + foreach ($changedLinesByFile as $filePath => $lineNumbers) { + $fileChanges[] = new FileChange($filePath, $lineNumbers); + } + + return new Diff($fileChanges); } } diff --git a/src/Diff/FileChange.php b/src/Diff/FileChange.php new file mode 100644 index 00000000..6b31ca22 --- /dev/null +++ b/src/Diff/FileChange.php @@ -0,0 +1,15 @@ + $addedLineNumbers */ + public function __construct( + public string $filePath, + public array $addedLineNumbers, + ) { + } +} diff --git a/src/Runner/SniffRunner.php b/src/Runner/SniffRunner.php index 15e7e9a9..28841553 100644 --- a/src/Runner/SniffRunner.php +++ b/src/Runner/SniffRunner.php @@ -6,6 +6,7 @@ use DocbookCS\Config\ConfigData; use DocbookCS\Config\SniffEntry; +use DocbookCS\Diff\Diff; use DocbookCS\Path\EntityResolver; use DocbookCS\Path\PathLoader; use DocbookCS\Path\PathMatcher; @@ -25,11 +26,10 @@ public function __construct(?ProgressInterface $progress = null) /** * @param list|null $overridePaths - * @param array>|null $diffLines * @throws \RuntimeException if a sniff class cannot be found or does not implement SniffInterface. * @throws \UnexpectedValueException if no files are found to scan. */ - public function run(ConfigData $config, ?array $overridePaths = null, ?array $diffLines = null): Report + public function run(ConfigData $config, ?array $overridePaths = null, ?Diff $diff = null): Report { $startTime = microtime(true); @@ -45,8 +45,11 @@ public function run(ConfigData $config, ?array $overridePaths = null, ?array $di $pathLoader = new PathLoader($includePaths, $matcher); $files = $pathLoader->loadPaths(); - if ($diffLines !== null) { - $files = $this->filterByDiff($files, array_keys($diffLines)); + if ($diff !== null) { + $files = array_values(array_filter( + $files, + static fn(string $file): bool => $diff->changeFor($file) !== null, + )); } $report = new Report(); @@ -60,9 +63,7 @@ public function run(ConfigData $config, ?array $overridePaths = null, ?array $di foreach ($files as $index => $file) { $report->incrementFilesScanned(); - $changedLines = $diffLines !== null - ? $this->getChangedLinesForFile($file, $diffLines) - : null; + $changedLines = $diff?->changeFor($file)?->addedLineNumbers; $fileReport = $processor->processFile( $file, @@ -125,40 +126,6 @@ private function instantiateSniffs(array $entries): array return $sniffs; } - /** - * @param list $files - * @param list $diffPaths - * @return list - */ - private function filterByDiff(array $files, array $diffPaths): array - { - return array_values( - array_filter( - $files, - fn(string $file) => $this->matchesDiffPath($file, $diffPaths), - ) - ); - } - - /** @param list $diffPaths */ - private function matchesDiffPath(string $absolutePath, array $diffPaths): bool - { - $normalized = str_replace('\\', '/', $absolutePath); - - foreach ($diffPaths as $diffPath) { - $normalizedDiff = str_replace('\\', '/', $diffPath); - - if ( - $normalized === $normalizedDiff - || str_ends_with($normalized, '/' . ltrim($normalizedDiff, '/')) - ) { - return true; - } - } - - return false; - } - private function makeRelative(string $absolutePath): string { $cwd = getcwd(); @@ -175,26 +142,4 @@ private function makeRelative(string $absolutePath): string return $absolutePath; // @codeCoverageIgnore } - - /** - * @param array> $diffLines - * @return list - */ - private function getChangedLinesForFile(string $absolutePath, array $diffLines): array - { - $normalized = str_replace('\\', '/', $absolutePath); - - foreach ($diffLines as $diffPath => $lines) { - $normalizedDiff = str_replace('\\', '/', $diffPath); - - if ( - $normalized === $normalizedDiff - || str_ends_with($normalized, '/' . ltrim($normalizedDiff, '/')) - ) { - return $lines; - } - } - - return []; // @codeCoverageIgnore - } } diff --git a/tests/Unit/Diff/DiffParserTest.php b/tests/Unit/Diff/DiffParserTest.php index 298a3c69..b23e2ade 100644 --- a/tests/Unit/Diff/DiffParserTest.php +++ b/tests/Unit/Diff/DiffParserTest.php @@ -4,6 +4,7 @@ namespace DocbookCS\Tests\Unit\Diff; +use DocbookCS\Diff\Diff; use DocbookCS\Diff\DiffParser; use PHPUnit\Framework\Attributes\CoversClass; use PHPUnit\Framework\Attributes\Test; @@ -24,7 +25,7 @@ protected function setUp(): void #[Test] public function itReturnsEmptyArrayForEmptyDiff(): void { - self::assertSame([], $this->parser->parse('')); + self::assertSame([], $this->lineNumbersByFile($this->parser->parse(''))); } #[Test] @@ -41,7 +42,7 @@ public function itParsesAddedLineNumbers(): void line3 DIFF; - $result = $this->parser->parse($diff); + $result = $this->lineNumbersByFile($this->parser->parse($diff)); self::assertArrayHasKey('reference/file.xml', $result); self::assertSame([2], $result['reference/file.xml']); @@ -62,7 +63,7 @@ public function itParsesMultipleAddedLines(): void last line DIFF; - $result = $this->parser->parse($diff); + $result = $this->lineNumbersByFile($this->parser->parse($diff)); self::assertSame([6, 7], $result['doc/chapter.xml']); } @@ -79,7 +80,7 @@ public function itStripsTheBPrefix(): void +added DIFF; - $result = $this->parser->parse($diff); + $result = $this->lineNumbersByFile($this->parser->parse($diff)); self::assertArrayHasKey('src/file.xml', $result); self::assertArrayNotHasKey('b/src/file.xml', $result); @@ -99,7 +100,7 @@ public function itExcludesDeletedFiles(): void -line3 DIFF; - self::assertSame([], $this->parser->parse($diff)); + self::assertSame([], $this->lineNumbersByFile($this->parser->parse($diff))); } #[Test] @@ -116,7 +117,7 @@ public function itHandlesNewlyCreatedFiles(): void +line3 DIFF; - $result = $this->parser->parse($diff); + $result = $this->lineNumbersByFile($this->parser->parse($diff)); self::assertArrayHasKey('new.xml', $result); self::assertSame([1, 2, 3], $result['new.xml']); @@ -142,7 +143,7 @@ public function itHandlesMultipleFilesInOneDiff(): void unchanged DIFF; - $result = $this->parser->parse($diff); + $result = $this->lineNumbersByFile($this->parser->parse($diff)); self::assertArrayHasKey('first.xml', $result); self::assertArrayHasKey('second.xml', $result); @@ -164,9 +165,9 @@ public function itIgnoresRemovedLines(): void line3 DIFF; - $result = $this->parser->parse($diff); + $result = $this->lineNumbersByFile($this->parser->parse($diff)); - // No lines added, so the changed set is empty (not absent — the file is tracked). + // No lines added, so the changed set is empty self::assertArrayHasKey('file.xml', $result); self::assertSame([], $result['file.xml']); } @@ -185,7 +186,7 @@ public function itIgnoresTheMissingFinalNewlineMarker(): void +second DIFF; - $result = $this->parser->parse($diff); + $result = $this->lineNumbersByFile($this->parser->parse($diff)); self::assertSame([1, 2], $result['file.xml']); } @@ -209,7 +210,7 @@ public function itTracksLineNumbersAcrossMultipleHunks(): void line12 DIFF; - $result = $this->parser->parse($diff); + $result = $this->lineNumbersByFile($this->parser->parse($diff)); self::assertSame([2, 12], $result['file.xml']); } @@ -225,8 +226,21 @@ public function itHandlesHunkWithNoContext(): void +only line DIFF; - $result = $this->parser->parse($diff); + $result = $this->lineNumbersByFile($this->parser->parse($diff)); self::assertSame([1], $result['file.xml']); } + + // TODO: avoids test diff churn; remove when fixers merged + /** @return array> */ + private function lineNumbersByFile(Diff $diff): array + { + $lineNumbersByFile = []; + + foreach ($diff->fileChanges as $fileChange) { + $lineNumbersByFile[$fileChange->filePath] = $fileChange->addedLineNumbers; + } + + return $lineNumbersByFile; + } } diff --git a/tests/Unit/Runner/SniffRunnerTest.php b/tests/Unit/Runner/SniffRunnerTest.php index af38188b..b12ed1e0 100644 --- a/tests/Unit/Runner/SniffRunnerTest.php +++ b/tests/Unit/Runner/SniffRunnerTest.php @@ -6,6 +6,8 @@ use DocbookCS\Config\ConfigData; use DocbookCS\Config\SniffEntry; +use DocbookCS\Diff\Diff; +use DocbookCS\Diff\FileChange; use DocbookCS\Path\EntityResolver; use DocbookCS\Path\PathLoader; use DocbookCS\Path\PathMatcher; @@ -238,8 +240,8 @@ public function itFiltersFilesToOnlyThoseInTheDiff(): void $config = $this->createConfig(); $runner = new SniffRunner(); - $diffLines = ['sniff_runner/default/file_a.xml' => [1]]; - $report = $runner->run($config, null, $diffLines); + $diff = new Diff([new FileChange('sniff_runner/default/file_a.xml', [1])]); + $report = $runner->run($config, null, $diff); self::assertSame(1, $report->getFilesScanned()); } @@ -250,8 +252,8 @@ public function itScansNoFilesWhenDiffContainsNoMatchingPaths(): void $config = $this->createConfig(); $runner = new SniffRunner(); - $diffLines = ['completely/different/file.xml' => [1, 2, 3]]; - $report = $runner->run($config, null, $diffLines); + $diff = new Diff([new FileChange('completely/different/file.xml', [1, 2, 3])]); + $report = $runner->run($config, null, $diff); self::assertSame(0, $report->getFilesScanned()); } @@ -264,8 +266,8 @@ public function itMatchesWhenDiffPathEqualsDiscoveredPath(): void $discoveredPath = self::FIXTURE_DIR . '/file_a.xml'; - $diffLines = [$discoveredPath => [1]]; - $report = $runner->run($config, null, $diffLines); + $diff = new Diff([new FileChange($discoveredPath, [1])]); + $report = $runner->run($config, null, $diff); self::assertSame(1, $report->getFilesScanned()); } @@ -311,8 +313,8 @@ public function setProperty(string $name, string $value): void $config = $this->createConfig(sniffs: [new SniffEntry($sniff::class)]); $runner = new SniffRunner(); - $diffLines = ['sniff_runner/default/file_a.xml' => []]; - $report = $runner->run($config, null, $diffLines); + $diff = new Diff([new FileChange('sniff_runner/default/file_a.xml', [])]); + $report = $runner->run($config, null, $diff); self::assertSame(1, $report->getFilesScanned()); self::assertFalse($report->hasViolations()); From ded415c0e533ebb30fb2f0863a9f7aaf47592914 Mon Sep 17 00:00:00 2001 From: NickSdot Date: Tue, 21 Jul 2026 02:56:00 +0700 Subject: [PATCH 2/3] chore: restored original comment --- tests/Unit/Diff/DiffParserTest.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/Unit/Diff/DiffParserTest.php b/tests/Unit/Diff/DiffParserTest.php index b23e2ade..9e625da0 100644 --- a/tests/Unit/Diff/DiffParserTest.php +++ b/tests/Unit/Diff/DiffParserTest.php @@ -167,7 +167,7 @@ public function itIgnoresRemovedLines(): void $result = $this->lineNumbersByFile($this->parser->parse($diff)); - // No lines added, so the changed set is empty + // No lines added, so the changed set is empty (not absent — the file is tracked). self::assertArrayHasKey('file.xml', $result); self::assertSame([], $result['file.xml']); } From bdcf4e26fcc1c12f333b311982559be29ed41970 Mon Sep 17 00:00:00 2001 From: NickSdot Date: Tue, 21 Jul 2026 03:08:35 +0700 Subject: [PATCH 3/3] test: declare diff value object usage --- tests/Unit/ApplicationTest.php | 5 +++++ tests/Unit/Diff/DiffParserTest.php | 4 ++++ tests/Unit/Runner/SniffRunnerTest.php | 3 +++ 3 files changed, 12 insertions(+) diff --git a/tests/Unit/ApplicationTest.php b/tests/Unit/ApplicationTest.php index 42335376..275288a7 100644 --- a/tests/Unit/ApplicationTest.php +++ b/tests/Unit/ApplicationTest.php @@ -6,7 +6,9 @@ use DocbookCS\Application; use DocbookCS\Config\ConfigData; +use DocbookCS\Diff\Diff; use DocbookCS\Diff\DiffParser; +use DocbookCS\Diff\FileChange; use DocbookCS\Config\ConfigParser; use DocbookCS\Config\ConfigParserException; use DocbookCS\Config\SniffEntry; @@ -25,6 +27,7 @@ use DocbookCS\Sniff\ExceptionNameSniff; use PHPUnit\Framework\Attributes\CoversClass; use PHPUnit\Framework\Attributes\Test; +use PHPUnit\Framework\Attributes\UsesClass; use PHPUnit\Framework\TestCase; #[ @@ -47,6 +50,8 @@ CoversClass(SniffEntry::class), CoversClass(SniffRunner::class), CoversClass(XmlFileProcessor::class), + UsesClass(Diff::class), + UsesClass(FileChange::class), ] final class ApplicationTest extends TestCase { diff --git a/tests/Unit/Diff/DiffParserTest.php b/tests/Unit/Diff/DiffParserTest.php index 9e625da0..ad3e5d2c 100644 --- a/tests/Unit/Diff/DiffParserTest.php +++ b/tests/Unit/Diff/DiffParserTest.php @@ -6,12 +6,16 @@ use DocbookCS\Diff\Diff; use DocbookCS\Diff\DiffParser; +use DocbookCS\Diff\FileChange; use PHPUnit\Framework\Attributes\CoversClass; use PHPUnit\Framework\Attributes\Test; +use PHPUnit\Framework\Attributes\UsesClass; use PHPUnit\Framework\TestCase; #[ CoversClass(DiffParser::class), + UsesClass(Diff::class), + UsesClass(FileChange::class), ] final class DiffParserTest extends TestCase { diff --git a/tests/Unit/Runner/SniffRunnerTest.php b/tests/Unit/Runner/SniffRunnerTest.php index b12ed1e0..47b6d25e 100644 --- a/tests/Unit/Runner/SniffRunnerTest.php +++ b/tests/Unit/Runner/SniffRunnerTest.php @@ -23,6 +23,7 @@ use DocbookCS\Sniff\SniffInterface; use PHPUnit\Framework\Attributes\CoversClass; use PHPUnit\Framework\Attributes\Test; +use PHPUnit\Framework\Attributes\UsesClass; use PHPUnit\Framework\TestCase; #[ @@ -38,6 +39,8 @@ CoversClass(SniffRunner::class), CoversClass(Violation::class), CoversClass(XmlFileProcessor::class), + UsesClass(Diff::class), + UsesClass(FileChange::class), ] final class SniffRunnerTest extends TestCase {