Skip to content

Commit f8ebb86

Browse files
NickSdotNickSdot
authored
refactor: introduced Diff and FileChange VOs (#29)
* refactor: model file changes as value objects * chore: restored original comment * test: declare diff value object usage
1 parent 64d9330 commit f8ebb86

8 files changed

Lines changed: 117 additions & 92 deletions

File tree

‎src/Application.php‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -83,12 +83,12 @@ public function run(): int
8383
$overridePaths = $this->resolveOverridePaths($overridePaths);
8484
}
8585

86-
$diffLines = null;
86+
$diff = null;
8787

8888
if ($options['diff'] !== null) {
8989
try {
9090
$diffContent = $this->readDiff($options['diff']);
91-
$diffLines = (new DiffParser())->parse($diffContent);
91+
$diff = (new DiffParser())->parse($diffContent);
9292
} catch (\Throwable $e) {
9393
$this->writeError('Error reading diff: ' . $e->getMessage() . PHP_EOL);
9494

@@ -100,7 +100,7 @@ public function run(): int
100100

101101
try {
102102
$runner = new SniffRunner($progress);
103-
$report = $runner->run($config, $overridePaths, $diffLines);
103+
$report = $runner->run($config, $overridePaths, $diff);
104104
} catch (\Throwable $e) {
105105
$this->writeError('Runtime error: ' . $e->getMessage() . PHP_EOL);
106106

‎src/Diff/Diff.php‎

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,31 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
5+
namespace DocbookCS\Diff;
6+
7+
final readonly class Diff
8+
{
9+
/** @param list<FileChange> $fileChanges */
10+
public function __construct(public array $fileChanges)
11+
{
12+
}
13+
14+
public function changeFor(string $filePath): ?FileChange
15+
{
16+
$normalisedPath = str_replace('\\', '/', $filePath);
17+
18+
foreach ($this->fileChanges as $fileChange) {
19+
$normalisedDiffPath = str_replace('\\', '/', $fileChange->filePath);
20+
21+
if (
22+
$normalisedPath === $normalisedDiffPath
23+
|| str_ends_with($normalisedPath, '/' . ltrim($normalisedDiffPath, '/'))
24+
) {
25+
return $fileChange;
26+
}
27+
}
28+
29+
return null;
30+
}
31+
}

‎src/Diff/DiffParser.php‎

Lines changed: 13 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -8,10 +8,10 @@ final class DiffParser
88
{
99
private const string NO_FINAL_LINE_MARKER = '\ No newline at end of file';
1010

11-
/** @return array<string, list<int>> */
12-
public function parse(string $diff): array
11+
public function parse(string $diff): Diff
1312
{
14-
$result = [];
13+
/** @var array<string, list<int>> $changedLinesByFile */
14+
$changedLinesByFile = [];
1515
$currentFile = null;
1616
$deleted = false;
1717
$newLineNumber = 0;
@@ -41,8 +41,8 @@ public function parse(string $diff): array
4141
}
4242
$currentFile = $path !== '/dev/null' ? $path : null;
4343
$inHunk = false;
44-
if ($currentFile !== null && !isset($result[$currentFile])) {
45-
$result[$currentFile] = [];
44+
if ($currentFile !== null && !isset($changedLinesByFile[$currentFile])) {
45+
$changedLinesByFile[$currentFile] = [];
4646
}
4747
continue;
4848
}
@@ -67,7 +67,7 @@ public function parse(string $diff): array
6767
}
6868

6969
if (str_starts_with($line, '+')) {
70-
$result[$currentFile][] = $newLineNumber;
70+
$changedLinesByFile[$currentFile][] = $newLineNumber;
7171
$newLineNumber++;
7272
$newLinesRemaining--;
7373
} elseif (str_starts_with($line, '-')) {
@@ -84,6 +84,12 @@ public function parse(string $diff): array
8484
}
8585
}
8686

87-
return $result;
87+
$fileChanges = [];
88+
89+
foreach ($changedLinesByFile as $filePath => $lineNumbers) {
90+
$fileChanges[] = new FileChange($filePath, $lineNumbers);
91+
}
92+
93+
return new Diff($fileChanges);
8894
}
8995
}

