diff --git a/src/Application.php b/src/Application.php index 45a0a0a..9827b56 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 0000000..83b3bd9 --- /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 fc29ef2..9dbd2fe 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 0000000..6b31ca2 --- /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 15e7e9a..2884155 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/ApplicationTest.php b/tests/Unit/ApplicationTest.php index 4233537..275288a 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 298a3c6..ad3e5d2 100644 --- a/tests/Unit/Diff/DiffParserTest.php +++ b/tests/Unit/Diff/DiffParserTest.php @@ -4,13 +4,18 @@ namespace DocbookCS\Tests\Unit\Diff; +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 { @@ -24,7 +29,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 +46,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 +67,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 +84,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 +104,7 @@ public function itExcludesDeletedFiles(): void -line3 DIFF; - self::assertSame([], $this->parser->parse($diff)); + self::assertSame([], $this->lineNumbersByFile($this->parser->parse($diff))); } #[Test] @@ -116,7 +121,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 +147,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,7 +169,7 @@ 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). self::assertArrayHasKey('file.xml', $result); @@ -185,7 +190,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 +214,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 +230,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 af38188..47b6d25 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; @@ -21,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; #[ @@ -36,6 +39,8 @@ CoversClass(SniffRunner::class), CoversClass(Violation::class), CoversClass(XmlFileProcessor::class), + UsesClass(Diff::class), + UsesClass(FileChange::class), ] final class SniffRunnerTest extends TestCase { @@ -238,8 +243,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 +255,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 +269,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 +316,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());