Created Started Finished
A comment that couples two places carries a tag or a ref
## What this adds
`CommentCouplingConnection`, a module-level rule: a comment that says two
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 the encoder and the decoder in sync. -- reported
-- [tag:WireFormat] Keep the encoder and the decoder in sync. -- fine
## What "connection" turned out to mean
You asked for prose, and for no false positives, so the check fires only where
an intended connection is certain. 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`.
That second condition is the one that earns the no-false-positives bar. "Waits
until the replica is in sync" is a state somebody waits for; "keep the two in
sync" is an instruction to whoever edits this next. Without it the rule fires on
the first, which is a finding nobody can act on. `good.hs` pins both readings.
Sentence-bounded rather than comment-bounded, so "Keep this simple. The cache is
in sync only by accident." stays quiet.
Phrases I tried and dropped, because each has a reading that is not a coupling:
- `vice versa`: describes a bijection inside one function as often as a
relationship between two places.
- `counterpart`: "has no counterpart" says there is no coupling.
- `hand in hand`, `must match`, `the same way`: comparisons, which is how most
prose describes anything.
- `remaining`, `remained` as keeping words: "the remaining rows are in sync" is
an adjective, not an instruction.
## What satisfies it
Only the paired forms, as you chose: `[tag:X]`, `[ref:X]`, `[check:tag X]`,
`[check:ref X]`, each of which has to name 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 in the
same comment block. This shape has to keep working, and the two lines are two
blocks because a change of comment style ends one:
-- | [check:ref WireFormat]
--
-- Keep this in sync with the encoder.
A scope is a declaration, which is also the granularity a suppression is matched
at, so it is the granularity a reader can predict. The cost is that at file
scope one tag anywhere silences every file-level coupling comment. That errs
towards silence, which is the direction you asked for.
## Things to look at
- **The two phrase lists.** They are the whole of the rule's precision, and
they are deliberately the floor rather than the ceiling. Each entry is one I
can defend; the question is whether any of them still has a reading I have
not thought of, and whether something obvious is missing.
- **The sentence split** is on `. ! ? ; :`. An abbreviation splits one sentence
into two, which costs a finding and never invents one. Colons are common in
this repository's comments, and splitting there is deliberate.
- **`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
it rather than suppressing. Documentation *about* couplings is a real
false-positive class, and the suppression is the answer where rewording is
not.
- **A multi-line Haddock is several comment blocks**, since `Hopinion.Comment`
ends a block at a change of style. So a phrase spanning the first and second
line of a `-- |` block is missed. That is pre-existing and affects
`CommentBareTodo` too; I did not touch it. It is a miss, never a wrong
finding.
- **Suppressions are not excluded.** A suppression whose reason says "kept in
sync by hand" lands in the scope that suppression covers, so it suppresses
itself. I tried that out both ways round and it is quiet.
## 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 of your own in the shapes you actually use and
seeing whether it fires where you want it to. The recall side is the part I am
least sure of.
## 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.