Skip to content

Commit f44cc54

Browse files
SeldaekSeldaek
committed
Improve comment readability
1 parent f3c7883 commit f44cc54

7 files changed

Lines changed: 269 additions & 21 deletions

File tree

‎action.yml‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -167,6 +167,7 @@ runs:
167167
SHOW_MODIFIED: ${{ inputs.show-modified }}
168168
COMMENT_MARKER: ${{ inputs.comment-marker }}
169169
HEADING: ${{ inputs.heading }}
170+
PR_NUMBER: ${{ steps.pr.outputs.number }}
170171
ACTION_PATH: ${{ github.action_path }}
171172
run: bash "${ACTION_PATH}/scripts/detect.sh"
172173

‎scripts/detect.sh‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -120,6 +120,12 @@ diff_args=(
120120
if [[ "${INCLUDE_INTERNAL}" == "true" ]]; then
121121
diff_args+=("--include-internal=true")
122122
fi
123+
if [[ -n "${GITHUB_REPOSITORY:-}" ]]; then
124+
diff_args+=("--repo=${GITHUB_REPOSITORY}")
125+
fi
126+
if [[ -n "${PR_NUMBER:-}" ]]; then
127+
diff_args+=("--pr-number=${PR_NUMBER}")
128+
fi
123129

124130
php "${SCRIPT_DIR}/diff.php" "${diff_args[@]}"
125131

‎scripts/diff.php‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,8 @@
3434
'show-modified' => $opts['show-modified'],
3535
'comment-marker' => $opts['comment-marker'],
3636
'heading' => $opts['heading'],
37+
'repo' => $opts['repo'],
38+
'pr-number' => $opts['pr-number'],
3739
]);
3840

3941
file_put_contents($opts['output'], $differ->diff($head, $base));
@@ -51,6 +53,8 @@ function parseArgs(array $argv): array
5153
'show-modified' => false,
5254
'comment-marker' => '<!-- api-surface-bot -->',
5355
'heading' => '## API Surface Changes',
56+
'repo' => null,
57+
'pr-number' => null,
5458
];
5559
foreach (array_slice($argv, 1) as $arg) {
5660
if (str_starts_with($arg, '--head=')) {
@@ -75,6 +79,12 @@ function parseArgs(array $argv): array
7579
$opts['comment-marker'] = substr($arg, 17);
7680
} elseif (str_starts_with($arg, '--heading=')) {
7781
$opts['heading'] = substr($arg, 10);
82+
} elseif (str_starts_with($arg, '--repo=')) {
83+
$value = substr($arg, 7);
84+
$opts['repo'] = $value !== '' ? $value : null;
85+
} elseif (str_starts_with($arg, '--pr-number=')) {
86+
$value = substr($arg, 12);
87+
$opts['pr-number'] = $value !== '' ? $value : null;
7888
}
7989
}
8090
foreach (['head', 'base', 'output'] as $required) {

‎src/Differ.php‎

Lines changed: 120 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,8 @@ final class Differ
3030
* show-modified?: bool,
3131
* comment-marker?: string,
3232
* heading?: string,
33+
* repo?: string,
34+
* pr-number?: int|string,
3335
* } $options
3436
*/
3537
public function __construct(private array $options = [])
@@ -70,13 +72,13 @@ public function diff(array $headRecords, array $baseRecords): string
7072

7173
$body = '';
7274
if (!empty($added)) {
73-
$body .= self::renderSection('New API Surface', $added);
75+
$body .= $this->renderSection('New API Surface', $added, side: 'R');
7476
}
7577
if ($showRemoved && !empty($removed)) {
76-
$body .= self::renderSection('Removed API Surface', $removed);
78+
$body .= $this->renderSection('Removed API Surface', $removed, side: 'L');
7779
}
7880
if ($showModified && !empty($modified)) {
79-
$body .= self::renderModifiedSection($modified);
81+
$body .= $this->renderModifiedSection($modified);
8082
}
8183

8284
if ($body === '') {
@@ -136,7 +138,7 @@ public static function recordKey(array $record): string
136138
/**
137139
* @param list<array<string,mixed>> $records
138140
*/
139-
private static function renderSection(string $title, array $records): string
141+
private function renderSection(string $title, array $records, string $side): string
140142
{
141143
$byKind = [];
142144
foreach ($records as $r) {
@@ -149,26 +151,31 @@ private static function renderSection(string $title, array $records): string
149151
}
150152
$out .= "\n#### " . self::KIND_LABELS[$kind] . "\n";
151153
foreach ($byKind[$kind] as $r) {
152-
$out .= self::renderRecord($r) . "\n";
154+
$out .= $this->renderRecord($r, $side) . "\n";
153155
}
154156
}
155157
return $out . "\n";
156158
}
157159

158-
private static function renderRecord(array $record): string
160+
private function renderRecord(array $record, string $side): string
159161
{
160-
$loc = '`' . $record['file'] . ':' . $record['line'] . '`';
161-
if ($record['member'] !== null) {
162-
$label = $record['fqcn'] . '::' . $record['member'];
163-
return "- `{$label}` — `" . trim($record['signature']) . '` in ' . $loc;
162+
$label = $record['member'] !== null
163+
? $record['fqcn'] . '::' . $record['member']
164+
: $record['fqcn'];
165+
166+
$head = $this->formatLabel($label, $record['file'], (int) $record['line'], $side);
167+
$sig = trim($record['signature']);
168+
$line = '- ' . $head . ' — `' . $sig . '`';
169+
if (!$this->hasRepoLink()) {
170+
$line .= ' in `' . $record['file'] . ':' . $record['line'] . '`';
164171
}
165-
return "- `" . trim($record['signature']) . '` in ' . $loc;
172+
return $line;
166173
}
167174

168175
/**
169176
* @param list<array{head: array<string,mixed>, base: array<string,mixed>}> $changes
170177
*/
171-
private static function renderModifiedSection(array $changes): string
178+
private function renderModifiedSection(array $changes): string
172179
{
173180
$byKind = [];
174181
foreach ($changes as $c) {
@@ -184,11 +191,109 @@ private static function renderModifiedSection(array $changes): string
184191
$h = $c['head'];
185192
$b = $c['base'];
186193
$label = $h['member'] !== null ? ($h['fqcn'] . '::' . $h['member']) : $h['fqcn'];
187-
$out .= "- `{$label}` in `" . $h['file'] . ':' . $h['line'] . "`\n";
188-
$out .= " - was: `" . trim($b['signature']) . "`\n";
189-
$out .= " - now: `" . trim($h['signature']) . "`\n";
194+
195+
$head = $this->formatLabel($label, $h['file'], (int) $h['line'], 'R');
196+
$line = '- ' . $head;
197+
if (!$this->hasRepoLink()) {
198+
$line .= ' in `' . $h['file'] . ':' . $h['line'] . '`';
199+
}
200+
$out .= $line . "\n";
201+
$out .= self::renderSignatureDiff(
202+
self::sourceFor($b),
203+
self::sourceFor($h),
204+
) . "\n";
190205
}
191206
}
192207
return $out . "\n";
193208
}
209+
210+
private function hasRepoLink(): bool
211+
{
212+
return !empty($this->options['repo']) && !empty($this->options['pr-number']);
213+
}
214+
215+
private function formatLabel(string $label, string $file, int $line, string $side): string
216+
{
217+
if (!$this->hasRepoLink()) {
218+
return '`' . $label . '`';
219+
}
220+
$url = sprintf(
221+
'https://github.com/%s/pull/%s/files#diff-%s%s%d',
222+
$this->options['repo'],
223+
$this->options['pr-number'],
224+
hash('sha256', $file),
225+
$side,
226+
$line,
227+
);
228+
return '[`' . $label . '`](' . $url . ')';
229+
}
230+
231+
private static function sourceFor(array $record): string
232+
{
233+
if (!empty($record['signature_source'])) {
234+
return rtrim($record['signature_source']);
235+
}
236+
return rtrim($record['signature']);
237+
}
238+
239+
/**
240+
* Render the was → now diff. For short snippets (≤ 5 lines on both sides),
241+
* emit the full was prefixed by `-` and now by `+` inside a ```diff fence.
242+
* For longer snippets, emit a unified diff (only changed hunks) via `diff -u`.
243+
*/
244+
private static function renderSignatureDiff(string $was, string $now): string
245+
{
246+
$wasLines = explode("\n", $was);
247+
$nowLines = explode("\n", $now);
248+
249+
if (count($wasLines) <= 5 && count($nowLines) <= 5) {
250+
$body = '';
251+
foreach ($wasLines as $l) {
252+
$body .= '- ' . $l . "\n";
253+
}
254+
foreach ($nowLines as $l) {
255+
$body .= '+ ' . $l . "\n";
256+
}
257+
return " ```diff\n" . self::indentBlock($body) . " ```";
258+
}
259+
260+
$diff = self::computeUnifiedDiff($was, $now);
261+
return " ```diff\n" . self::indentBlock($diff) . " ```";
262+
}
263+
264+
private static function computeUnifiedDiff(string $was, string $now): string
265+
{
266+
$wasFile = tempnam(sys_get_temp_dir(), 'asc_was_');
267+
$nowFile = tempnam(sys_get_temp_dir(), 'asc_now_');
268+
try {
269+
file_put_contents($wasFile, $was . "\n");
270+
file_put_contents($nowFile, $now . "\n");
271+
$cmd = sprintf(
272+
'diff -U1 --label was --label now %s %s',
273+
escapeshellarg($wasFile),
274+
escapeshellarg($nowFile),
275+
);
276+
$out = shell_exec($cmd) ?? '';
277+
} finally {
278+
@unlink($wasFile);
279+
@unlink($nowFile);
280+
}
281+
282+
// Strip the file headers (--- was / +++ now) — the @@ hunks are what's useful.
283+
$lines = explode("\n", rtrim($out, "\n"));
284+
$kept = [];
285+
foreach ($lines as $line) {
286+
if (str_starts_with($line, '--- ') || str_starts_with($line, '+++ ')) {
287+
continue;
288+
}
289+
$kept[] = $line;
290+
}
291+
return implode("\n", $kept) . "\n";
292+
}
293+
294+
private static function indentBlock(string $block): string
295+
{
296+
$lines = explode("\n", rtrim($block, "\n"));
297+
return implode("\n", array_map(static fn (string $l) => ' ' . $l, $lines)) . "\n";
298+
}
194299
}

