Skip to content

feat: summarise findings by default and keep per-occurrence facts out… #7

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Heyosseus merged 1 commit into from
Aug 11, 2026
Merged

feat: summarise findings by default and keep per-occurrence facts out… #7

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
162 changes: 139 additions & 23 deletions src/Console/Renderers/ReportRenderer.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,46 +10,142 @@
use Symfony\Component\Console\Output\OutputInterface;

/**
* Prints a migration analysis: findings grouped by rule and risk, worst first, then
* the tally underneath.
* Prints a migration analysis, in one of two modes.
*
* ## Why grouped, and why the prose is printed once
* ## Summary, by default
*
* The first version printed every finding in full, and on a real application that
* was unusable: 124 cascading-foreign-key findings meant the same five-line
* explanation and three-line remediation printed 124 times — about a thousand lines
* of identical prose. An explanation is a property of the *rule*, not of each place
* the rule fired, and printing it per occurrence buried the one thing the reader
* needed, which is the list of places.
* A count per risk level with the rules contributing to it, the worst few findings,
* and where to find the rest. Its length does not depend on how many findings there
* are, which is the entire point: this renderer previously printed 693 lines against
* a real 170-migration application, and output that long is not read, it is scrolled
* past — so the findings in it are worth nothing however correct they are.
*
* So a group whose findings all share the same explanation prints it once and then
* lists the occurrences as one line each. A group whose explanations genuinely
* differ — `drop-column` names the indexes on each column, `add-index` quotes each
* table's row count — prints the first few in full and says how many it held back.
* The distinction is made by comparing the text, so it stays right as rules change.
* ## Detail, with `-v`
*
* Three things are never abbreviated away: the risk level, whether the operation is
* destructive, and whether the parser understood the whole file.
* Every finding, grouped by rule, risk and the prose it carries, so a shared
* explanation is printed once for the whole group rather than once per finding.
*
* That grouping only works because rules keep per-occurrence facts out of their
* explanations and put them in {@see MigrationFinding::$context} instead. When they
* did not — when `drop-column` appended each table's row count to its paragraph —
* every finding became a group of one and the detail view was ten times longer than
* it needed to be.
*
* Four things are never abbreviated away, in either mode: the risk tally, the count
* of accepted findings, whether the database was reachable, and what the parser
* could not read. They are what a reader would not know to ask for, and dropping
* them is how a summary becomes a lie.
*/
final class ReportRenderer
{
/** How many occurrences a group shows before it starts counting instead. */
private const int PREVIEW = 3;

public function render(OutputInterface $output, MigrationReport $report): void
/**
* @param string $command What to point the reader at for the full detail.
*/
public function render(OutputInterface $output, MigrationReport $report, string $command = 'difflock:lint'): void
{
if ($report->migrations === []) {
$this->nothingFound($output);

return;
}

// Summary unless asked otherwise. On a real application this is the
// difference between twenty lines and seven hundred, and seven hundred lines
// of correct findings are worth nothing because nobody reads them.
$output->isVerbose()
? $this->detail($output, $report)
: $this->summary($output, $report, $command);

$this->warnings($output, $report);
}

/** Every finding, grouped. What `-v` gives you. */
public function detail(OutputInterface $output, MigrationReport $report): void
{
foreach ($this->grouped($report->findings) as $group) {
$this->group($output, $group, $output->isVerbose());
$this->group($output, $group, true);
}

$this->tally($output, $report);
$this->warnings($output, $report);
}

/**
* The bounded view: what was found, the worst of it, and where the rest is.
*
* Length is independent of the number of findings — one line per risk level that
* has any, plus a fixed-size worst list. The tally, the accepted count and the
* parser warnings are never abbreviated away, because they are the things a
* reader would not know to ask for.
*/
public function summary(OutputInterface $output, MigrationReport $report, string $command): void
{
$summary = $report->summary();

if ($summary->total === 0) {
$output->writeln(' <fg=green>✓</> Nothing to report.');
$output->writeln('');
$this->analysed($output, $report);

return;
}

foreach (array_reverse(RiskLevel::ascending()) as $level) {
$count = $summary->count($level);

if ($count === 0) {
continue;
}

$output->writeln(
' <fg='.$level->colour().';options=bold>'.$level->glyph().' '.Text::pad($level->label(), 9).'</>'
.str_pad((string) $count, 4, ' ', STR_PAD_LEFT)
.' <fg=gray>'.implode(', ', $this->rulesAt($report->findings, $level)).'</>',
);
}

$output->writeln('');
$output->writeln(' <options=bold>Worst</>');

foreach (array_slice($report->findings, 0, self::PREVIEW) as $finding) {
$output->writeln(' '.$finding->message);
$output->writeln(' <fg=gray>'.$this->where($finding).'</>');
}

$output->writeln('');
$this->analysed($output, $report);

foreach ([
$command.' -v' => 'every finding in full',
$command.' --rule=NAME' => 'one rule at a time',
'difflock:report' => 'a shareable HTML report',
] as $invocation => $describes) {
$output->writeln(' <fg=gray>→ '.Text::pad($invocation, 30).$describes.'</>');
}

$output->writeln('');
}

/**
* The rules contributing to a level, so the tally says what kind of problem it is
* rather than only how much of it there is.
*
* @param list<MigrationFinding> $findings
* @return list<string>
*/
private function rulesAt(array $findings, RiskLevel $level): array
{
$rules = [];

foreach ($findings as $finding) {
if ($finding->risk === $level) {
$rules[$finding->rule] = true;
}
}

return array_keys($rules);
}

/**
Expand Down Expand Up @@ -136,10 +232,16 @@ private function prose(OutputInterface $output, MigrationFinding $finding, strin
*/
private function occurrences(OutputInterface $output, array $group, bool $verbose): void
{
// `-v` means every finding — the summary is where brevity lives now, so
// truncating here as well would leave no way to see the whole picture.
$shown = $verbose ? $group : array_slice($group, 0, self::PREVIEW);

foreach ($shown as $finding) {
$output->writeln(' '.$finding->message.' <fg=gray>'.$this->where($finding).'</>');

if ($finding->context !== null) {
$output->writeln(' <fg=gray>'.$finding->context.'</>');
}
}

$this->remainder($output, count($group) - count($shown), $group[0]->rule);
Expand Down Expand Up @@ -196,7 +298,6 @@ private function nothingFound(OutputInterface $output): void
private function tally(OutputInterface $output, MigrationReport $report): void
{
$summary = $report->summary();
$analyzed = count($report->migrations);

$output->writeln(' <options=bold>Risk</>');

Expand All @@ -210,10 +311,25 @@ private function tally(OutputInterface $output, MigrationReport $report): void
}

$output->writeln('');
$output->writeln(' <fg=gray>'.$analyzed.' migration'.($analyzed === 1 ? '' : 's').' analysed.</>');
$this->analysed($output, $report);
}

/**
* How much was looked at, and what was held back.
*
* Printed in both modes. An accepted backlog nobody can see is a backlog that
* quietly becomes permanent.
*/
private function analysed(OutputInterface $output, MigrationReport $report): void
{
$analyzed = count($report->migrations);
$total = $report->summary()->total;

$output->writeln(
' <fg=gray>'.$analyzed.' migration'.($analyzed === 1 ? '' : 's').' analysed'
.($total === 0 ? '' : ' · '.$total.' finding'.($total === 1 ? '' : 's')).'.</>',
);

// Never silent: an accepted backlog that nobody can see is a backlog that
// quietly becomes permanent.
if ($report->accepted !== []) {
$output->writeln(
' <fg=gray>'.count($report->accepted).' previously accepted finding'
Expand Down
2 changes: 2 additions & 0 deletions src/Migration/MigrationContext.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,7 @@ public function finding(
bool $destructive = false,
bool $reversible = true,
?Operation $operation = null,
?string $context = null,
): MigrationFinding {
return new MigrationFinding(
rule: $rule,
Expand All @@ -113,6 +114,7 @@ public function finding(
reversible: $reversible,
line: $operation->line ?? $this->statement->line,
conditional: $operation->conditional ?? $this->statement->conditional,
context: $context,
);
}
}
7 changes: 7 additions & 0 deletions src/Migration/MigrationFinding.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,11 @@
* @param string|null $subject The column, index or constraint the finding is about.
* @param bool $conditional Whether the operation sits inside an `if` or a loop, so it
* may not run at all. The message is phrased accordingly.
* @param string|null $context A short phrase about *this* occurrence — `82,325 rows`,
* `covered by users_email_index`. Everything that varies
* between two findings of the same rule belongs here, so
* that the explanation can stay invariant and be printed
* once for the whole group instead of once per finding.
*/
public function __construct(
public string $rule,
Expand All @@ -47,6 +52,7 @@ public function __construct(
public bool $reversible = true,
public ?int $line = null,
public bool $conditional = false,
public ?string $context = null,
) {}

/**
Expand Down Expand Up @@ -95,6 +101,7 @@ public function toArray(): array
'reversible' => $this->reversible,
'conditional' => $this->conditional,
'line' => $this->line,
'context' => $this->context,
];

if ($this->subject !== null && $this->subjectType !== Subject::None) {
Expand Down
5 changes: 1 addition & 4 deletions src/Migration/Rules/AddIndexRule.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,10 +77,6 @@ private function finding(MigrationContext $context, Operation $operation): Migra
.'takes a lock, and for how long, depends on the database engine and version — Difflock '
.'does not know which applies here, so it judges by size alone.';

$explanation .= $size === null
? ' The size of the table could not be determined.'
: ' The table holds '.$size.'.';

if ($unique) {
$explanation .= ' A unique index also fails outright if the existing rows contain '
.'duplicates, which stops the migration partway through.';
Expand All @@ -97,6 +93,7 @@ private function finding(MigrationContext $context, Operation $operation): Migra
subjectType: Subject::Index,
reversible: $context->reversible(),
operation: $operation,
context: $size ?? 'table size could not be determined',
);
}

Expand Down
9 changes: 6 additions & 3 deletions src/Migration/Rules/AddNotNullColumnRule.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,9 @@ private function finding(
$column = Blueprint::columnsOf($operation)[0] ?? '<unresolved>';
$table = $context->tableName() ?? '<unresolved>';

// Three genuinely different arguments, not three renderings of one — so these
// stay in the explanation and collapse to three groups. The row count itself
// varies per finding and goes in the context line.
[$risk, $because] = match (true) {
$rows === null => [
RiskLevel::Medium,
Expand All @@ -78,9 +81,8 @@ private function finding(
],
$rows > 0 => [
RiskLevel::High,
'The table holds '.($context->database->describeSize($table) ?? $rows.' rows')
.', and every one of them needs a value the migration does not supply. Most '
.'engines refuse the statement rather than inventing one.',
'The table is not empty, and every existing row needs a value the migration does not '
.'supply. Most engines refuse the statement rather than inventing one.',
],
default => [
RiskLevel::Low,
Expand All @@ -100,6 +102,7 @@ private function finding(
subjectType: Subject::Column,
reversible: $context->reversible(),
operation: $operation,
context: $context->database->describeSize($table),
);
}
}
30 changes: 20 additions & 10 deletions src/Migration/Rules/DropColumnRule.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,20 +64,29 @@ private function column(MigrationContext $context, Operation $operation, string
{
$table = $context->tableName() ?? '<unresolved>';

// Invariant: the same sentence for every dropped column, so the renderer can
// print it once for the whole group. Everything specific to *this* column —
// how many rows, what is built on it — goes in the context line.
$explanation = 'Dropping a column destroys the values in it. A `down()` that adds the column '
.'back gives you the column and not one row of what was in it.';

if (str_starts_with($operation->method, 'dropConstrainedForeignId')) {
$explanation .= ' This form also drops the foreign key constraint on the column, so '
.'anything relying on it for referential integrity loses it.';
}

$facts = [];

$size = $context->database->describeSize($context->tableName());

if ($size !== null) {
$explanation .= ' The table holds '.$size.'.';
$facts[] = $size;
}

$explanation .= $this->dependants($context->liveTable(), $column);
$dependants = $this->dependants($context->liveTable(), $column);

if (str_starts_with($operation->method, 'dropConstrainedForeignId')) {
$explanation .= ' This also drops the foreign key constraint on the column, so anything '
.'relying on that constraint for referential integrity loses it.';
if ($dependants !== '') {
$facts[] = $dependants;
}

return $context->finding(
Expand All @@ -92,6 +101,7 @@ private function column(MigrationContext $context, Operation $operation, string
destructive: true,
reversible: false,
operation: $operation,
context: $facts === [] ? null : implode(' · ', $facts),
);
}

Expand Down Expand Up @@ -123,20 +133,20 @@ private function dependants(?Table $table, string $column): string
}
}

$notes = '';
$notes = [];

if ($indexes !== []) {
$notes .= ' It is covered by '.$this->list(array_map(
$notes[] = 'covered by '.$this->list(array_map(
static fn (Index $index): string => $index->name,
$indexes,
)).', which the drop takes with it.';
));
}

if ($keys !== []) {
$notes .= ' The foreign key '.$this->list($keys).' is built on it.';
$notes[] = 'foreign key '.$this->list($keys).' built on it';
}

return $notes;
return implode(', ', $notes);
}

private function unresolved(MigrationContext $context, Operation $operation): MigrationFinding
Expand Down
Loading
Loading