‎src/Diff/FileChange.php‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
5+
namespace DocbookCS\Diff;
6+
7+
final readonly class FileChange
8+
{
9+
/** @param list<int> $addedLineNumbers */
10+
public function __construct(
11+
public string $filePath,
12+
public array $addedLineNumbers,
13+
) {
14+
}
15+
}

‎src/Runner/SniffRunner.php‎

Lines changed: 8 additions & 63 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66

77
use DocbookCS\Config\ConfigData;
88
use DocbookCS\Config\SniffEntry;
9+
use DocbookCS\Diff\Diff;
910
use DocbookCS\Path\EntityResolver;
1011
use DocbookCS\Path\PathLoader;
1112
use DocbookCS\Path\PathMatcher;
@@ -25,11 +26,10 @@ public function __construct(?ProgressInterface $progress = null)
2526

2627
/**
2728
* @param list<string>|null $overridePaths
28-
* @param array<string, list<int>>|null $diffLines
2929
* @throws \RuntimeException if a sniff class cannot be found or does not implement SniffInterface.
3030
* @throws \UnexpectedValueException if no files are found to scan.
3131
*/
32-
public function run(ConfigData $config, ?array $overridePaths = null, ?array $diffLines = null): Report
32+
public function run(ConfigData $config, ?array $overridePaths = null, ?Diff $diff = null): Report
3333
{
3434
$startTime = microtime(true);
3535

@@ -45,8 +45,11 @@ public function run(ConfigData $config, ?array $overridePaths = null, ?array $di
4545
$pathLoader = new PathLoader($includePaths, $matcher);
4646
$files = $pathLoader->loadPaths();
4747

48-
if ($diffLines !== null) {
49-
$files = $this->filterByDiff($files, array_keys($diffLines));
48+
if ($diff !== null) {
49+
$files = array_values(array_filter(
50+
$files,
51+
static fn(string $file): bool => $diff->changeFor($file) !== null,
52+
));
5053
}
5154

5255
$report = new Report();
@@ -60,9 +63,7 @@ public function run(ConfigData $config, ?array $overridePaths = null, ?array $di
6063
foreach ($files as $index => $file) {
6164
$report->incrementFilesScanned();
6265

63-
$changedLines = $diffLines !== null
64-
? $this->getChangedLinesForFile($file, $diffLines)
65-
: null;
66+
$changedLines = $diff?->changeFor($file)?->addedLineNumbers;
6667

6768
$fileReport = $processor->processFile(
6869
$file,
@@ -125,40 +126,6 @@ private function instantiateSniffs(array $entries): array
125126
return $sniffs;
126127
}
127128

128-
/**
129-
* @param list<string> $files
130-
* @param list<string> $diffPaths
131-
* @return list<string>
132-
*/
133-
private function filterByDiff(array $files, array $diffPaths): array
134-
{
135-
return array_values(
136-
array_filter(
137-
$files,
138-
fn(string $file) => $this->matchesDiffPath($file, $diffPaths),
139-
)
140-
);
141-
}
142-
143-
/** @param list<string> $diffPaths */
144-
private function matchesDiffPath(string $absolutePath, array $diffPaths): bool
145-
{
146-
$normalized = str_replace('\\', '/', $absolutePath);
147-
148-
foreach ($diffPaths as $diffPath) {
149-
$normalizedDiff = str_replace('\\', '/', $diffPath);
150-
151-
if (
152-
$normalized === $normalizedDiff
153-
|| str_ends_with($normalized, '/' . ltrim($normalizedDiff, '/'))
154-
) {
155-
return true;
156-
}
157-
}
158-
159-
return false;
160-
}
161-
162129
private function makeRelative(string $absolutePath): string
163130
{
164131
$cwd = getcwd();
@@ -175,26 +142,4 @@ private function makeRelative(string $absolutePath): string
175142

176143
return $absolutePath; // @codeCoverageIgnore
177144
}
178-
179-
/**
180-
* @param array<string, list<int>> $diffLines
181-
* @return list<int>
182-
*/
183-
private function getChangedLinesForFile(string $absolutePath, array $diffLines): array
184-
{
185-
$normalized = str_replace('\\', '/', $absolutePath);
186-
187-
foreach ($diffLines as $diffPath => $lines) {
188-
$normalizedDiff = str_replace('\\', '/', $diffPath);
189-
190-
if (
191-
$normalized === $normalizedDiff
192-
|| str_ends_with($normalized, '/' . ltrim($normalizedDiff, '/'))
193-
) {
194-
return $lines;
195-
}
196-
}
197-
198-
return []; // @codeCoverageIgnore
199-
}
200145
}

‎tests/Unit/ApplicationTest.php‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,9 @@
66

77
use DocbookCS\Application;
88
use DocbookCS\Config\ConfigData;
9+
use DocbookCS\Diff\Diff;
910
use DocbookCS\Diff\DiffParser;
11+
use DocbookCS\Diff\FileChange;
1012
use DocbookCS\Config\ConfigParser;
1113
use DocbookCS\Config\ConfigParserException;
1214
use DocbookCS\Config\SniffEntry;
@@ -25,6 +27,7 @@
2527
use DocbookCS\Sniff\ExceptionNameSniff;
2628
use PHPUnit\Framework\Attributes\CoversClass;
2729
use PHPUnit\Framework\Attributes\Test;
30+
use PHPUnit\Framework\Attributes\UsesClass;
2831
use PHPUnit\Framework\TestCase;
2932

3033
#[
@@ -47,6 +50,8 @@
4750
CoversClass(SniffEntry::class),
4851
CoversClass(SniffRunner::class),
4952
CoversClass(XmlFileProcessor::class),
53+
UsesClass(Diff::class),
54+
UsesClass(FileChange::class),
5055
]
5156
final class ApplicationTest extends TestCase
5257
{

‎tests/Unit/Diff/DiffParserTest.php‎

Lines changed: 29 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -4,13 +4,18 @@
44

55
namespace DocbookCS\Tests\Unit\Diff;
66

7+
use DocbookCS\Diff\Diff;
78
use DocbookCS\Diff\DiffParser;
9+
use DocbookCS\Diff\FileChange;
810
use PHPUnit\Framework\Attributes\CoversClass;
911
use PHPUnit\Framework\Attributes\Test;
12+
use PHPUnit\Framework\Attributes\UsesClass;
1013
use PHPUnit\Framework\TestCase;
1114

1215
#[
1316
CoversClass(DiffParser::class),
17+
UsesClass(Diff::class),
18+
UsesClass(FileChange::class),
1419
]
1520
final class DiffParserTest extends TestCase
1621
{
@@ -24,7 +29,7 @@ protected function setUp(): void
2429
#[Test]
2530
public function itReturnsEmptyArrayForEmptyDiff(): void
2631
{
27-
self::assertSame([], $this->parser->parse(''));
32+
self::assertSame([], $this->lineNumbersByFile($this->parser->parse('')));
2833
}
2934

3035
#[Test]
@@ -41,7 +46,7 @@ public function itParsesAddedLineNumbers(): void
4146
line3
4247
DIFF;
4348

44-
$result = $this->parser->parse($diff);
49+
$result = $this->lineNumbersByFile($this->parser->parse($diff));
4550

4651
self::assertArrayHasKey('reference/file.xml', $result);
4752
self::assertSame([2], $result['reference/file.xml']);
@@ -62,7 +67,7 @@ public function itParsesMultipleAddedLines(): void
6267
last line
6368
DIFF;
6469

65-
$result = $this->parser->parse($diff);
70+
$result = $this->lineNumbersByFile($this->parser->parse($diff));
6671

6772
self::assertSame([6, 7], $result['doc/chapter.xml']);
6873
}
@@ -79,7 +84,7 @@ public function itStripsTheBPrefix(): void
7984
+added
8085
DIFF;
8186

82-
$result = $this->parser->parse($diff);
87+
$result = $this->lineNumbersByFile($this->parser->parse($diff));
8388

8489
self::assertArrayHasKey('src/file.xml', $result);
8590
self::assertArrayNotHasKey('b/src/file.xml', $result);
@@ -99,7 +104,7 @@ public function itExcludesDeletedFiles(): void
99104
-line3
100105
DIFF;
101106

102-
self::assertSame([], $this->parser->parse($diff));
107+
self::assertSame([], $this->lineNumbersByFile($this->parser->parse($diff)));
103108
}
104109

105110
#[Test]
@@ -116,7 +121,7 @@ public function itHandlesNewlyCreatedFiles(): void
116121
+line3
117122
DIFF;
118123

119-
$result = $this->parser->parse($diff);
124+
$result = $this->lineNumbersByFile($this->parser->parse($diff));
120125

121126
self::assertArrayHasKey('new.xml', $result);
122127
self::assertSame([1, 2, 3], $result['new.xml']);
@@ -142,7 +147,7 @@ public function itHandlesMultipleFilesInOneDiff(): void
142147
unchanged
143148
DIFF;
144149

145-
$result = $this->parser->parse($diff);
150+
$result = $this->lineNumbersByFile($this->parser->parse($diff));
146151

147152
self::assertArrayHasKey('first.xml', $result);
148153
self::assertArrayHasKey('second.xml', $result);
@@ -164,7 +169,7 @@ public function itIgnoresRemovedLines(): void
164169
line3
165170
DIFF;
166171

167-
$result = $this->parser->parse($diff);
172+
$result = $this->lineNumbersByFile($this->parser->parse($diff));
168173

169174
// No lines added, so the changed set is empty (not absent — the file is tracked).
170175
self::assertArrayHasKey('file.xml', $result);
@@ -185,7 +190,7 @@ public function itIgnoresTheMissingFinalNewlineMarker(): void
185190
+second
186191
DIFF;
187192

188-
$result = $this->parser->parse($diff);
193+
$result = $this->lineNumbersByFile($this->parser->parse($diff));
189194

190195
self::assertSame([1, 2], $result['file.xml']);
191196
}
@@ -209,7 +214,7 @@ public function itTracksLineNumbersAcrossMultipleHunks(): void
209214
line12
210215
DIFF;
211216

212-
$result = $this->parser->parse($diff);
217+
$result = $this->lineNumbersByFile($this->parser->parse($diff));
213218

214219
self::assertSame([2, 12], $result['file.xml']);
215220
}
@@ -225,8 +230,21 @@ public function itHandlesHunkWithNoContext(): void
225230
+only line
226231
DIFF;
227232

228-
$result = $this->parser->parse($diff);
233+
$result = $this->lineNumbersByFile($this->parser->parse($diff));
229234

230235
self::assertSame([1], $result['file.xml']);
231236
}
237+
238+
// TODO: avoids test diff churn; remove when fixers merged
239+
/** @return array<string, list<int>> */
240+
private function lineNumbersByFile(Diff $diff): array
241+
{
242+
$lineNumbersByFile = [];
243+
244+
foreach ($diff->fileChanges as $fileChange) {
245+
$lineNumbersByFile[$fileChange->filePath] = $fileChange->addedLineNumbers;
246+
}
247+
248+
return $lineNumbersByFile;
249+
}
232250
}

0 commit comments

Comments
 (0)