‎src/Snapshotter.php‎

Lines changed: 72 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -92,6 +92,7 @@ public function snapshot(array $files): array
9292
// enumeration the upfront cost is bounded by the number of changed
9393
// files; vendor is only touched lazily during parent resolution.
9494
foreach ($targetFiles as $abs => $relative) {
95+
$fileLines = self::readFileLines($abs);
9596
foreach ($this->extractClassFqcns($abs) as $fqcn) {
9697
try {
9798
$class = $reflector->reflectClass($fqcn);
@@ -103,7 +104,7 @@ public function snapshot(array $files): array
103104
continue;
104105
}
105106

106-
foreach ($this->collectFromClass($class, $relative) as $record) {
107+
foreach ($this->collectFromClass($class, $relative, $fileLines) as $record) {
107108
$records[] = $record;
108109
}
109110
}
@@ -204,9 +205,10 @@ private function buildReflector(array $targetAbsPaths): Reflector
204205
}
205206

206207
/**
208+
* @param list<string> $fileLines Source file split by line (0-indexed array, but file lines are 1-indexed).
207209
* @return iterable<array<string,mixed>>
208210
*/
209-
private function collectFromClass(ReflectionClass $class, string $relativeFile): iterable
211+
private function collectFromClass(ReflectionClass $class, string $relativeFile, array $fileLines): iterable
210212
{
211213
$classInternal = self::isInternalDoc($class->getDocComment());
212214
$kind = self::classKind($class);
@@ -229,6 +231,7 @@ private function collectFromClass(ReflectionClass $class, string $relativeFile):
229231
'line' => $class->getStartLine(),
230232
'signature' => $classSignature,
231233
'signature_hash' => hash('sha256', $classSignature),
234+
'signature_source' => self::extractDeclarationSource($fileLines, $class->getStartLine(), $class->getEndLine(), stripBody: true),
232235
];
233236

234237
foreach ($class->getImmediateMethods() as $method) {
@@ -247,6 +250,7 @@ private function collectFromClass(ReflectionClass $class, string $relativeFile):
247250
'line' => $method->getStartLine(),
248251
'signature' => $signature,
249252
'signature_hash' => hash('sha256', $signature),
253+
'signature_source' => self::extractDeclarationSource($fileLines, $method->getStartLine(), $method->getEndLine(), stripBody: !$method->isAbstract()),
250254
];
251255
}
252256

