Skip to content

CLAUDE.md: comments should be concise, explained at first mention, cross-referenced with above/below - #74

Merged
jnasbyupgrade merged 6 commits into
Postgres-Extensions:masterfrom
jnasbyupgrade:claude-md-comment-conciseness
Aug 3, 2026
Merged

CLAUDE.md: comments should be concise, explained at first mention, cross-referenced with above/below#74
jnasbyupgrade merged 6 commits into
Postgres-Extensions:masterfrom
jnasbyupgrade:claude-md-comment-conciseness

Conversation

@jnasbyupgrade

Copy link
Copy Markdown
Contributor

Prompted by a review pass on PR #54's test-harness comments, which had drifted into unnecessary length and repetition. Two rules worth keeping for every future comment, not just that PR: humans have limited context too, so say the point in as few words as it needs; and a comment referencing another spot in the same file should say whether that spot is above or below, so the reader isn't left searching both directions.

…d cross-reference with above/below

Prompted by a review pass on PR Postgres-Extensions#54's test-harness comments, which had
drifted into unnecessary length and repetition. Two rules worth keeping
for every future comment, not just that PR: humans have limited context
too, so say the point in as few words as it needs; and a comment
referencing another spot in the same file should say whether that spot
is above or below, so the reader isn't left searching both directions.
@coderabbitai

coderabbitai Bot commented Aug 3, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 2a62f17d-718b-46b0-b6ac-49ba1c2f1d86

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions

github-actions Bot commented Aug 3, 2026

Copy link
Copy Markdown

Claude finished @jnasbyupgrade's task in 1m 19s —— View job


Review: PR #74 — CLAUDE.md comment conciseness rules

  • Gather context (PR diff, CLAUDE.md instructions)
  • Review the diff for correctness, clarity, consistency with rest of CLAUDE.md
  • Post review feedback

Diff is a clean, doc-only addition (+8/-0) under ## Code Style### Comments in CLAUDE.md:

Be concise — humans have limited context too, not just AI. Say the point in as few words
as it needs, not as many as the topic could support.

Explain something at the *first* place it's mentioned, not somewhere later that assumes
context the reader hasn't reached yet (exceptions need a reason). When a comment points
to another spot in the same file, say `above` or `below` so the reader doesn't have to
search both directions.

Findings:

  • Placement is correct. It sits in the ### Comments subsection alongside the existing SQL block-comment rule, and reads fine as a general comment-writing rule (not SQL-specific), which is consistent with how it'll be applied in practice.
  • Self-consistent with existing usage. The file's only prior same-file cross-reference is in ## Bug Fixes (CLAUDE.md:37): ("same as above") — already following the above/below convention this PR formalizes. No existing text needs to change to comply.
  • Minor overlap, not a blocker: ## Bug Fixes (CLAUDE.md:33-37) already says "keep it concise — no novels" and "never repeat the same comment verbatim... reference it." The new rules generalize the same intent to all comments repo-wide. There's conceptual overlap between the two sections, but they're scoped differently (bug-fix comments vs. all comments) and short enough that I wouldn't merge them — just flagging in case a future pass wants to cross-link them.
  • No factual errors, no bare PR/issue-number references, no formatting issues found.

Overall: straightforward, well-scoped documentation change. No changes requested.

Distilled from PR Postgres-Extensions#54's back-and-forth over where TEST_SCHEMA and the
update-testing check should live: a make-level dimension runs locally
and never costs a container/job, while a ci.yml-level one always does
unless deliberately batched -- which is its own added complexity. The
one real exception is when a dimension's whole point is isolation
(pg_tle needing its own cluster), not just another combination to run.
@github-actions

github-actions Bot commented Aug 3, 2026

Copy link
Copy Markdown

Claude finished @jnasbyupgrade's task in 9m 19s —— View job


Review: PR #74 — CLAUDE.md comment conciseness rules

  • Gather context (PR diff, CLAUDE.md instructions)
  • Review the diff (both commits: comment-conciseness rules + test-matrix-placement rule)
  • Post review feedback

The PR now contains two commits (the second was pushed after the initial automated review, which only covered the first):

1. ### Comments additions (## Code Style) — be concise, explain at first mention, use above/below for same-file cross-references. Well-scoped, consistent with the existing above/below usage already in ## Bug Fixes (CLAUDE.md around line 37), no factual issues.

