fix(coding-dev): scoped suppressions, contribution-dependent precedence
- suppressions: prefer the narrowest scope that covers the problem (line, function, module) with a reason comment; a project-wide disable silences the rule in code nobody has written yet. Always name the specific rule. Propagated to rust (#[allow]), csharp (#pragma/SuppressMessage). - precedence: a project's own rules win only when contributing upstream. For local/private work these rules win, adopting only the project's load-bearing conventions. - type checking must pass with zero errors *or warnings*. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
1 parent
57968904bf
commit
9571eb52d2
4 files changed
+48
-17
No files matched your search
@@ -37,8 +37,11 @@ dotnet build MySolution.sln -warnaserror
|
|||||||
- **Nullable warnings are real bugs.** Never silence one with `!` (null-forgiving) — narrow
|
- **Nullable warnings are real bugs.** Never silence one with `!` (null-forgiving) — narrow
|
||||||
the type or fix the contract. `!` is an assertion, and an assertion you cannot prove is a
|
the type or fix the contract. `!` is an assertion, and an assertion you cannot prove is a
|
||||||
future `NullReferenceException`.
|
future `NullReferenceException`.
|
||||||
- Never blanket-disable an analyzer. If a rule is genuinely wrong for the codebase, configure
|
- Never blanket-disable an analyzer. When a rule is genuinely wrong for a piece of code,
|
||||||
it once in `.editorconfig` with a comment.
|
scope the suppression to it — `#pragma warning disable CA1822` around the smallest region
|
||||||
|
that needs it (with a matching `restore`), or a targeted `[SuppressMessage]` attribute —
|
||||||
|
and always with a `Justification` / comment saying why. An `.editorconfig` entry turns the
|
||||||
|
rule off for every file it covers; use it only when the rule is wrong codebase-wide.
|
||||||
|
|
||||||
---
|
---
|
||||||
|
|
||||||
|
|||||||
@@ -43,13 +43,14 @@ into a project that has neither.
|
|||||||
}
|
}
|
||||||
```
|
```
|
||||||
|
|
||||||
**`tsc --noEmit` must pass with 0 errors**, and it belongs in the build gate alongside lint
|
**`tsc --noEmit` must pass with zero errors *and* zero warnings**, and it belongs in the build
|
||||||
and tests — not as a thing someone runs occasionally in their editor.
|
gate alongside lint and tests — not as a thing someone runs occasionally in their editor.
|
||||||
|
|
||||||
> [!NOTE]
|
> [!NOTE]
|
||||||
> Some established codebases deliberately relax `noImplicitAny` or `noUncheckedIndexedAccess`
|
> Some established codebases deliberately relax `noImplicitAny` or `noUncheckedIndexedAccess`
|
||||||
> because their idioms depend on it. Respect the project's `tsconfig.json`; the table above is
|
> because their idioms depend on it. When **contributing upstream**, respect the project's
|
||||||
> the default for **new** projects.
|
> `tsconfig.json` as-is. In **our own repos and non-upstreamed forks**, the settings above are
|
||||||
|
> the target — tightening them is fair game (as its own change, not smuggled into a feature).
|
||||||
|
|
||||||
---
|
---
|
||||||
|
|
||||||
|
|||||||
+11
-2
@@ -61,8 +61,17 @@ workspace = true
|
|||||||
> carries a second copy that drifts — which is how a binary once kept reading its own stale
|
> carries a second copy that drifts — which is how a binary once kept reading its own stale
|
||||||
> allows while the workspace's were being actively edited.
|
> allows while the workspace's were being actively edited.
|
||||||
|
|
||||||
- **Never `#[allow]` on a whim.** If a lint is genuinely wrong for the codebase, allow it
|
- **Never `#[allow]` on a whim** — but when a lint is genuinely wrong for a piece of code,
|
||||||
**workspace-wide** with a comment explaining why, so the decision is visible.
|
**allow it at the narrowest scope that covers it** (the expression, the function, the
|
||||||
|
module) with a comment saying why:
|
||||||
|
```rust
|
||||||
|
// The FFI callback signature is fixed by the C API; we cannot take fewer arguments.
|
||||||
|
#[allow(clippy::too_many_arguments)]
|
||||||
|
extern "C" fn on_event(/* … */) { }
|
||||||
|
```
|
||||||
|
- A **workspace-wide** allow in `[workspace.lints]` is for the rare case where the lint is
|
||||||
|
wrong for the *entire* codebase — not a shortcut for one stubborn call site. It silences
|
||||||
|
the rule in code nobody has written yet.
|
||||||
- **Re-test every `#[allow]` periodically.** If removing one produces no warning, delete it.
|
- **Re-test every `#[allow]` periodically.** If removing one produces no warning, delete it.
|
||||||
- `too_many_lines` is the mechanical enforcement of the 100-line function limit. A function
|
- `too_many_lines` is the mechanical enforcement of the 100-line function limit. A function
|
||||||
that trips it wants extracting, not an allow.
|
that trips it wants extracting, not an allow.
|
||||||
|
|||||||
+27
-9
@@ -14,9 +14,17 @@ These are the defaults for **all** code work, in any language. They are preferen
|
|||||||
reasons attached, not style trivia — most exist because ignoring them caused a real problem.
|
reasons attached, not style trivia — most exist because ignoring them caused a real problem.
|
||||||
|
|
||||||
> [!IMPORTANT]
|
> [!IMPORTANT]
|
||||||
> **A project's own rules win.** If a repository has an `AGENTS.md`, `CLAUDE.md`, `AGENT.md`,
|
> **Whose rules win depends on where the code is going.** Always read a repository's
|
||||||
> `.rules`, or `CONTRIBUTING.md`, read it first — it is more specific than this document and
|
> `AGENTS.md`, `CLAUDE.md`, `AGENT.md`, `.rules`, or `CONTRIBUTING.md` first — but what you do
|
||||||
> overrides it on conflict. This file is the default for everything those files do not say.
|
> on a conflict differs:
|
||||||
|
>
|
||||||
|
> | Situation | Precedence |
|
||||||
|
> | :--- | :--- |
|
||||||
|
> | **Contributing upstream** — a PR, a patch, or anything landing in someone else's repo | **The project's rules win.** Match their conventions exactly, even where they contradict this document. A PR that fights the house style gets rejected on style instead of reviewed on substance. |
|
||||||
|
> | **Local / private work** — our own repos, forks we don't upstream, private branches, local patches | **These rules win.** A project's conventions are a data point, not an obligation. Adopt the ones that are load-bearing (their build gate, their architecture contracts, anything that breaks if ignored) and apply our own preferences for the rest. |
|
||||||
|
>
|
||||||
|
> When in doubt about which mode you are in, **ask** — it changes the answer on strictness,
|
||||||
|
> file size, typing, and test policy all at once.
|
||||||
|
|
||||||
Per-language specifics live in [`code-languages/`](./code-languages/):
|
Per-language specifics live in [`code-languages/`](./code-languages/):
|
||||||
|
|
||||||
@@ -39,10 +47,15 @@ Strict mode is the starting point, not an aspiration to migrate toward later.
|
|||||||
|
|
||||||
- Warnings are errors in CI and in the build gate (`-warnaserror`, `-D warnings`, `--strict`).
|
- Warnings are errors in CI and in the build gate (`-warnaserror`, `-D warnings`, `--strict`).
|
||||||
- Enable the pedantic/extra lint tiers, not just the defaults.
|
- Enable the pedantic/extra lint tiers, not just the defaults.
|
||||||
- Type checking must pass with **zero** errors — `tsc --noEmit`, `mypy`, `cargo check` alike.
|
- Type checking must pass with **zero errors or warnings** — `tsc --noEmit`, `mypy`,
|
||||||
|
`cargo check` alike.
|
||||||
- Never weaken a check to make a build pass. Fix the failing step. If a check is genuinely
|
- Never weaken a check to make a build pass. Fix the failing step. If a check is genuinely
|
||||||
wrong for the codebase, disable it **project-wide in one place with a comment explaining
|
wrong for a specific piece of code, **suppress it at the narrowest scope that covers the
|
||||||
why**, so the decision is visible and reviewable — never with a scattered inline suppression.
|
problem — the line, the function, or the module — with a comment explaining why.** A
|
||||||
|
project-wide disable is a bigger hammer than it looks: it silences the rule everywhere,
|
||||||
|
including in code written later by someone who never saw the reason. Reach for it only when
|
||||||
|
the rule is genuinely wrong for the *whole* codebase, never as a shortcut for a stubborn
|
||||||
|
call site.
|
||||||
|
|
||||||
### Suppressions are a last resort, and they expire
|
### Suppressions are a last resort, and they expire
|
||||||
|
|
||||||
@@ -50,9 +63,14 @@ Strict mode is the starting point, not an aspiration to migrate toward later.
|
|||||||
- When you do suppress one, **say so explicitly in your report to the user** — name the
|
- When you do suppress one, **say so explicitly in your report to the user** — name the
|
||||||
warning, where it is, and why it could not be fixed. A silent suppression is the failure
|
warning, where it is, and why it could not be fixed. A silent suppression is the failure
|
||||||
mode this rule exists to prevent.
|
mode this rule exists to prevent.
|
||||||
- **Re-test suppressions periodically.** Removing an `#[allow]` / `// eslint-disable` /
|
- **Every suppression carries a comment saying why.** A bare `#[allow]` / `// eslint-disable`
|
||||||
`#pragma warning disable` that no longer suppresses anything should produce no warning —
|
/ `# noqa` with no reason is indistinguishable from someone giving up, and nobody will ever
|
||||||
if it doesn't, delete it. Stale suppressions accumulate and hide real regressions.
|
dare remove it.
|
||||||
|
- **Name the specific rule**, never a blanket suppression: `# noqa: S603`, not `# noqa`;
|
||||||
|
`//nolint:errcheck`, not `//nolint`.
|
||||||
|
- **Re-test suppressions periodically.** Removing one that no longer suppresses anything
|
||||||
|
should produce no warning — if it doesn't, delete it. Stale suppressions accumulate and
|
||||||
|
hide real regressions.
|
||||||
|
|
||||||
---
|
---
|
||||||
|
|
||||||
|
|||||||
Reference in new issue
Block a user