fix(coding-dev): prefer a test over the build script for size limits
A shell check only runs where that script runs. A test travels with the repo, needs no extra tooling, and fails with a message naming the offending files. Enforcement is now a preference order: native linter rule > test > build script as fallback, with a worked example and a note on why a per-compile build hook usually is not worth the incremental-build cost. Propagated per language: rust (clippy has no file-length lint -> workspace root test), js-typescript (ESLint max-lines covers both natively, no test needed), go/python/csharp (linter for functions, test for files), bash (don't reimplement in the gate what a test can assert). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
1 parent
5d96e69d2b
commit
32b47b075b
7 files changed
+60
-8
No files matched your search
@@ -125,6 +125,10 @@ mistakes before a human does.
|
|||||||
- **A skipped test fails the gate.** Never let a skip report green.
|
- **A skipped test fails the gate.** Never let a skip report green.
|
||||||
- **Print a summary with numbers**, not just a verdict — counts of warnings, suppressions, and
|
- **Print a summary with numbers**, not just a verdict — counts of warnings, suppressions, and
|
||||||
file-size violations. A suppression count that has grown is worth seeing even on a pass.
|
file-size violations. A suppression count that has grown is worth seeing even on a pass.
|
||||||
|
- **Don't reimplement in the gate what a test can assert.** Checks like file-size limits
|
||||||
|
belong in the test suite where they travel with the repo and fail with a useful message;
|
||||||
|
the build script is the fallback for projects with no suite to hang them on. See
|
||||||
|
[`coding-dev.md` §4](skills-public/coding-dev.md).
|
||||||
- Keep per-step logs in a known directory so a failure can be read after the fact.
|
- Keep per-step logs in a known directory so a failure can be read after the fact.
|
||||||
- **Never bypass a failed gate** by copying a binary or running the install step by hand. Fix
|
- **Never bypass a failed gate** by copying a binary or running the install step by hand. Fix
|
||||||
the failing step; do not weaken the check to make it pass.
|
the failing step; do not weaken the check to make it pass.
|
||||||
|
|||||||
@@ -80,7 +80,8 @@ dotnet build MySolution.sln -warnaserror
|
|||||||
- Pattern matching and switch expressions over `if`/`else` ladders.
|
- Pattern matching and switch expressions over `if`/`else` ladders.
|
||||||
- `IReadOnlyList<T>` / `IReadOnlyDictionary<K,V>` in public signatures returning collections.
|
- `IReadOnlyList<T>` / `IReadOnlyDictionary<K,V>` in public signatures returning collections.
|
||||||
- `internal` by default; `public` only for genuine API surface.
|
- `internal` by default; `public` only for genuine API surface.
|
||||||
- File and function size limits from the general rules apply.
|
- File and function size limits from the general rules apply — put the file limit in a test
|
||||||
|
rather than an MSBuild target (see [`coding-dev.md` §4](skills-public/coding-dev.md)).
|
||||||
- `dotnet format` before committing.
|
- `dotnet format` before committing.
|
||||||
|
|
||||||
---
|
---
|
||||||
|
|||||||
@@ -137,7 +137,8 @@ CGO_ENABLED=0 go build -trimpath -ldflags="-s -w" -o bin/app ./cmd/app
|
|||||||
- Keep the exported surface small. Unexported by default.
|
- Keep the exported surface small. Unexported by default.
|
||||||
- Embed frontend/static assets with `//go:embed` for single-binary distribution — the frontend
|
- Embed frontend/static assets with `//go:embed` for single-binary distribution — the frontend
|
||||||
itself follows [`js-typescript.md`](skills-public/code-languages/js-typescript.md).
|
itself follows [`js-typescript.md`](skills-public/code-languages/js-typescript.md).
|
||||||
- File and function size limits from the general rules apply.
|
- File and function size limits from the general rules apply — `funlen` in `golangci-lint`
|
||||||
|
covers functions; put the file limit in a test (see [`coding-dev.md` §4](skills-public/coding-dev.md)).
|
||||||
|
|
||||||
---
|
---
|
||||||
|
|
||||||
|
|||||||
@@ -141,7 +141,8 @@ Attach an `AbortController` to every long-lived `fetch` and abort it in cleanup.
|
|||||||
a new formatter or lint config into a project that already has one.
|
a new formatter or lint config into a project that already has one.
|
||||||
- **One file until it earns a split** — around 250 lines for a self-contained plugin/feature,
|
- **One file until it earns a split** — around 250 lines for a self-contained plugin/feature,
|
||||||
or when a second file is genuinely reusable (`utils.ts`, `types.ts`, `components/`).
|
or when a second file is genuinely reusable (`utils.ts`, `types.ts`, `components/`).
|
||||||
Hard limits from the general rules still apply.
|
Hard limits from the general rules still apply: ESLint's `max-lines` and
|
||||||
|
`max-lines-per-function` cover both natively, so prefer those over a separate test here.
|
||||||
|
|
||||||
---
|
---
|
||||||
|
|
||||||
|
|||||||
@@ -132,7 +132,9 @@ When talking to a rate-limited API (a chat platform, a store, a tracker):
|
|||||||
installable package** with its own repo and version, consumed as a dependency.
|
installable package** with its own repo and version, consumed as a dependency.
|
||||||
- Keep persistent state in the framework's own config/storage system so it survives restarts;
|
- Keep persistent state in the framework's own config/storage system so it survives restarts;
|
||||||
do not invent a parallel JSON file beside it.
|
do not invent a parallel JSON file beside it.
|
||||||
- File and function size limits from the general rules apply.
|
- File and function size limits from the general rules apply — Ruff's `PLR0915` covers
|
||||||
|
function length; put the file limit in a pytest case (see
|
||||||
|
[`coding-dev.md` §4](skills-public/coding-dev.md)).
|
||||||
|
|
||||||
---
|
---
|
||||||
|
|
||||||
|
|||||||
@@ -75,6 +75,10 @@ workspace = true
|
|||||||
- **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.
|
||||||
|
- Clippy has **no file-length lint**, so the 1000-line file limit belongs in a test — see
|
||||||
|
[`coding-dev.md` §4](skills-public/coding-dev.md) for the pattern. Put it in the workspace
|
||||||
|
root's `tests/` so one test covers every member crate, walking `src/` plus each
|
||||||
|
`crates/*/src`. `cargo test --workspace` then enforces it for free.
|
||||||
|
|
||||||
---
|
---
|
||||||
|
|
||||||
|
|||||||
+43
-4
@@ -142,10 +142,49 @@ depends on those.
|
|||||||
| **Function parameters** | 3–4 | — | ~5, then pass a struct/object |
|
| **Function parameters** | 3–4 | — | ~5, then pass a struct/object |
|
||||||
| **Nesting depth** | 2 | — | 3, then extract |
|
| **Nesting depth** | 2 | — | 3, then extract |
|
||||||
|
|
||||||
- **Enforce the function limit with the linter, not by eye** (e.g. Clippy's `too_many_lines`).
|
### Enforce both mechanically, never by eye
|
||||||
A function that trips it wants extracting, not a suppression.
|
|
||||||
- **Enforce the file limit in the build script** — a check that fails over the hard limit and
|
A limit nobody checks is a suggestion. Pick the **highest** applicable option:
|
||||||
*names* anything past the soft split point, so the seam gets found while splitting is cheap.
|
|
||||||
|
1. **A linter rule**, where one exists — the function limit is usually covered natively
|
||||||
|
(Clippy's `too_many_lines`, ESLint's `max-lines-per-function`, `golangci-lint`'s `funlen`,
|
||||||
|
Ruff's `PLR0915`). A function that trips it wants extracting, not a suppression.
|
||||||
|
2. **A test**, for anything the linter does not cover — file length almost always falls here.
|
||||||
|
A test is the best home for it: it runs with the suite everyone already runs, it needs no
|
||||||
|
extra tooling, it fails with a message naming the offending files, and it travels with the
|
||||||
|
repository rather than living in one machine's shell script.
|
||||||
|
3. **The build script**, only as a fallback when the project has no test suite to hang it on.
|
||||||
|
|
||||||
|
Whichever you use, it should **fail over the hard limit** and separately *name* anything past
|
||||||
|
the soft split point — so the seam gets found while splitting is still cheap, rather than at
|
||||||
|
the moment the hard limit finally trips.
|
||||||
|
|
||||||
|
```rust
|
||||||
|
#[test]
|
||||||
|
fn no_source_file_exceeds_limits() {
|
||||||
|
const HARD: usize = 1000;
|
||||||
|
const WARN: usize = 600;
|
||||||
|
let mut over = Vec::new();
|
||||||
|
|
||||||
|
for path in source_files() {
|
||||||
|
let lines = std::fs::read_to_string(&path).unwrap().lines().count();
|
||||||
|
if lines > HARD {
|
||||||
|
over.push(format!("{} — {lines} lines (limit {HARD})", path.display()));
|
||||||
|
} else if lines > WARN {
|
||||||
|
// Not a failure: surfaces the seam while splitting is still cheap.
|
||||||
|
eprintln!("note: {} is {lines} lines, approaching the {HARD} limit", path.display());
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
assert!(over.is_empty(), "files over the size limit:\n{}", over.join("\n"));
|
||||||
|
}
|
||||||
|
```
|
||||||
|
|
||||||
|
> [!NOTE]
|
||||||
|
> A test runs with the suite, not on every compile. If you genuinely need the check on every
|
||||||
|
> build, most toolchains have a pre-build hook for it (Cargo's `build.rs`, an MSBuild target,
|
||||||
|
> a bundler plugin) — but that cost is paid on every incremental build forever, and the suite
|
||||||
|
> is usually soon enough. Reach for the hook only if oversized files keep landing anyway.
|
||||||
- **Split by concern, never by line count.** A large `strip.rs` becomes
|
- **Split by concern, never by line count.** A large `strip.rs` becomes
|
||||||
`strip/fader.rs`, `strip/knobs.rs`, `strip/routing.rs` — never `strip1.rs` / `strip2.rs`.
|
`strip/fader.rs`, `strip/knobs.rs`, `strip/routing.rs` — never `strip1.rs` / `strip2.rs`.
|
||||||
- One coherent responsibility per module. A module that draws widgets should not also own
|
- One coherent responsibility per module. A module that draws widgets should not also own
|
||||||
|
|||||||
Reference in new issue
Block a user