270edde4

Say each of the three things once

A diagnostic prints three pieces of text: the finding message as the
error headline, the rule text as the label under the marker, and the
rule why as a note. Every rule was answering the same question in all
three, and most of them were answering it with negations. A reader
stopped by an `Eq` finding got:

```
error[HsNoCustomEq]: This Eq instance is written out. Derive it instead, so that what it compares is the fields of the type.
   │ ^^^^^^^^^^^^^^^^^^^^^ Eq is derived.
   = note: If you need a custom equality operation, use a separate function, not the Eq instance.
```

"Derive it instead", "Eq is derived", and then the argument for deriving
a third time. Nine of the sixteen finding messages carried a remedy
sentence that the marker two lines below already said.

Two commits: one giving each field a single job, one taking the
negations out of the prose.

## Say each of the three things once

**The finding message names what was found here, and only that.** Where
the rule knows which one it found, it now says so, because that is what
varies between two findings of one rule and what a reader scanning a
list of them needs:

| | before | after |
|---|---|---|
| `HsNoCustomEq` | This Eq instance is written out. Derive it instead, so that what it compares is the fields of the type. | `Eq User is written out.` |
| `HsNoCustomShowRead` | This Show instance is written out. Derive it instead, or discard what it is given if the point is to hide the value. | `Show Token is written out, and uses the value.` |
| `TestNoTestHelpers` | A top-level binding in a test file, beside spec. | `A top-level binding beside spec: resourceDir.` |
| `HsAppOnlyMain` | A declaration in an executable's own source, beside main. Nothing here can be imported or tested, so move it into the library. | `A declaration beside main: twice.` |
| `HsGenValidInGenPackage` | A GenValid instance outside a -gen package. | `A GenValid instance in thing's library.` |
| `CommentBareTodo` | A bare TODO. Reference an issue or a URL, or do it. | `A TODO with no issue reference or URL.` |
| `HsNoFilePath` | The type FilePath. Say what the path is with Path from path. | `The type FilePath.` |

The names come off facts that were already there: `instanceFactType`
for the four instance rules, the decl name for the two that report a
declaration, the package name for the generator rule.

**The rule text is the standard, in the one line the marker has room
for.** Six were two sentences or a clause chain where the marker fits a
noun phrase. `HsNoFilePath` went from "Say what a path is with path's
Path, rather than with FilePath." to "A path is a Path, not a
FilePath."; `TestJsonSpecPerJsonType` from 26 words to "ToJSON and
FromJSON oblige jsonSpec @T in the gen package's test suite."

**The rule why is the argument for the rule.** It stays general to the
rule and stops when it has made it. `HsNoFilePath` went 130 words to
43, `TestJsonSpecPerJsonType` 110 to 38, `HsNoCustomShowRead` 105 to
40, `HsAppOnlyMain` 85 to 32, `CommentBareTodo` 80 to 20.

Two exceptions were worth their sentence and stayed: `toFilePath` at
the edge is not what `HsNoFilePath` reports, and a `Show` that ignores
the value is how a secret stays out of the logs.

`ExampleNoShouting` in `hopinion-gen` is left alone. It exists to show a
repository adding a rule of its own, and its why is about that rather
than about shouting.

## Say it without the negations

A reader parses "nothing X is Y" slower than "only X is Y", and stacked
negations worse still.

| | before | after |
|---|---|---|
| `CommentBareTodo` why | A TODO **nothing** points at is a note **nobody** is holding: … and **nothing** brings it back up. | A reference tells the reader where the work is tracked. A TODO on its own reads like a plan, and goes stale in place. |
| `HsNoFilePath` why | it does **not** say file or directory, absolute or relative, or path at all | Path says in the type whether it is a file or a directory, and whether it is absolute or relative. FilePath is String, so those mistakes compile. |
| `HsAppOnlyMain` why | **Nothing** … can be imported, so **nothing** in it can be tested. | Only a library can be imported, so only a library can be tested. |
| `TestGenValidSpecPerGenValid` why | A generator **nothing** runs is one **nobody** has checked | genValidSpec is what checks the generator |
| `TestJsonSpecPerJsonType` why | **nothing** in the types makes the two halves agree | the compiler accepts an encoder and a decoder that disagree |
| `HsTestOneSpecPerFile` why | **Nothing** reads a test file **but** the Main… | Only the Main that gathers the specs reads a test file… |
| `TestNoTestHelpers` text | A test file binds **nothing** at the top level **but** spec. | A test file binds only spec at the top level. |
| `HsAppOnlyMain` text | …holds main = theRealMain, and **nothing else**. | …holds only main = theRealMain. |
| `HsNoSemigroupOnText` why | <> does **not** format well, literal lists do. | A literal list of the pieces formats better than a <> chain. |
| `HsLambdaCase` why | …the diff says **nothing** about what changed. | …buries the real change in the diff. |
| `HsNoCustomShowRead` text | Show and Read are derived, **unless** the methods ignore the value. | Show and Read are either derived, or written with methods that ignore the value. |

The report layer got the same pass: a suppression sits on its own rather
than not being attached to anything, an outlived suppression misleads
the next reader rather than being worse than no suppression, and an
unknown rule id asks the reader to check the spelling rather than
explaining what turning off a name nothing answers to leaves running.

Single contrastive negations stayed where the contrast is the content
and it is one clause deep: "A path is a Path, not a FilePath.",
"Concatenate strings and text with a list, not with <> or ++."

Goldens reset. 496 tests pass, `nix flake check` passes.

Total

27m09s

Created 1 month ago Started 1 month ago Finished 1 month ago

Timeline