Add refactor planning docs (.claude/refactor/)
Platform-wide refactor plan: README, CONVENTIONS, CONTRACTS, MODULE_SPLIT, TASKS, OPEN_QUESTIONS. Source of truth for the phased refactor across all 12 submodules. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
175
.claude/refactor/CONVENTIONS.md
Normal file
175
.claude/refactor/CONVENTIONS.md
Normal file
@@ -0,0 +1,175 @@
|
||||
# Conventions
|
||||
|
||||
These rules apply to **every file written or edited** during the refactor.
|
||||
They override personal preference. Be explicit about deviations in
|
||||
`OPEN_QUESTIONS.md`.
|
||||
|
||||
## File size
|
||||
|
||||
| Type | Soft target | Hard cap |
|
||||
|---|---|---|
|
||||
| Domain module (one class / one concern) | ≤ 200 lines | 300 lines |
|
||||
| Pure-function utility module | ≤ 150 lines | 250 lines |
|
||||
| Test file (one .test.js) | ≤ 300 lines | 500 lines |
|
||||
| Markdown spec (in this dir) | — | — |
|
||||
|
||||
If you go over the soft target, ask: is this two concerns? If yes, split.
|
||||
Split before refactoring callers — the smaller pieces test easier.
|
||||
|
||||
## Function size
|
||||
|
||||
- Soft target: ≤ 30 lines.
|
||||
- Hard cap: 60 lines (excluding comments).
|
||||
- A `switch` with mostly-trivial cases counts as one statement, not many.
|
||||
- A long pure-math function (e.g. an integrator) is OK if it can't be
|
||||
meaningfully split.
|
||||
|
||||
## Comments
|
||||
|
||||
Lead with the rule: **default to no comments**. Add one only when *why*
|
||||
is non-obvious to a reader who can already read the code.
|
||||
|
||||
✅ Good comments:
|
||||
```js
|
||||
// Latest-wins: if a new demand arrives mid-dispatch, queue it and
|
||||
// pick up after the current dispatch settles. Without this gate
|
||||
// every PS tick aborts in-flight pump ramps.
|
||||
```
|
||||
|
||||
❌ Bad comments:
|
||||
```js
|
||||
// Set inflow to the value
|
||||
this.inflow = value;
|
||||
```
|
||||
|
||||
```js
|
||||
// Loop over machines
|
||||
for (const m of machines) { ... }
|
||||
```
|
||||
|
||||
Function-level docstring policy:
|
||||
- One short line above the function describing **what it produces** when
|
||||
the name alone isn't enough.
|
||||
- Skip JSDoc `@param` blocks unless the function is part of a public
|
||||
contract (the things in `CONTRACTS.md`). Inline destructuring + good
|
||||
names beats JSDoc that drifts.
|
||||
- Never write multi-paragraph docstrings.
|
||||
|
||||
Inline comments inside a function:
|
||||
- Use to flag a non-obvious invariant, a workaround, or a regression
|
||||
guard. Reference a ticket / commit SHA only if the workaround is
|
||||
load-bearing.
|
||||
- Never narrate what the next line does.
|
||||
|
||||
## Naming
|
||||
|
||||
| Thing | Convention | Example |
|
||||
|---|---|---|
|
||||
| File holding a class | `PascalCase.js` matching the class name | `BasinGeometry.js` |
|
||||
| File of utilities / pure functions | `camelCase.js` | `flowAggregator.js` |
|
||||
| Folder under `src/` | `camelCase` (concern, plural for collections) | `control/`, `strategies/`, `commands/` |
|
||||
| Class | `PascalCase` | `class BasinGeometry` |
|
||||
| Function / method | `camelCase` | `selectBestNetFlow()` |
|
||||
| Private method (convention only) | leading `_` | `_validateThresholdOrdering()` |
|
||||
| Constant | `UPPER_SNAKE_CASE` | `CANONICAL_UNITS` |
|
||||
| Module-private | leading `_` on the local | `const _DEFAULTS = {...}` |
|
||||
| Test file | `<name>.<tier>.test.js` | `flowAggregator.basic.test.js` |
|
||||
|
||||
## Imports
|
||||
|
||||
- A node may import from:
|
||||
- `generalFunctions` (the shared lib)
|
||||
- its own `src/` tree
|
||||
- Node built-ins (`events`, `path`, ...)
|
||||
- declared `dependencies` in its `package.json`
|
||||
- A node MUST NOT import from another node's `src/`.
|
||||
- Cross-node coupling happens only through:
|
||||
- the shared `generalFunctions` API
|
||||
- Node-RED messages (Port 0/1/2)
|
||||
- the parent/child registration handshake (`childRegistrationUtils`)
|
||||
- Avoid deep imports inside `generalFunctions`. Always import from the
|
||||
package root: `const { logger } = require('generalFunctions')`.
|
||||
Exception: tests for `generalFunctions` itself.
|
||||
|
||||
## Module shape
|
||||
|
||||
Default to **one default export per file** when the file is named after
|
||||
the thing it exports (a class, a singleton). Use named exports for
|
||||
collections of small utilities.
|
||||
|
||||
```js
|
||||
// File: BasinGeometry.js
|
||||
class BasinGeometry { ... }
|
||||
module.exports = BasinGeometry;
|
||||
```
|
||||
|
||||
```js
|
||||
// File: flowAggregator.js
|
||||
function selectBestNetFlow(ctx) { ... }
|
||||
function updatePredictedVolume(ctx) { ... }
|
||||
module.exports = { selectBestNetFlow, updatePredictedVolume };
|
||||
```
|
||||
|
||||
## Error handling
|
||||
|
||||
- Validate at boundaries (Node-RED input handler, child registration).
|
||||
Trust internal calls — don't re-validate parameters that already
|
||||
passed an outer check.
|
||||
- Logging on a recoverable issue: `logger.warn` once, fall back to a safe
|
||||
default, continue. Don't throw.
|
||||
- Logging on an unrecoverable issue: `logger.error` and stop ticking the
|
||||
affected subsystem (don't crash Node-RED).
|
||||
- Hard fail (`throw`) only for invariant violations the caller can't
|
||||
recover from (e.g. config schema mismatch detected at construction
|
||||
time).
|
||||
|
||||
## Logging
|
||||
|
||||
- Use the `generalFunctions` `logger` exclusively. No `console.log`.
|
||||
- Log levels:
|
||||
- `error`: something is wrong and downstream behaviour will be
|
||||
affected.
|
||||
- `warn`: something is unexpected; falling back to a safe default.
|
||||
- `info`: state transitions of operational interest (mode changes,
|
||||
child registrations, calibrations).
|
||||
- `debug`: per-tick / per-event traces.
|
||||
- Do **not** ship `enableLog: "debug"` in any default config or example
|
||||
flow. Logs flood within seconds.
|
||||
|
||||
## Testing
|
||||
|
||||
Three tiers per module, mirroring the existing structure:
|
||||
|
||||
```
|
||||
test/
|
||||
basic/<module>.basic.test.js # one module in isolation
|
||||
integration/<feature>.integration.test.js # multiple modules together
|
||||
edge/<scenario>.edge.test.js # edge cases / regressions
|
||||
```
|
||||
|
||||
Rules:
|
||||
- Every new module from a refactor gets at least a basic test.
|
||||
- Every regression discovered during refactor gets an edge test pinning
|
||||
it.
|
||||
- Tests run with `node --test`. No external test framework.
|
||||
- A PR may not lower the green-test count.
|
||||
- Production-readiness ("trial-ready") still requires Docker E2E in
|
||||
addition to `node --test`. See per-node memory.
|
||||
|
||||
## Pure-domain rule (specificClass and below)
|
||||
|
||||
Code under `src/` (other than `nodeClass.js`) is **pure domain**. It must
|
||||
not:
|
||||
- Touch `RED.*`
|
||||
- Read `process.env`
|
||||
- Assume Node-RED is running
|
||||
|
||||
This makes every domain module testable from a plain Node process.
|
||||
|
||||
## Observability of changes
|
||||
|
||||
When a refactor moves logic from one file to another:
|
||||
- Keep behaviour identical at first. Tests pin it.
|
||||
- Behavioural changes (renaming a topic, changing a payload shape) go in
|
||||
separate PRs that are explicitly behavioural.
|
||||
- `git mv` for pure relocations so blame stays useful.
|
||||
Reference in New Issue
Block a user