2. New ## Where a test-matrix dimension belongs section — states a preference for make over ci.yml for new test-matrix dimensions, carving out isolation (e.g. pg_tle's dedicated cluster) as the exception.

  • Verified this is accurate against the repo: pg_tle does get its own dedicated, isolated jobs in .github/workflows/ci.yml (separate shared_preload_libraries cluster, its own matrix), so the isolation exception is a real, correctly-described case, not hypothetical.
  • Placement (right after the existing ## GitHub CI section, before ## Build/test system (pgxntool)) is reasonable — it's CI/test-authoring guidance grouped with other CI content.
  • No conflict with the later ## CI: PostgreSQL version support / ### CI jobs section, which documents the existing matrix rather than where new dimensions should go.

Both commits are self-consistent, doc-only, and match the conventions elsewhere in the file. No changes requested.

…le list

It was never a valid example -- cat_tools' control file pins schema =
'cat_tools' with relocatable = false, so there's no schema dimension to
put anywhere, in make or ci.yml. The general make-vs-ci.yml principle
itself is unaffected; it just needed a real example in its place.
@github-actions

github-actions Bot commented Aug 3, 2026

Copy link
Copy Markdown

Claude finished @jnasbyupgrade's task in 1m 9s —— View job


Review: PR #74 — CLAUDE.md comment conciseness rules

  • Gather context (PR diff, prior review comments)
  • Review new commit since last automated review
  • Post review feedback

Since the last automated pass (which covered the first two commits), a third commit was pushed:

758e246 — drop schema-targeting from the test-matrix-dimension example list

Removes schema targeting from the example dimensions listed in ## Where a test-matrix dimension belongs (CLAUDE.md:10), leaving PostgreSQL major, update path, pg_tle vs. filesystem. Verified the commit's stated rationale against the repo: cat_tools.control pins schema = 'cat_tools' and relocatable = false, so there genuinely is no schema-targeting dimension to place anywhere — the removed example was invalid. The surrounding sentence still reads cleanly and the general make-vs-ci.yml principle is untouched.

All three commits together (comment-conciseness rules, test-matrix-placement section, this example fix) remain doc-only, self-consistent, and match the file's existing conventions — e.g. ## Bug Fixes (CLAUDE.md:37) already uses the ("same as above") pattern the new rules formalize. No factual errors, no bare PR/issue-number references, no formatting issues.

No changes requested.

Prompted directly by PR Postgres-Extensions#54/Postgres-Extensions#75 turning into hard-to-follow walls of
text (a real contributor to a costly mid-PR mix-up), versus Postgres-Extensions#74's own
description staying easy to follow despite covering real backstory.
Length past the opening isn't the problem -- an unstructured mass of
prose is.
@github-actions

github-actions Bot commented Aug 3, 2026

Copy link
Copy Markdown

Claude finished @jnasbyupgrade's task in 1m 14s —— View job


Review: PR #74 — CLAUDE.md comment conciseness rules

  • Gather context (PR diff, prior review comments)
  • Check for new commits since last automated review
  • Post review feedback

A fourth commit (d702762 — "PR descriptions can be long, but need real structure") was pushed since the last automated pass, which covered the first three. Reviewing what's new:

d702762 adds a bullet to ## Pull request descriptions (CLAUDE.md:79-82):

Length past the opening is fine — backstory and detail are often worth keeping. Being hard to scan is the actual problem: if there's enough detail to justify it, give the rest real structure (headers, bullet lists, separate sections for "what changed" vs. "how it was verified"), not one undifferentiated block of prose.

  • Placement is correct — sits with the other bullets in the same section, after the existing hard-wrap rule.
  • No conflict with the "Do NOT hard-wrap paragraphs" bullet just above it: that rule governs how individual paragraphs are written (single long line, no 80-col wrapping) for GitHub's PR-description rendering; this new bullet is about overall document structure (headers/sections) once there's enough content to need it. Different concerns, same section — fits fine.
  • Reasonable complement to the "make the opening stand alone" / "keep incidental changes out of the opening" bullets already there: those constrain the opening, this one gives guidance for everything after it, so the section now covers both halves of a PR description.
  • No factual errors, no bare issue/PR references, no formatting issues.

All four commits together (comment-conciseness rules, test-matrix-placement section, the schema-targeting example fix, and this PR-description-structure bullet) remain doc-only, self-consistent, and match the file's existing conventions.

No changes requested.

Concrete exception worth naming: already mid-explanation of A, need to
mention B (which also needs explaining) -- fully explaining B right
there often confuses A's explanation more than deferring B would.
@github-actions

github-actions Bot commented Aug 3, 2026

Copy link
Copy Markdown

Claude finished @jnasbyupgrade's task in 1m 29s —— View job


Review: PR #74 — CLAUDE.md comment conciseness rules

  • Gather context (PR diff, prior review comments)
  • Check for new commits since last automated review
  • Review the diff for correctness, clarity, consistency
  • Post review feedback

One new commit since the last automated pass:

906a1bb — "the first-mention rule isn't absolute"

Softens the first-mention rule added earlier in this PR (### Comments, CLAUDE.md:188-193 at HEAD): "Usually, explain something at the first place it's mentioned... but this isn't absolute. One real exception: you're already in the middle of explaining A, and need to mention B (which also needs its own explanation) — stopping to fully explain B right there often confuses the explanation of A more than deferring B... Use judgment."

  • This directly addresses the parenthetical (exceptions need a reason) from the original wording by naming the concrete exception, which is a genuine improvement — a rule with an unstated "exceptions need a reason" is weaker than one that names the actual case.
  • The exception is well-motivated and matches a real writing problem (nested-explanation derailment), not a hypothetical.
  • No contradiction with the rest of ### Comments — the concision rule and the above/below cross-reference rule are untouched and still apply.
  • Grammar/wording is clean; "Use judgment" as a closer is consistent with this file's style elsewhere (it doesn't try to enumerate every case).

All five commits together (comment-conciseness + first-mention rules, the test-matrix-placement section, the schema-targeting example fix, and the PR-description-structure bullet) remain doc-only, self-consistent, and match the file's existing conventions (e.g. ## Bug Fixes at CLAUDE.md:48 already uses the ("same as above") pattern the new rules formalize). No factual errors, no bare PR/issue-number references, no formatting issues.

Aside (not a PR issue): the local working tree has an uncommitted modification to CLAUDE.md that strips out all of this PR's additions — visible via git status/git diff HEAD. This appears to be an artifact of the review sandbox's setup (the CLAUDE.md snapshot loaded into context also predates these commits) rather than anything in the PR itself, so it hasn't affected this review, but flagging it in case it's unexpected on your end.

No changes requested.

…text-leak in PR descriptions

Two fixes: the test-matrix-dimension example list didn't need full
enumeration (an "etc." example works fine) and implied ci.yml would
never grow, plus overstated isolation as the ONLY reason to prefer it
-- it's a real tradeoff, just one to bias against by default. Separately,
the PR-description structure rule covered organization but not plain
wordiness or including discussion history that never resulted in
merged code -- both worth calling out on their own.
@github-actions

github-actions Bot commented Aug 3, 2026

Copy link
Copy Markdown

Claude finished @jnasbyupgrade's task in 1m 43s —— View job


Review: PR #74 — CLAUDE.md comment conciseness rules

  • Gather context (PR diff against origin/master, prior review comments)
  • Review all 6 commits, focusing on bd3410a (not covered by earlier automated passes)
  • Post review feedback

All 6 commits are doc-only changes to CLAUDE.md (+29/-0). Summary of what's new since the last automated review (which covered through 906a1bb):

bd3410a — "stop over-enumerating examples, and call out verbosity/context-leak in PR descriptions"

  1. Reworks ## Where a test-matrix dimension belongs: trims the enumerated example list to PostgreSQL major, UPDATE vs. CREATE EXTENSION, etc. and softens "the one reason to prefer ci.yml" to "one good reason... not the only possible one." Verified against .github/workflows/ci.yml: pg_tle still gets its own dedicated cluster/job (lines ~749-822, explicit comment "A dedicated cluster, never shared with the other jobs... pg_tle requires shared_preload_libraries"), so the isolation example remains accurate.
  2. Adds a new PR-description bullet calling out verbosity and "leaking" pre-merge process/discussion into the description as distinct failure modes from the structure issue the prior bullet addressed.

Minor nit (non-blocking): the reworded test-matrix paragraph's closing clause — "weigh the actual tradeoff, just start from a bias toward make" — largely restates the sentence's own opening ("It's a real tradeoff... bias toward putting a new dimension in make"). Slightly ironic given this very PR is adding a "say the point in as few words as it needs" rule, but it's a style nit, not a correctness issue.

Everything else (previously reviewed, still holds):

  • ### Comments additions (concision, first-mention-with-exception, above/below cross-referencing) are well-scoped and consistent with the existing ("same as above") usage in ## Bug Fixes (CLAUDE.md:37).
  • ## Where a test-matrix dimension belongs placement (after ## GitHub CI, before ## Build/test system) is reasonable and doesn't conflict with the later ### CI jobs section, which documents the existing matrix rather than prescribing where new dimensions go.
  • The dropped schema targeting example was correctly removed — cat_tools.control pins schema = 'cat_tools' / relocatable = false, so there's no schema-targeting dimension to place.

No factual errors, no bare PR/issue-number references, no formatting issues. No changes requested.

Aside (not a PR issue): the sandbox's local working tree has an uncommitted modification to CLAUDE.md that strips this PR's additions back out — this is a stale-snapshot artifact of the review environment (confirmed via git show HEAD:CLAUDE.md, which has all the additions correctly committed), not anything in the PR itself.

@jnasbyupgrade
jnasbyupgrade merged commit 47f0ded into Postgres-Extensions:master Aug 3, 2026
12 checks passed
@jnasbyupgrade
jnasbyupgrade deleted the claude-md-comment-conciseness branch August 3, 2026 22:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant