diff options
Diffstat (limited to 'docs/design')
| -rw-r--r-- | docs/design/2026-07-10-curated-help-menu-survey.org | 96 | ||||
| -rw-r--r-- | docs/design/2026-07-10-ledger-config-audit.org | 146 | ||||
| -rw-r--r-- | docs/design/2026-07-10-org-module-ownership.org | 138 | ||||
| -rw-r--r-- | docs/design/signal-client-review.org | 2 | ||||
| -rw-r--r-- | docs/design/subr-mock-migration.org | 158 | ||||
| -rw-r--r-- | docs/design/utility-inventory.org | 6 |
6 files changed, 542 insertions, 4 deletions
diff --git a/docs/design/2026-07-10-curated-help-menu-survey.org b/docs/design/2026-07-10-curated-help-menu-survey.org new file mode 100644 index 00000000..a7469ac1 --- /dev/null +++ b/docs/design/2026-07-10-curated-help-menu-survey.org @@ -0,0 +1,96 @@ +#+TITLE: Survey — where a curated "?" help menu makes sense +#+AUTHOR: Craig Jennings +#+DATE: 2026-07-10 + +* What this is + +The mechanical half of the "=?= = curated help menu" convention task. It reports, +per mode, whether =?= is free, whether =H= is free, and what the package already +ships. It changes no keybinding and makes no per-mode recommendation, because which +commands belong in a curated menu is a taste call. + +The reference implementation is calibredb (=modules/calibredb-epub-config.el:129-131=): +=?= runs =cj/calibredb-menu=, a curated transient of the frequent workflows, and =H= +runs the package's own =calibredb-dispatch=. + +* Method + +Bindings were read from the live keymaps, not from source, because a package's +=define-key= calls and its =:bind= forms don't always agree with what ends up in the +map. + +The probe ran in a throwaway =emacs --batch= with =package-initialize=, not in the +running daemon, for two reasons. Probing =calibredb-search-mode-map= or +=mu4e-headers-mode-map= in the daemon would force-load those packages into a live +session. And a batch Emacs reports the *package's* defaults rather than the defaults +plus Craig's config, which is the question being asked: what does the package ship? + +=mu4e= is not an ELPA package. It installs with =mu= into +=/usr/share/emacs/site-lisp/mu4e=, so it needed an explicit =load-path= entry. + +* The table + +=FREE= means the key is unbound in that mode's map. + +| Mode | =?= | =H= | Ships a dispatch? | +|---------------+--------------------------------+---------------------------+------------------------| +| calibredb | =cj/calibredb-menu= (curated) | =calibredb-dispatch= | yes, moved to =H= | +|---------------+--------------------------------+---------------------------+------------------------| +| nov | FREE | FREE | no | +|---------------+--------------------------------+---------------------------+------------------------| +| eat | FREE | FREE | no | +|---------------+--------------------------------+---------------------------+------------------------| +| elfeed-search | FREE | FREE | no | +|---------------+--------------------------------+---------------------------+------------------------| +| elfeed-show | FREE | FREE | no | +|---------------+--------------------------------+---------------------------+------------------------| +| dired/dirvish | =dired-summary= | =dired-do-hardlink= | no (summary is a | +| | | | one-line echo) | +|---------------+--------------------------------+---------------------------+------------------------| +| magit-status | =magit-dispatch= | =magit-describe-section= | yes, already on =?= | +|---------------+--------------------------------+---------------------------+------------------------| +| pdf-view | =describe-mode= | =pdf-view-fit-height-...= | no | +|---------------+--------------------------------+---------------------------+------------------------| +| org-agenda | =org-agenda-show-the-flagging- | =org-agenda-holidays= | no | +| | note= | | | +|---------------+--------------------------------+---------------------------+------------------------| +| mu4e-headers | =mu4e-headers-mark-for-unread= | =mu4e-display-manual= | no | +|---------------+--------------------------------+---------------------------+------------------------| + +* What the table shows + +Four modes have both keys free: nov, eat, elfeed-search, elfeed-show. Nothing has to +be displaced to adopt the convention there. These are the cheap ones. + +Three modes bind =?= to something small. =dired-summary= echoes a one-line key +reminder. =pdf-view= sends =?= to =describe-mode=. =org-agenda= puts a +flagging-note command on it. In each case the convention would displace a command of +low value, but it is a displacement and needs a decision. + +=mu4e-headers= is the awkward one. Its =?= is =mu4e-headers-mark-for-unread=, a real +editing command in the marking family (=u=, =d=, =!=). Rebinding it would break +muscle memory for an operation that mutates mail state. Its =H= is already +=mu4e-display-manual=. + +=magit= is the counterexample worth noting. It already does exactly what the +convention proposes: =?= opens =magit-dispatch=, its own curated transient. The +convention is not new, it is magit's habit, generalized. Magit needs no change. + +=dirvish= inherits =dired-mode-map=, so a change there lands on plain dired too. + +* Caveats + +The probe reads package defaults. Craig's own config may rebind =?= in a mode after +the package loads, and the batch probe would not see it. A grep for =?=-rebinding in +=modules/*.el= found only the calibredb case, so the table should hold, but a mode +whose config binds =?= inside a =:config= block reached by a path the grep missed +would not show up. + +=signel= and =pearl= are in the task's candidate list and are absent from the table. +Neither was probeable: they are local checkouts, not ELPA packages, and loading them +in batch pulls in dependencies the probe deliberately avoided. + +* The decisions this leaves + +For each mode, whether a curated =?= menu is worth its cost, and if so, which +commands go in it. That is the taste call, and the reason this survey stops here. diff --git a/docs/design/2026-07-10-ledger-config-audit.org b/docs/design/2026-07-10-ledger-config-audit.org new file mode 100644 index 00000000..ff969d78 --- /dev/null +++ b/docs/design/2026-07-10-ledger-config-audit.org @@ -0,0 +1,146 @@ +#+TITLE: Ledger config audit — correctness and guardrail gaps +#+AUTHOR: Craig Jennings +#+DATE: 2026-07-10 + +* Scope + +The correctness half of the ledger guardrail work. It audits +=modules/ledger-config.el= (71 lines, no test file before today) and reports what's +wrong and what's missing. It designs no guardrails, because those choices are +Craig's. + +Characterization tests for the behavior that exists now landed alongside this note +at =tests/test-ledger-config.el=. + +The stakes set the bar. This is a financial file, and Craig's stated goal for the +follow-on work is "enough guardrails that it's hard to make a costly mistake." + +* Findings + +** F1. The linting doesn't run. Nothing checks unbalanced transactions. + +FIXED 2026-07-10. =flycheck-config.el= now hooks =flycheck-mode= to =ledger-mode=. +Verified end to end on an unbalanced fixture: flycheck reports "Transaction does not +balance" with the =$10.00= remainder. The finding is kept below as the record of +what was wrong. + +The module's own commentary says it provides "flycheck linting." + +=flycheck-ledger= is loaded (=ledger-config.el:59-60=) and registers the =ledger= +checker. But =flycheck-mode= is never enabled in a ledger buffer. +=flycheck-config.el:47= hooks =flycheck-mode= to exactly two modes: + +#+begin_src elisp +:hook ((sh-mode emacs-lisp-mode) . flycheck-mode) +#+end_src + +There is no =global-flycheck-mode= anywhere in the config. Verified against a real +=ledger-mode= buffer in the running daemon: =major-mode= is =ledger-mode=, +=flycheck-checkers= contains =ledger=, and =flycheck-mode= is =nil=. + +So an unbalanced transaction, a malformed date, and a typo'd account name all +produce no warning. The guardrail Craig believes he has is not connected. + +This is the finding that matters. Everything below is smaller. + +** F2. Every save silently reorders the entire file. + +=cj/ledger--clean-before-save= runs =ledger-mode-clean-buffer= on +=before-save-hook= (=ledger-config.el:32-40=), with =cj/ledger-clean-on-save= +defaulting to =t=. + +=ledger-mode-clean-buffer= (=ledger-mode.el:226=) is not a formatter. Its own +docstring says "Indent, remove multiple line feeds and sort the buffer," and its +body calls, in order: =untabify=, =ledger-sort-buffer=, =ledger-post-align-postings=, +=ledger-mode-remove-extra-lines=. + +=ledger-sort-buffer= (=ledger-sort.el:106=) sorts the whole buffer by date, from +=point-min= to =point-max= unless the file carries explicit sort markers. + +So every =C-x C-s= rewrites the transaction order of a financial file. The task +asked whether clean-on-save "ever reorders or rewrites in a surprising way." It +reorders, every time, by design of the upstream function. + +*Accepted 2026-07-10, on inspection of the sort key.* The reordering is +chronological and nothing else. =ledger-sort-startkey= (=ledger-sort.el:62=) builds +its key from the first ten characters of a transaction's opening line, the ISO date, +through =ledger-parse-iso-date= and =float-time=. Payee, amount, and account never +enter the key. =sort-subr= uses Emacs's stable sort, so transactions sharing a date +keep the order they were typed in. + +The scope is bounded on request: =ledger-sort-buffer= narrows to the region between +=; Ledger-mode: Start sort= and =; Ledger-mode: End sort= marker comments when the +file carries them, and only falls back to the whole buffer when it doesn't. + +So a save sorts by date, stably, over a region the file can choose. Craig accepts +that. The finding stays recorded because "clean-on-save" still reads as whitespace +tidying, and the next person to meet this function should know it sorts. + +** F3. The demoted error hides a partial rewrite, not just a message. + +The clean is wrapped in =with-demoted-errors= so "a malformed buffer still saves" +(=ledger-config.el:34-37=). + +The comment is accurate about the save. It is misleading about the buffer. The +operations inside =ledger-mode-clean-buffer= run in sequence and mutate as they go. +An error raised by =ledger-post-align-postings= or by the =search-forward= that +restores point (=ledger-mode.el:239=) happens *after* =ledger-sort-buffer= has +already reordered everything. Nothing rolls back. + +The result is a file that saved, in a state neither the user nor the cleaner +intended, with a message in the echo area that scrolls away. + +** F4. Reconcile has no confirmation, and clears whole transactions. + +=ledger-clear-whole-transactions= is =t= (=ledger-config.el:43=), so a reconcile +marks entire transactions cleared rather than individual postings. Combined with no +confirmation step anywhere in the module, a stray keypress in a reconcile buffer +mutates the ledger file. + +This is upstream behavior, not a bug in this module. It is named here because the +follow-on guardrail work asked about "reconcile safety" and this is what reconcile +safety currently amounts to. + +* What is not a problem + +Worth recording so the guardrail work doesn't chase it. + +- *company-ledger's global backend is correctly scoped.* =ledger-config.el:68= does + =(add-to-list 'company-backends 'company-ledger)= globally, which looks like it + would offer ledger completions everywhere. It doesn't: the backend's own =prefix= + command (=company-ledger.el:110-111=) returns nil unless the buffer is + =beancount-mode= or derives from =ledger-mode=. +- *The reports pass =--strict=.* All five entries in =ledger-reports= + (=ledger-config.el:48-52=) use =--strict=, so a report over a file with an + undeclared account errors rather than silently inventing one. This is a real + guardrail and it is on. +- *The missing-binary check is at the right place.* =cj/executable-find-or-warn= + runs at =:config= (=ledger-config.el:54=), so a missing =ledger= CLI warns at load + rather than failing cryptically inside a report. +- *The clean-on-save hook is installed buffer-locally* (=ledger-config.el:40=), not + globally. Pinned by a test. + +* Gaps, as a list + +1. +No linting reaches ledger buffers (F1).+ Fixed 2026-07-10. +2. No confirmation before the date-sort on save (F2). Accepted: the sort is + chronological and stable. +3. No rollback when that rewrite fails partway (F3). +4. No confirmation before reconcile mutates the file (F4). +5. No validation that a save leaves the file balanced. +6. No test coverage before today. + +* What this note deliberately does not do + +It designs no guardrails. Whether a clean that fails should roll back, and what +shape a reconcile confirmation takes, are preference calls about Craig's own +accounting workflow. Those belong to the ledger guardrail UX task. + +The one finding that wasn't a preference call was F1: turning flycheck on in ledger +buffers restored a guardrail the module already claimed to have. That was a defect, +not a design choice, and it is fixed. + +F2 was resolved by reading the sort key rather than by changing code. What remains +for the guardrail task is F3 (no rollback when a clean fails partway) and F4 (no +confirmation before reconcile), plus the open question of whether a save should +verify the file still balances. diff --git a/docs/design/2026-07-10-org-module-ownership.org b/docs/design/2026-07-10-org-module-ownership.org new file mode 100644 index 00000000..5717b66f --- /dev/null +++ b/docs/design/2026-07-10-org-module-ownership.org @@ -0,0 +1,138 @@ +#+TITLE: Org module ownership and load boundaries +#+AUTHOR: Craig Jennings +#+DATE: 2026-07-10 + +* Why this note exists + +The Org workflow is spread across sixteen modules with overlapping +responsibilities. The code works. What's missing is a map: which module owns which +behavior, what each one adds to a shared variable, and in what order they load. + +Future load-order work needs that map. Four modules add capture templates, four +touch =org-agenda-files=, and five register org-protocol handlers. None of that is +wrong, but none of it is written down either, so the only way to answer "who sets +this?" is to grep. + +This is a description of what is, not a proposal. Where the map turned up something +that looks like a defect, it's flagged at the bottom rather than fixed here. + +* The modules + +Every Org module declares =Layer=, =Category=, and =Load shape= in its commentary +header. The layer numbers below come from those headers, not from this note. + +| Module | Layer | Owns | +|------------------------------+-------+---------------------------------------------------------| +| =org-faces-config.el= | 2 | Agenda keyword and priority faces, plus their -dim | +| | | variants for unfocused windows | +|------------------------------+-------+---------------------------------------------------------| +| =org-config.el= | 3 | Base org-mode settings, the org keymap, tag alignment | +|------------------------------+-------+---------------------------------------------------------| +| =org-agenda-config.el= | 3 | Agenda views, task tracking, notifications; loads | +| | | =org-agenda-config-debug= on demand | +|------------------------------+-------+---------------------------------------------------------| +| =org-capture-config.el= | 3 | The base capture templates and their dispatch keys | +|------------------------------+-------+---------------------------------------------------------| +| =org-refile-config.el= | 3 | Refile targets and the cached target list | +|------------------------------+-------+---------------------------------------------------------| +| =org-roam-config.el= | 3 | Roam database, capture, and its agenda-file additions | +|------------------------------+-------+---------------------------------------------------------| +| =org-contacts-config.el= | 3 | Contacts storage and its capture template | +|------------------------------+-------+---------------------------------------------------------| +| =org-babel-config.el= | 3 | Source-block languages and tempo templates | +|------------------------------+-------+---------------------------------------------------------| +| =org-export-config.el= | 3 | Export backends and their defaults | +|------------------------------+-------+---------------------------------------------------------| +| =org-spec-links.el= | 3 | Resolves =[[id:]]= links into project spec docs | +|------------------------------+-------+---------------------------------------------------------| +| =hugo-config.el= | 3 | ox-hugo blog publishing (=C-; h=) | +|------------------------------+-------+---------------------------------------------------------| +| =org-drill-config.el= | 4 | Spaced repetition, its own refile targets | +|------------------------------+-------+---------------------------------------------------------| +| =org-noter-config.el= | 4 | PDF and EPUB annotation | +|------------------------------+-------+---------------------------------------------------------| +| =org-reveal-config.el= | 4 | reveal.js presentations (=C-; p=) | +|------------------------------+-------+---------------------------------------------------------| +| =org-webclipper.el= | 4 | org-protocol web clipping into roam | +|------------------------------+-------+---------------------------------------------------------| +| =quick-video-capture.el= | 4 | Video download via an org-protocol bookmark | +|------------------------------+-------+---------------------------------------------------------| + +* Shared variables and who writes to them + +This is the part grep answers slowly and a table answers fast. Each of these +variables is appended to by more than one module. + +** =org-capture-templates= — four writers + +=org-capture-config.el= establishes the base set. =org-contacts-config.el=, +=org-webclipper.el=, and =quick-video-capture.el= each append their own. Dispatch +keys must not collide across all four, which is why +=tests/test-org-capture-templates-integrity.el= exists: it loads the cleanly +loadable capture modules, applies their additions, and asserts no two templates +share a key. + +That test is the enforcement mechanism for a boundary no single module can see. + +** =org-agenda-files= — four writers + +=org-agenda-config.el= sets the base list. =org-roam-config.el= adds roam files. +=config-utilities.el= manipulates it, and =org-agenda-config-debug.el= reads it for +diagnostics. + +** org-protocol handlers — five registrars + +=org-config.el=, =org-capture-config.el=, =org-drill-config.el=, +=org-webclipper.el=, and =quick-video-capture.el= all touch org-protocol. The +handlers are keyed by protocol name, so collisions are possible in principle and +nothing checks for them today. + +** =org-refile-targets= — two writers + +=org-refile-config.el= owns the general targets. =org-drill-config.el= adds its +own, because drill files are refile destinations but not agenda files. + +* Load order + +=init.el= loads the Org stack at lines 130 through 145, in this order: base config, +faces, agenda, babel, capture, contacts, drill, export, hugo, reveal, refile, roam, +spec-links, webclipper, noter. + +The order is deliberate in two places. =org-config= precedes everything because it +establishes the keymap the rest bind into. =org-faces-config= precedes +=org-agenda-config= because the agenda renders with those faces. + +One module sits outside that block. =quick-video-capture.el= loads at line 106, +twenty-four lines before =org-config=, because it is grouped with the media modules +rather than the Org modules. It nonetheless appends to =org-capture-templates=. It +works because the append is a plain =add-to-list= against a variable org-mode has +not yet claimed, but the grouping means a reader looking for "who adds capture +templates" in the Org block will not find it. + +* Load shape + +Every Org module declares =Load shape: eager=. Nothing in the Org stack defers. + +This is the single largest fact about the Org workflow's startup cost, and it is +worth stating plainly rather than leaving implicit across sixteen headers. Layer-4 +modules (drill, noter, reveal, webclipper, video capture) are optional features by +their own classification, and all five load eagerly on every launch. + +Whether that matters is a measurement question, not an argument, and this note does +not make it. It records the fact so the question can be asked. + +* Observations for follow-up + +These turned up while mapping. None is fixed here. + +1. =modules/org-config.el.faces.bak= is tracked in git (25KB). It appears to be a + leftover from the 2026-06-14 face-stripping work. A =.bak= file in =modules/= + is not loaded, but it is on the load-path's directory and will confuse the next + reader. +2. =org-agenda-config-debug.el= carries no =Layer= / =Category= / =Load shape= + header, unlike every other module. It loads conditionally from + =org-agenda-config.el:42= when =cj/debug-modules= names it, so its absence from + =init.el= is by design, not an orphan. +3. Nothing checks org-protocol handler collisions the way the capture-template test + checks key collisions. Five modules register handlers. +4. Every Layer-4 Org module loads eagerly despite being classified optional. diff --git a/docs/design/signal-client-review.org b/docs/design/signal-client-review.org index 7e8a73e9..e1ac462e 100644 --- a/docs/design/signal-client-review.org +++ b/docs/design/signal-client-review.org @@ -5,7 +5,7 @@ * Scope reviewed - =.ai/workflows/spec-review.org=. -- =docs/specs/signal-client-spec-doing.org=, including the base design, open-question dispositions, initiate-message workflow, architecture additions, accepted caveats, test plan, scope summary, and readiness rubric. +- =docs/specs/signal-client-spec.org=, including the base design, open-question dispositions, initiate-message workflow, architecture additions, accepted caveats, test plan, scope summary, and readiness rubric. - =modules/signal-config.el=, including =cj/signal--parse-contacts=, notify-suppression helpers, private config loading, and current =use-package signel= wiring. - =~/code/signel/signel.el=, including =signel-start=, =signel--send-rpc=, =signel--dispatch=, =signel--handle-error=, =signel--handle-receive=, =signel--insert-msg=, =signel--insert-system-msg=, =signel--send-input=, =signel-chat=, and dashboard commands. - =tests/test-signal-config.el=, covering contact parsing and notify-suppression helpers. diff --git a/docs/design/subr-mock-migration.org b/docs/design/subr-mock-migration.org new file mode 100644 index 00000000..874d03ef --- /dev/null +++ b/docs/design/subr-mock-migration.org @@ -0,0 +1,158 @@ +#+TITLE: Spec: Migrate Tests Off Mocking C Primitives +#+AUTHOR: Craig Jennings +#+DATE: 2026-06-30 +#+STATUS: Draft — for discussion + +* Status + +Draft. Pulled out of =todo.org= (=** TODO [#C] Migrate tests off mocking +primitives (native-comp robustness) :test:refactor:solo:=) so the scope and +approach can be settled before any code moves. Execution is deferred; this +document is the discussion vehicle. + +Companion reference: [[file:../native-comp-subr-mocking.org][native-comp-subr-mocking.org]] holds the full mechanism, the +upstream research, and the 2026-06-21 decision. This spec does not restate the +mechanism; it plans the remaining work that decision deferred. + +* Background — how we hit this + +We re-enabled native compilation config-wide (early-init.el, commit 3fd28987, +2026-06-20). Tests that had been green for months immediately started failing +with no change to their source — the first 8 were window-primitive mocks in +=test-dirvish-config-wrappers.el= and =test-calibredb-epub-config.el= +(=window-body-width=, =window-margins=, =current-window-configuration=, +=get-buffer-window=), throwing =wrong-number-of-arguments= for a zero-arg mock +lambda called with one argument. + +** What we struggled with (the consequences) + +- *Intermittent, non-deterministic failure.* The same test passed, then + crashed, then passed again within a session. Native-comp generates a + per-primitive trampoline =.eln= lazily and caches it on disk; whether a mock + "works" depends on whether that trampoline has been built yet. The + non-determinism was the tell, and it made the failures hard to trust or + reproduce. +- *Three distinct failure modes from one cause* (full detail in the companion + doc): (1) trampoline generation failure under =--batch=; (2) silent bypass — + natively-compiled callers ignore the mock and run the real primitive, so a + test passes for the wrong reason; (3) arity mismatch — the trampoline calls + the mock with the primitive's *maximum* arity, so a fixed-arity mock narrower + than the primitive throws. Mode 3 is the one that bit us; modes 1 and 2 sit + latent. +- *The tempting quick fix is the dangerous one.* Disabling subr trampolines + (=native-comp-enable-subr-trampolines nil=) is the most-cited workaround, but + in our native-comp-heavy setup it produces mode 2 — tests that pass while + asserting against the real primitive. A quiet false pass is worse than a loud + crash. +- *Scale of the latent surface.* The suite mocks subrs in hundreds of places. + The variadic sweep touched 188 arity-narrow mocks. + +** What is already done (the stopgap, 2026-06-21) + +Not currently broken — two things shipped (commits 571da499, b62c3c88): + +1. *Variadic sweep.* Every arity-narrow subr mock got =&rest _= appended, which + tolerates the trampoline's full-arity call. Deterministic, keeps trampolines + on, so no silent bypass. Fixes mode 3. +2. *Meta-test gate.* =tests/test-meta-subr-mock-arity.el= statically scans every + test file for =symbol-function= / =fset= / =setf= subr redefinitions and + fails =make test= if any mock can't accept the primitive's maximum arity. A + new arity-narrow mock can't merge silently. + +The stopgap fixes the mode we actually suffered. It leaves modes 1 and 2 latent. +The durable fix the ecosystem (and our own =elisp-testing.md=) points to is to +*not redefine primitives at all*. That is the work this spec scopes. + +* The real scope — most mocks should NOT move + +The raw inventory is large, but the headline number is misleading. =testing.md= +says to mock external boundaries; converting those to "drive real state" would +mean running real shells and touching the real filesystem in unit tests — the +opposite of what we want. So the actual migration target is narrow. + +Current subr-mock sites across 261 test files (=cl-letf= / =fset= / =advice-add= +on the named primitive): + +| Primitive | Sites | Classification | Disposition | +|-------------------------+-------+-------------------------+-----------------------------------| +| shell-command-to-string | 62 | external boundary | keep mocked (variadic) | +|-------------------------+-------+-------------------------+-----------------------------------| +| executable-find | 60 | external boundary | keep mocked (variadic) | +|-------------------------+-------+-------------------------+-----------------------------------| +| shell-command | 29 | external boundary | keep mocked (variadic) | +|-------------------------+-------+-------------------------+-----------------------------------| +| call-process | 17 | external boundary | keep mocked (variadic) | +|-------------------------+-------+-------------------------+-----------------------------------| +| current-time | 11 | time boundary | keep mocked (variadic) | +|-------------------------+-------+-------------------------+-----------------------------------| +| save-buffer | 10 | file I/O boundary | keep, or real temp-file fixture | +|-------------------------+-------+-------------------------+-----------------------------------| +| write-region | 4 | file I/O boundary | keep, or real temp-file fixture | +|-------------------------+-------+-------------------------+-----------------------------------| +| message | 69 | output-silencing | keep mocked (variadic) | +|-------------------------+-------+-------------------------+-----------------------------------| +| completing-read | 25 | UI prompt | MIGRATE — extract pure internal | +|-------------------------+-------+-------------------------+-----------------------------------| +| read-string | 16 | UI prompt | MIGRATE — extract pure internal | +|-------------------------+-------+-------------------------+-----------------------------------| +| yes-or-no-p | 14 | UI prompt | MIGRATE — extract pure internal | +|-------------------------+-------+-------------------------+-----------------------------------| +| read-from-minibuffer | 6 | UI prompt | MIGRATE — extract pure internal | +|-------------------------+-------+-------------------------+-----------------------------------| + +The genuine migration target is the UI-prompt bucket: ~61 sites. Per +=elisp-testing.md='s Interactive-vs-Internal rule, the fix is to extract a pure +internal that takes the value as an argument and test that directly, leaving the +interactive wrapper a thin un-tested (or smoke-tested) shell. That removes the +prompt mock entirely — immune to all three failure modes — and improves the +production code's testability as a side effect. + +Boundary mocks (shell, file I/O, time, =executable-find=, =call-process=) stay +mocked: that is correct unit-test practice, and the variadic form already +handles native-comp. =message= is output-silencing, not logic — keep it. + +* Proposed approach + +Not a single sweep. The migration touches production code (each extraction is a +small design change), so it is incremental and reviewable, not mechanical. + +1. *Scoped exemplar pass first.* Pick one module with a few prompt-mocks, do the + extract-internal conversion there, set the pattern, and measure the per-case + effort. This calibrates the rest. +2. *Batch by module afterward.* Convert remaining UI-prompt sites a module at a + time, each its own commit, with the suite green between. +3. *Leave boundaries alone.* No conversion of shell / file / time / process + mocks. The meta-test keeps them arity-safe. + +* Open decisions (resolve in discussion) + +** TODO Confirm the scope: UI-prompt mocks only, boundaries stay +Is the migration scoped to the ~61 UI-prompt sites (completing-read, read-string, +yes-or-no-p, read-from-minibuffer), with all boundary mocks explicitly out of +scope? Or is there an appetite to also convert the file-I/O mocks +(=save-buffer=, =write-region=) to real temp-file fixtures where it reads +cleaner? + +** TODO Reframe the todo.org task title to match the real scope +The current title — "Migrate tests off mocking primitives" — reads as all 300+ +sites. If we agree on UI-prompt-only, retitle to something like "Extract pure +internals for UI-prompt-mocked tests" so a future session does not re-scope it +as a wholesale sweep. + +** TODO Pick the exemplar module for the first pass +Which module gets the calibrating conversion? A small one with 2-4 prompt-mocks +is ideal. Candidate selection needs a per-module breakdown of the ~61 sites +(not yet collected). + +** TODO Decide priority and timing +Currently =[#C]=, =:solo:=. The suite is not broken (stopgap holds), so this is +test-quality debt, not urgent. Confirm it stays low and gets done in batches +between other work, rather than as a dedicated push. + +* Non-goals + +- Re-deriving or re-documenting the native-comp trampoline mechanism (see the + companion doc). +- Converting boundary mocks (shell, file I/O, time, process, executable-find). +- Removing the variadic-mock convention or the meta-test gate — both stay; they + are the standing protection for every mock that legitimately remains. diff --git a/docs/design/utility-inventory.org b/docs/design/utility-inventory.org index 8438a592..9b811429 100644 --- a/docs/design/utility-inventory.org +++ b/docs/design/utility-inventory.org @@ -4,7 +4,7 @@ * Status -Living inventory. Phase 1 of [[id:fc2e3926-b4a1-4b45-92eb-20841e13f655][utility-consolidation-spec-doing.org]]. Records the current state of helpers identified in the spec's Candidate Extraction Table plus any new candidates discovered during module walkthroughs. Decisions become concrete tasks in =todo.org= for Phase 2+. +Living inventory. Phase 1 of [[id:fc2e3926-b4a1-4b45-92eb-20841e13f655][utility-consolidation-spec.org]]. Records the current state of helpers identified in the spec's Candidate Extraction Table plus any new candidates discovered during module walkthroughs. Decisions become concrete tasks in =todo.org= for Phase 2+. * Scope @@ -82,7 +82,7 @@ Caller counts in the inventory below reflect grep results from 2026-05-10. The c | Symbol | File | Vis | Deps | Side effects | Proposed home / name | Callers (modules) | Tests | Pri | Decision | Rationale | |--------+------+-----+------+--------------+----------------------+-------------------+-------+-----+----------+-----------| -| =cj/modeline-vc-cache-*= helpers (key/get/put/clear/valid-p) | =modeline-config.el:108-140= | private | buffer-local vars | mutates buffer-local state | =cj-cache.el= / =cj/cache-valid-p=, =cj/cache-get=, =cj/cache-put=, =cj/cache-clear= | 1 (within file) | =test-modeline-config-vc-cache.el= | Medium | Defer | Good pattern, but variable-local cache shape differs from the agenda/refile caches. Needs design before extraction. Spec calls out a Phase 5 design addendum at =docs/specs/cache-helper-design-spec-implemented.org=. | +| =cj/modeline-vc-cache-*= helpers (key/get/put/clear/valid-p) | =modeline-config.el:108-140= | private | buffer-local vars | mutates buffer-local state | =cj-cache.el= / =cj/cache-valid-p=, =cj/cache-get=, =cj/cache-put=, =cj/cache-clear= | 1 (within file) | =test-modeline-config-vc-cache.el= | Medium | Defer | Good pattern, but variable-local cache shape differs from the agenda/refile caches. Needs design before extraction. Spec calls out a Phase 5 design addendum at =docs/specs/cache-helper-design-spec.org=. | | agenda/refile cache vars and build flags | =org-agenda-config.el=, =org-refile-config.el= | n/a | timers, file scans | scans filesystem, sets vars | =cj-cache.el= / =cj/cache-value-or-rebuild= | 2 | none | Medium | Defer | TTL/build/invalidate lifecycle; higher risk than the modeline cache. Same Phase 5 work. | ** Logging / Warnings @@ -144,7 +144,7 @@ These become =todo.org= entries (or update existing ones) as Phase 2 starts. ** Deferred (track in =todo.org= but no commit yet) -- Cache abstraction (modeline + agenda/refile) -- needs Phase 5 design addendum at =docs/specs/cache-helper-design-spec-implemented.org=. +- Cache abstraction (modeline + agenda/refile) -- needs Phase 5 design addendum at =docs/specs/cache-helper-design-spec.org=. - =cj/--open-with-is-launcher-p= -- move when external-open ownership is finalized. - =cj/log-silently= rename -- low value; do during incidental =system-lib= work. - HTML/text helpers (=strip-html=, =clean-text=) -- defer until a second consumer. |