Skip to content

docs(comments): drop comments that carry nothing the code does not - #140

Merged
Ilyes512 merged 1 commit into
refactor/GH-109-ndjson-table-recordsfrom
chore/prune-unnecessary-comments
Sep 4, 2026
Merged

docs(comments): drop comments that carry nothing the code does not#140
Ilyes512 merged 1 commit into
refactor/GH-109-ndjson-table-recordsfrom
chore/prune-unnecessary-comments

Conversation

@Ilyes512

@Ilyes512 Ilyes512 commented Sep 4, 2026

Copy link
Copy Markdown
Member

Top of the stack on #135. Comments only — no behaviour changes, so no test or doc updates accompany it.

The repo's convention is a comment that explains why, and most of them earn their place. This prunes the ones that do not, across the stack's additions and the code that was already there.

Diff narration. templateRow explaining that Name "was a JSON key only because it was also a heading", and executeTemplate naming the Phase 7 and Phase 8 that introduced it. That belongs in a commit message; in the code the next reader has no idea what "was" refers to.

Restatement. // Check file content. above os.ReadFile, // Build dependency graph among computed keys. above the loop that builds it, the numbered 1–5 walkthrough of Execute's walk function, // validateGitHubName checks that a GitHub owner or repo name is valid., and three TestX verifies that… comments that only spell out their own test name. The two "copy so the caller's map is not mutated" notes in context.go say what the function doc two paragraphs up already promises.

Duplication. The rationale for splitting display strings from row values was written out four times, once per site the stack touched. It stays where the split is defined — Column and TableData — and the other sites point at it rather than restating it, so rewording it later is one edit instead of four. Same for the CI-hang rationale, which now lives on prompter.cannotPrompt alone.

Where a comment mixed a real constraint into a restatement, the constraint is kept and the restatement dropped: hasEmptySegment's "conditional directory exclusion" becomes the subtree exclusion it actually means, and validate.go keeps the non-obvious half about Template.Referenced already covering computed expressions.

Checks

task test, go test -race, go test -tags=integration -race ./internal/cmd/... and go build all pass.

One note: the local task lint reports 6 pre-existing SA5011 staticcheck findings in internal/cmd/metadata_test.go, a file this PR does not touch. They reproduce identically on #135's head and CI's golangci-lint is green there, so it is local toolchain drift rather than anything introduced here.


Stack created with GitHub Stacks CLIGive Feedback 💬

The repo's convention is a comment that explains why, and most of them earn
their place. These do not.

Three kinds are removed. Diff narration — templateRow explaining that `Name`
"was a JSON key only because it was also a heading", and executeTemplate
naming the Phase 7 and Phase 8 that introduced it — records what changed, not
what is. That belongs in a commit message; in the code the next reader has no
idea what "was" refers to.

Restatement — "Check file content." above os.ReadFile, "Build dependency graph
among computed keys." above the loop that builds it, the numbered walkthrough
of Execute's walk function, "validateGitHubName checks that a GitHub owner or
repo name is valid." The two "copy so the caller's map is not mutated" notes
in context.go say what the function doc two paragraphs up already promises.

Duplication — the rationale for splitting display strings from row values was
written out four times, once per site it touched. It is kept where the split
is defined (Column and TableData) and the other sites point at it rather than
restating it, so rewording it later is one edit and not four.

Where a comment mixed a real constraint into a restatement the constraint is
kept and the restatement dropped: hasEmptySegment's "conditional directory
exclusion" becomes the subtree exclusion it actually means, and validate.go
keeps the non-obvious half about Template.Referenced covering computed
expressions.

No behaviour changes, so no test or doc updates accompany this.
@Ilyes512
Ilyes512 force-pushed the chore/prune-unnecessary-comments branch from 68df26f to b80b430 Compare September 4, 2026 08:45
@Ilyes512
Ilyes512 merged commit 2ae6e40 into main Sep 4, 2026
5 checks passed
@Ilyes512
Ilyes512 deleted the chore/prune-unnecessary-comments branch September 4, 2026 10:10
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