Repository navigation
Fail the build on issue numbers and history in comments - #944
Merged
Merged
Conversation
CommentConventionsTest scans the comments in src/main and src/test and fails on an issue number outside a `// TODO <change> in X.0.0 (#N)`, on history narration such as "as before" or "no longer", and on "@SInCE" or "since X.Y". It matches each comment block as one text, so a phrase split across lines is caught too. Each failure names the file and line, the rule, the matched phrase and the fix. A `lint-allow <rule> until YYYY-MM-DD by <name>: <reason>` marker on the line suppresses one rule for at most 90 days. An expired, reasonless or malformed marker fails. Removes the two comments that broke the rules. SchemaClassScannerInterfaceFieldTest already pins the scan order the scanner comment explained. Co-Authored-By: Claude <noreply@anthropic.com>
Ordinary prose failed the build: "returns the list unchanged", "the previously resolved type", "no longer than 80 characters", "is now cached", "keeps working", "step #1", a #000000 color, "since 2.5 seconds" and a comment that mentions lint-allow. The history list keeps only phrases that narrate a change, "no longer" needs a verb of change, the version rule needs a version-shaped token, and #N skips hex colors, URL fragments and ordinals like "step #1". Text is a marker only in the full `lint-allow <rule> until` form. Every fix line names the lint-allow escape, and an invalid marker says why: expired, more than 90 days out, no reason, or an unknown rule. Java block comments no longer nest and Java strings have no templates. The test fails when it finds no sources instead of passing, and the header counts violations and lines. Co-Authored-By: Claude <noreply@anthropic.com>
"keeps building old schemas" slipped through after the precision pass narrowed the keep-building phrase to one word order. Flag keep(s) building next to schemas in either order, and old or previous behavior. Self-tests pin both, plus "used to build the type" as purpose, not history.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.



Why
Agents working in this repo keep repeating the same mistakes, and the repo should change so a change that looks right from one file is right for the whole repo. One of the most corrected ones is comments: issue numbers in code (
before #552), and history narration like "keep those schemas building" or "as it did before this check". Review catches it a round late, so this makes the build catch it instead, with an error that says what to do.A check that fails the build has to be precise though. A false positive teaches agents to delete good comments, so the phrase list only keeps forms that narrate a change.
Description
CommentConventionsTestreads every comment insrc/mainandsrc/testand fails on:(#N)in// TODO <change> in X.0.0 (#N). Hex colors (#000000), URL fragments (/docs/#123) and ordinals (step #1) don't count@sinceor a version-shaped "since 1.2.0" / "since v2". "since 2.5 seconds" is finePlain prose like "returns the list unchanged", "the previously resolved type", "used to build the type", "no longer than 80 characters", "is now cached" or "keeps working on an empty map" passes. Those cases are pinned in the test's own self-tests.
It matches a whole comment block at once, so a phrase split over two
//lines is still caught. Strings, chars and backtick names are skipped. Kotlin block comments nest, Java ones don't. If it finds no sources (wrong working directory) it fails instead of passing. A failure looks like this:lint-allow <rule> until YYYY-MM-DD by <name>: <reason>on the line skips that rule for that line, for at most 90 days. A bad marker fails and says why: expired, more than 90 days out, no reason, or an unknown rule. Text only counts as a marker in the fulllint-allow <rule> untilform, so a comment that just mentions lint-allow is fine. The review of the marker line is the approval.Master had two comments that break the rules. I deleted both instead of rewording them. The scanner one explained the order of the interface field step, and
SchemaClassScannerInterfaceFieldTestalready pins that (moving the step first breaks 3 tests, dropping theincludeUnusedTypesguard breaks 1). ThelinkDirectiveone just restated the name.It doesn't try to catch comments that repeat the code. The prototype tried a heuristic for that and it caught none of the real ones, so that stays a review thing.
No release note, nothing changes for library users.
Checklist
Ran the test at the six revisions the prototype used. It fails at all of them. Flagged lines: 2 at master, 9 before the comment cleanup commit
d0147a72, 6 and 8 before the rewords infb402fa5and22418890, 2 each before7448f2c6and76ba3a9b. Every comment block the first version flagged is still flagged, including the issue links, the#552refs,before #552, "as before", "as it did before this check" and#823. The drop is extra lines inside blocks that are still flagged ("leave that path unchanged" one line under "as before") and the "keeps working with graphql-java 25" TODO, which was the one false positive.mvn clean verifyon JDK 17 passes (444 tests), the check takes about 0.1s.