A comment that couples two named places carries a tag or a ref
## What this adds
`CommentCouplingConnection`, a module-level rule: a comment that says two named
places move together has to carry a tag or a ref, so the other place is named to
a tool rather than only to whoever is reading that paragraph today.
-- Keep 'Encoder' and 'Decoder' in sync. -- reported
-- [tag:WireFormat] Keep 'Encoder' and 'Decoder' in sync. -- fine
## What "connection" turned out to mean
You asked for prose, and for no false positives. Prose alone did not clear that
bar, and the shape below is what did. A comment is reported when it gives **two**
signals in one sentence.
**A coupling phrase.** Two lists, in
`hopinion/src/Hopinion/Check/Comment/CouplingConnection/Rule.hs`:
- Phrases that are a coupling on their own: `change together`,
`changed together`, `changes together`, and the three `update` spellings.
- Phrases that only say two things are *alike*: `in sync`, `in lockstep`,
`in tandem`. These count only where the same sentence also asks for them to be
held that way, with one of `keep/keeps/kept/keeping`,
`stay/stays/stayed/staying`, `remain/remains`. "Waits until the replica is in
sync" is a state; "keep the two in sync" is an instruction.
**A name marked as code**: Haddock's `'Name'`, or the `` `Name` `` and `@Name@`
spellings. Capitalised only, which is what a module's tokens record.
## Why both, with the numbers
Tried out over nix-ci8, a repository of a thousand Haskell modules.
| signal | findings | worth acting on |
|---|---|---|
| phrase alone | 9 | 1 |
| name alone | 22 | 0 |
| both | 1 | 1 |
The phrase alone was wrong six times. Four were the same sentence across the
webhook handlers:
-- Keep the hub's type in sync with its forge (self-healing).
The keeping there is what the program does at run time against an external
system, not what the next person editing has to do. Two more argued that there
is deliberately *no* second place:
-- Kept in one place so the classification and its metric label stay in sync.
The name alone was worse. Twenty-two findings, none worth acting on, because
naming a sibling definition to orient a reader is what good Haddock is made of:
-- | One entry of a 'BranchSet'.
-- | The SQL expression-level version of 'QueryRunResult'.
-- (Compare with 'RunningSlots', which is shared between the loop and ...)
Together they report one comment in that repository, and it is the coupling:
-- Kept as a shared list so 'XNixNarInfo' and 'XNixCacheInfo' stay in sync.
So this is one rule and not two. Neither half stands up alone, which is why
there is no companion PR for the name matcher.
Phrases tried and dropped, because each has a reading that is not a coupling:
`vice versa` (a bijection inside one function as often as a relation between two
places), `counterpart` ("has no counterpart" says there is no coupling),
`hand in hand`, `must match`, `the same way`, and `remaining`/`remained` as
keeping words ("the remaining rows are in sync" is an adjective).
## What satisfies it
Only the paired forms: `[tag:X]`, `[ref:X]`, `[check:tag X]`, `[check:ref X]`,
each naming something. A bare `[check]`, `[check:file]`, `[check:all <glob>]` or
an empty `[tag:]` does not, and
`bad-a-bare-check-is-not-a-connection.hs` pins that.
The connection may sit in any comment in the same **scope**, not only the same
comment block, so this keeps working — two blocks, because a change of comment
style ends one, but one declaration:
-- | [check:ref WireFormat]
--
-- Keep this in sync with 'Encoder'.
## The cost, stated plainly
Recall. The canonical tagref example names nothing marked, so it is now silent:
-- Keep the encoder and decoder in sync.
`good.hs` pins that as deliberate rather than accidental. If that shape is
common in your code the gate is the wrong trade and this should go back to the
phrase alone, with the six suppressions that implies per repository of that
size.
## Things to look at
- **The two phrase lists and the gate.** They are the whole of the rule's
precision. Each entry is one I can defend; the question is whether any still
has a reading I have not thought of.
- **The sentence split** is on `. ! ? ; :`. An abbreviation splits one sentence
into two, which costs a finding and never invents one. Case is folded per
sentence rather than before the split, because the name test has to see which
words were capitalised — getting that wrong silently reported zero.
- **`alikePhrases`' own Haddock fired the rule on itself** while I was writing
it, because it quoted "kept in sync" to explain the discriminator. I reworded
rather than suppressed. Documentation *about* couplings is a real
false-positive class.
- **A multi-line Haddock is several comment blocks**, since `Hopinion.Comment`
ends a block at a change of style, so a phrase spanning its first two lines is
missed. Pre-existing, affects `CommentBareTodo` too, a miss and never a wrong
finding.
- **Suppressions are not excluded.** A suppression whose reason names a type and
says "kept in sync by hand" lands in the scope that suppression covers, so it
suppresses itself.
## Things to try out locally
cabal run hopinion -- explain CommentCouplingConnection
cabal run hopinion -- module <a file with a coupling comment>
Worth writing a few comments in the shapes you actually use. Recall is the side
I am least sure of, and the gate is what costs it.
## Found on the way, not fixed here
A Haddock above an `{-# ANN #-}` pragma attaches to a declaration named
`declaration` rather than to the definition below it:
nix-ci-cache/src/NixCI/Cache/NarInfo.hs:203:1: StyleHaddockNext
AttachedToDecl (DeclName {declNameText = "declaration"})
"Interpret lexed narinfo fields into a 'NarInfo'..."
That is a comment-attachment bug rather than a rule bug, and it costs this rule
only where somebody writes a tag in such a comment. Its own PR, after this one.
## Not done, and why
- `plan/rules.md` and `plan/plan.md`: I updated the two lists of what ships
(they were already one behind, missing `HsRecordFieldPerLine`) and noted on
`TagrefConnectCoupledCode` which half now ships. I left the measurement
paragraph in `plan.md` alone: it says "thirteen rules" and quotes line counts
I could not reproduce from the tree, so changing the number without the method
would make it less true rather than more.
- `hopinion check` in-process reports one `UNUSED_SUPPRESSION` for
`[allow:TestJsonSpecPerJsonType]` in `Hopinion/Check/Obligation.hs`. It does
the same on `master`, and `nix flake check` is green, so it is an artefact of
running without `.hie` files rather than anything this branch did.
- This branch is off `master`, since there is no `development` branch here.