@@ -266,6 +270,7 @@ private function collectFromClass(ReflectionClass $class, string $relativeFile):
266270
'line' => $constant->getStartLine(),
267271
'signature' => $signature,
268272
'signature_hash' => hash('sha256', $signature),
273+
'signature_source' => self::extractDeclarationSource($fileLines, $constant->getStartLine(), $constant->getEndLine(), stripBody: false),
269274
];
270275
}
271276

@@ -285,10 +290,75 @@ private function collectFromClass(ReflectionClass $class, string $relativeFile):
285290
'line' => $property->getStartLine(),
286291
'signature' => $signature,
287292
'signature_hash' => hash('sha256', $signature),
293+
'signature_source' => self::extractDeclarationSource($fileLines, $property->getStartLine(), $property->getEndLine(), stripBody: false),
288294
];
289295
}
290296
}
291297

298+
/**
299+
* @return list<string>
300+
*/
301+
private static function readFileLines(string $absPath): array
302+
{
303+
$content = @file_get_contents($absPath);
304+
if ($content === false) {
305+
return [];
306+
}
307+
return explode("\n", $content);
308+
}
309+
310+
/**
311+
* Slice the source between $startLine and $endLine (1-based inclusive). When $stripBody is true,
312+
* truncate the slice at the opening `{` of the body — found by scanning forward from after the
313+
* declaration's closing `)` at depth 0. Falls back to the un-truncated slice if no balanced `{` is found.
314+
*
315+
* Indentation of the first line is removed from every line so the snippet renders without leading dead space.
316+
*
317+
* @param list<string> $fileLines
318+
*/
319+
private static function extractDeclarationSource(array $fileLines, int $startLine, int $endLine, bool $stripBody): string
320+
{
321+
if ($startLine < 1 || $endLine < $startLine || $startLine > count($fileLines)) {
322+
return '';
323+
}
324+
$slice = array_slice($fileLines, $startLine - 1, $endLine - $startLine + 1);
325+
$source = implode("\n", $slice);
326+
327+
if ($stripBody) {
328+
$depth = 0;
329+
$sawCloseParen = false;
330+
$len = strlen($source);
331+
for ($i = 0; $i < $len; $i++) {
332+
$ch = $source[$i];
333+
if ($ch === '(') {
334+
$depth++;
335+
} elseif ($ch === ')') {
336+
$depth--;
337+
if ($depth === 0) {
338+
$sawCloseParen = true;
339+
}
340+
} elseif ($ch === '{' && $depth === 0 && $sawCloseParen) {
341+
$source = rtrim(substr($source, 0, $i));
342+
break;
343+
}
344+
}
345+
}
346+
347+
// Dedent: leading whitespace common to first non-empty line.
348+
if (preg_match('/^([ \t]+)/', $source, $m) === 1) {
349+
$indent = $m[1];
350+
$lines = explode("\n", $source);
351+
foreach ($lines as $i => $line) {
352+
if (str_starts_with($line, $indent)) {
353+
$lines[$i] = substr($line, strlen($indent));
354+
}
355+
}
356+
$source = implode("\n", $lines);
357+
}
358+
359+
return rtrim($source);
360+
}
361+
292362
public static function isInternalDoc(?string $doc): bool
293363
{
294364
if ($doc === null) {

‎tests/DetectTest.php‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -233,8 +233,9 @@ public function testModifiedSignatureFlag(): void
233233
$body = $this->runDetect();
234234
$this->assertStringContainsString('### Modified API Surface', $body);
235235
$this->assertStringContainsString('L\\Foo::mut', $body);
236-
$this->assertStringContainsString('was: `public function mut(int $x): void`', $body);
237-
$this->assertStringContainsString('now: `public function mut(int $x, int $y): void`', $body);
236+
$this->assertStringContainsString("```diff", $body);
237+
$this->assertStringContainsString('- public function mut(int $x): void', $body);
238+
$this->assertStringContainsString('+ public function mut(int $x, int $y): void', $body);
238239

239240
// Explicit opt-out → empty body.
240241
$this->assertSame('', $this->runDetect(['SHOW_MODIFIED' => 'false']));

0 commit comments

Comments
 (0)