Skip to content

Trim KDoc to contract plus counterexample #284

Description

Do this last: earlier issues delete or move a large share of the text this one would otherwise edit.

Why

44% of the lines in generator/src/main are comments (6151 of 14132). Ten files are more comment
than code:

File Comment share
SqlIdentifiers.kt 71% (161 of 226 lines)
SqlStarItem.kt 64%
SqlOutputClause.kt 60%
NodeTreeProvenanceExpression.kt 57%
NodeTreeNullabilityAnalyzer.kt 56%
SqlLexer.kt 55%
PgNodeExpression.kt 55%
Model.kt 54%
NodeTreeProvenance.kt 53%
ColumnNullabilityAnalyzer.kt 52%

Most of the mass is not contract. It is argument: why an alternative was rejected, which other
function already handles a case, what a previous bug looked like, and cross-references to other KDoc
("see X's KDoc for why"). Those paragraphs go stale the moment the referenced code moves, and a reader
who wants the contract has to find it inside the essay.

The rule

Each KDoc block keeps at most three things:

  1. The contract. What the function accepts, what it returns, what null/empty means. One to
    three sentences. Every optional or nullable parameter says what null means (project rule).
  2. The counterexample. When the contract encodes a non-obvious PostgreSQL fact, one concrete SQL
    snippet that shows it, in backticks, with the server version it was checked on. Example, replacing
    14 lines on foldAsciiCase: "PostgreSQL folds only ASCII A-Z: on 18.4, with a column "ü",
    SELECT Ü FROM t fails with column "Ü" does not exist." A counterexample stays true as long as
    PostgreSQL does, and a reader can re-run it.
  3. The invariant the caller must uphold, when there is one ("rawIdentifier must be the source
    text with quotes intact").

Everything else is cut, not condensed:

  • "Why not X" paragraphs. If the rejected alternative produced a wrong answer, that wrong answer is
    the counterexample (rule 2); the narrative around it goes.
  • Pointers to other KDoc or to test names ("see FooTest's bar case"). These are the fastest to
    rot. The counterexample carries the fact instead.
  • Restatements of what the code visibly does.
  • Descriptions of a previous bug ("before this fix...", "this replaced a pg_depend name-join...").
    Git history holds those.
  • Inline // comments that repeat a KDoc paragraph from another function ("See analyzeNodeTree's
    identical guard").

Procedure

Work one file at a time, largest comment share first. For each KDoc block:

  1. Read the block and the code it documents.
  2. Write the contract sentence(s). If you cannot state the contract in three sentences, the function
    is doing too much; note it and move on, do not paper over it with prose.
  3. Scan the old text for live-verified PostgreSQL facts (anything that quotes an error message, a
    version, or a result). For each, decide: is it the fact the contract rests on? Keep it as a
    counterexample. Is it a supporting anecdote? Cut it. Before cutting a fact, grep
    generator/src/test for its SQL or error text; if no test pins it and the fact is load-bearing,
    keep it as the counterexample rather than cutting it.
  4. Delete every cross-reference sentence whose only content is "see X".
  5. Fix stale references while there: PgNodeExpression.kt:263 names a non-existent SqlUtils.kt
    (Remove model fields nothing populates or reads #273 may already have fixed it).

Do not change any code. Do not rename anything. Do not reformat surviving sentences beyond what
spotlessApply requires.

What "done" looks like, per file

Targets, not hard limits; a file that lands above target with every block passing the rule is fine,
and a file that hits the target by deleting a load-bearing counterexample is not.

File Target comment share
SqlIdentifiers.kt ≤ 40%
SqlStarItem.kt ≤ 40%
SqlOutputClause.kt ≤ 40%
SqlLexer.kt ≤ 40%
NodeTreeProvenanceExpression.kt, NodeTreeProvenance.kt ≤ 40%
NodeTreeNullabilityAnalyzer.kt, ColumnNullabilityAnalyzer.kt ≤ 45% (dense semantics; every safe-list rule keeps its counterexample)
PgNodeExpression.kt, Model.kt ≤ 45% (@property lines are contract)
NeverNullSafeLists.kt leave as is; its per-entry audit bullets are the counterexamples

Measure with:

awk '{t++; s=$0; sub(/^[ \t]+/,"",s); if (s ~ /^(\*|\/\*\*|\/\*|\*\/|\/\/)/) c++} END{printf "%d%%\n", c*100/t}' FILE

Test design

No behavior changes, so no new tests. The check that nothing load-bearing was lost is step 3 of the
procedure: a fact is cut only if a test already pins it or it is not what the contract rests on.

Acceptance criteria

  1. git diff --stat shows changes only inside comments (git diff -w --ignore-blank-lines on each
    file, with comment lines stripped, is empty). Concretely: ./gradlew :generator:compileKotlin
    produces byte-identical class files before and after, or the reviewer spot-checks that every hunk
    is inside a /** */ or // block.
  2. Every file in the table is at or under its target, or the PR description names the file and the
    counterexample(s) that justified staying above it.
  3. No KDoc in generator/src/main contains the phrase "see .* KDoc" or references a test class by
    name (git grep -n -E "Test\]|Test's|see .*KDoc" generator/src/main returns nothing).
  4. ./gradlew :generator:check passes.

Files

Every file in the table above.


Conventions every issue inherits

  • Repo root: /Volumes/Code/3rd-party/norm. Module under change is almost always generator/.
  • Style: 2-space indent, 120 columns, full words in identifiers (parameter, not param), no
    section-separator comments, @Nested classes group tests. Format with ./gradlew spotlessApply.
  • TDD: tests are written first or alongside. For a behavior-preserving refactor the existing suite is
    the pin; each issue says which tests must also be added.
  • Golden files under test-scenarios*/ are never hand-edited. A refactor is behavior-preserving only
    if ./gradlew :gradle-plugin:generateGoldenFiles leaves git status --porcelain test-scenarios test-scenarios-frameworks empty.
  • Verification for a generator/ change: ./gradlew :generator:check :gradle-plugin:test (Docker
    required). Then the golden regeneration check above.
  • Commit message explains the design decision, not the diff.
  • Never run ./gradlew clean or disable the configuration/build cache to "fix" a build problem.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    enhancementNew feature or request

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions