⏺ :edit is wired — clippy -D warnings, 10 tests, fmt, and full workspace all #12

Merged

navicore merged 2 commits from edit into main 2026-06-17 00:00:47 +00:00

Conversation 1 Commits 2 Files changed 2 +134 -1

navicore commented 2026-06-16 23:26:38 +00:00

Owner

Copy link

green.

How it works:

• :edit sets app.should_edit. The event loop (which owns the Terminal) sees it
  and runs edit_session: writes the buffer (session.source()) to a temp
  plgr-session-*.pl, suspends the TUI (leave raw mode + alternate screen, show
  cursor), runs $EDITOR (default vi, parsed with shlex so EDITOR="code --wait"
  and quoted paths work) on that file, then resumes the TUI and reloads.
• Reload is atomic. apply_edit runs the edited text through the same
  load_source (validate-whole → split → ordered entries) on a scratch session
  first; only on success does it replace the live buffer, report edited. (N
  clause(s)), and recompile. A syntactically broken edit is rejected with edit
  rejected (buffer unchanged): … and the old buffer + binary stay intact.
• .pl suffix on the temp file so your editor's Prolog ftplugin lights up.
• Editor-launch failure (bad $EDITOR) is reported into the transcript via
  app.note, not by corrupting the restored TUI.

This is your redefine/delete path: :edit, change or remove clauses (e.g. drop
the stray test. fact, fix a rule), save and quit, and it recompiles the new
buffer. :edit is now advertised in :help.

One caveat: the suspend/resume and editor handoff are inherently interactive,
so they're not in the automated tests (the reload logic underneath is
load_source, which is). Worth a hands-on spin: type a couple of clauses, :edit,
modify one, :wq, then :list and a query to confirm the recompile took — and
try saving a deliberately broken clause to see the buffer survive.

green. How it works: - :edit sets app.should_edit. The event loop (which owns the Terminal) sees it and runs edit_session: writes the buffer (session.source()) to a temp plgr-session-*.pl, suspends the TUI (leave raw mode + alternate screen, show cursor), runs $EDITOR (default vi, parsed with shlex so EDITOR="code --wait" and quoted paths work) on that file, then resumes the TUI and reloads. - Reload is atomic. apply_edit runs the edited text through the same load_source (validate-whole → split → ordered entries) on a scratch session first; only on success does it replace the live buffer, report edited. (N clause(s)), and recompile. A syntactically broken edit is rejected with edit rejected (buffer unchanged): … and the old buffer + binary stay intact. - .pl suffix on the temp file so your editor's Prolog ftplugin lights up. - Editor-launch failure (bad $EDITOR) is reported into the transcript via app.note, not by corrupting the restored TUI. This is your redefine/delete path: :edit, change or remove clauses (e.g. drop the stray test. fact, fix a rule), save and quit, and it recompiles the new buffer. :edit is now advertised in :help. One caveat: the suspend/resume and editor handoff are inherently interactive, so they're not in the automated tests (the reload logic underneath is load_source, which is). Worth a hands-on spin: type a couple of clauses, :edit, modify one, :wq, then :list and a query to confirm the recompile took — and try saving a deliberately broken clause to see the buffer survive.

navicore added 1 commit 2026-06-16 23:26:38 +00:00

⏺ :edit is wired — clippy -D warnings, 10 tests, fmt, and full workspace all ... 5142960d3c

green.

  How it works:
  - :edit sets app.should_edit. The event loop (which owns the Terminal) sees it
  and runs edit_session: writes the buffer (session.source()) to a temp
  plgr-session-*.pl, suspends the TUI (leave raw mode + alternate screen, show
  cursor), runs $EDITOR (default vi, parsed with shlex so EDITOR="code --wait"
  and quoted paths work) on that file, then resumes the TUI and reloads.
  - Reload is atomic. apply_edit runs the edited text through the same
  load_source (validate-whole → split → ordered entries) on a scratch session
  first; only on success does it replace the live buffer, report edited. (N
  clause(s)), and recompile. A syntactically broken edit is rejected with edit
  rejected (buffer unchanged): … and the old buffer + binary stay intact.
  - .pl suffix on the temp file so your editor's Prolog ftplugin lights up.
  - Editor-launch failure (bad $EDITOR) is reported into the transcript via
  app.note, not by corrupting the restored TUI.

  This is your redefine/delete path: :edit, change or remove clauses (e.g. drop
  the stray test. fact, fix a rule), save and quit, and it recompiles the new
  buffer. :edit is now advertised in :help.

  One caveat: the suspend/resume and editor handoff are inherently interactive,
  so they're not in the automated tests (the reload logic underneath is
  load_source, which is). Worth a hands-on spin: type a couple of clauses, :edit,
  modify one, :wq, then :list and a query to confirm the recompile took — and
  try saving a deliberately broken clause to see the buffer survive.

navicore commented 2026-06-16 23:51:51 +00:00

Author

Owner

Copy link

Review — :edit ($EDITOR handoff)

The architecture is right. App sets a flag, the loop owner (main) does the terminal-suspend dance because it owns the Terminal. apply_edit builds a scratch Session::default(), runs load_source on it, and only replaces self.session = next on success — that's exactly how to get atomic-on-failure without duplicating the load logic. The .pl suffix for ftplugin and the shlex parse for EDITOR="code --wait" are the right small touches.

Issues below, ordered by impact.

Real bugs

1. Editor exit status is ignored. main.rs:130-135 always reads the temp file and calls apply_edit whenever the editor process spawned, regardless of how it exited. So :q! in vim, :cq, Ctrl-C, or any abnormal exit still reloads the (unchanged) file and prints edited. (N clause(s)) — misleading at best. If a future editor wrapper exits non-zero after partially corrupting the file, this silently reloads garbage. Fix: check status.success() and skip apply_edit on non-success, logging edit aborted (editor exited N) instead.

2. Reload always recompiles, even when nothing changed. A user who runs :edit, scrolls around, and saves without changing anything still triggers a recompile and gets edited. (N clause(s)). Easy guard: stash the original app.session.source() before writing the temp file and compare against the read-back content; if equal, skip apply_edit entirely (or call it with a no_change_ok path that doesn't recompile). This composes with #1 — both want to short-circuit before the recompile.

Small observations

3. $VISUAL is not honored. Convention is $VISUAL → $EDITOR → vi. A user with EDITOR=vi but VISUAL=nvim (extremely common) would get vi here. One line in main.rs:118:

let editor = std::env::var("VISUAL")
    .or_else(|_| std::env::var("EDITOR"))
    .unwrap_or_else(|_| "vi".to_string());

4. apply_edit has no unit tests. The PR description correctly notes the suspend/resume is inherently interactive, but apply_edit itself is pure: &mut App + &str → state change. Two tests pin the contract this PR is built around:

• apply_edit("garbage(") → buffer unchanged, "edit rejected" logged.
• apply_edit("foo. bar.") → buffer replaced with 2 clauses, "edited" logged.

These would prevent a future refactor from quietly breaking the atomic-reject guarantee, which is the whole point of the scratch-session pattern.

5. App::note is App::log. app.rs:55-58 — the comment justifies the rename ("on behalf of the host"), but the body is self.log(msg) verbatim. If the intent is purely documentary (host vs. self), fine, but it reads as cruft. Either inline the call site (app.log(...)) or have note actually do something different (timestamped? prefixed?).

6. Empty-buffer edits recompile an empty program. A user who :%d and saves gets apply_edit("\n") → load_source returns Ok(0) → session.clauses is empty → recompile() fires with empty source. Whether the engine accepts an empty program is engine-dependent; if it does, you've replaced a useful session with an empty binary. Worth either rejecting with edit rejected: no clauses or, more permissively, accepting it as :reset (probably surprising). Minor — the cure (revert via :edit again) is in the user's hands.

7. Suspend uses let _ = for errors, resume uses ?. main.rs:111-113 vs 123-126. The asymmetry is actually correct (suspend errors are non-fatal; resume errors must abort), but it's worth a one-line comment to keep a future reader from "fixing" the inconsistency.

What's good

• Ownership split is clean. App doesn't know what a Terminal is. should_edit is the boundary; main owns the rest. Mirrors how :load and :save don't touch the terminal either.
• Atomic reload via scratch Session. Reusing load_source's validate-whole-then-append semantics instead of re-implementing them is exactly the right move. The PR #10 unification keeps paying off.
• shlex for $EDITOR. Handles EDITOR="code --wait", quoted paths, the lot. Filter-on-Option correctly falls through to vi for empty/whitespace $EDITOR.
• .pl suffix on the temp file. Lights up the user's Prolog ftplugin without ceremony.
• Panic hook still covers the suspend window. Even between disable_raw_mode and enable_raw_mode, a panic restores the terminal — verified by inspection of run()'s hook (re-disables raw + leaves alt screen, idempotently).
• :edit shows in :help. Surface now matches behavior.

Suggested order

Items 1 and 2 are silent-data-loss-class for the user's mental model ("did my edit take or not?"). Worth doing before merge — both are <10 lines. 3 and 4 are quick wins. 5, 6, 7 are commentary.

## Review — `:edit` ($EDITOR handoff) The architecture is right. `App` sets a flag, the loop owner (`main`) does the terminal-suspend dance because it owns the `Terminal`. `apply_edit` builds a scratch `Session::default()`, runs `load_source` on it, and only replaces `self.session = next` on success — that's exactly how to get atomic-on-failure without duplicating the load logic. The `.pl` suffix for ftplugin and the shlex parse for `EDITOR="code --wait"` are the right small touches. Issues below, ordered by impact. ### Real bugs **1. Editor exit status is ignored.** `main.rs:130-135` always reads the temp file and calls `apply_edit` whenever the editor process *spawned*, regardless of how it exited. So `:q!` in vim, `:cq`, Ctrl-C, or any abnormal exit still reloads the (unchanged) file and prints `edited. (N clause(s))` — misleading at best. If a future editor wrapper exits non-zero after partially corrupting the file, this silently reloads garbage. Fix: check `status.success()` and skip `apply_edit` on non-success, logging `edit aborted (editor exited N)` instead. **2. Reload always recompiles, even when nothing changed.** A user who runs `:edit`, scrolls around, and saves without changing anything still triggers a recompile and gets `edited. (N clause(s))`. Easy guard: stash the original `app.session.source()` before writing the temp file and compare against the read-back content; if equal, skip `apply_edit` entirely (or call it with a `no_change_ok` path that doesn't recompile). This composes with #1 — both want to short-circuit before the recompile. ### Small observations **3. `$VISUAL` is not honored.** Convention is `$VISUAL` → `$EDITOR` → `vi`. A user with `EDITOR=vi` but `VISUAL=nvim` (extremely common) would get vi here. One line in `main.rs:118`: ```rust let editor = std::env::var("VISUAL") .or_else(|_| std::env::var("EDITOR")) .unwrap_or_else(|_| "vi".to_string()); ``` **4. `apply_edit` has no unit tests.** The PR description correctly notes the suspend/resume is inherently interactive, but `apply_edit` itself is pure: `&mut App` + `&str` → state change. Two tests pin the contract this PR is built around: - `apply_edit("garbage(")` → buffer unchanged, "edit rejected" logged. - `apply_edit("foo. bar.")` → buffer replaced with 2 clauses, "edited" logged. These would prevent a future refactor from quietly breaking the atomic-reject guarantee, which is the whole point of the scratch-session pattern. **5. `App::note` is `App::log`.** `app.rs:55-58` — the comment justifies the rename ("on behalf of the host"), but the body is `self.log(msg)` verbatim. If the intent is purely documentary (host vs. self), fine, but it reads as cruft. Either inline the call site (`app.log(...)`) or have `note` actually do something different (timestamped? prefixed?). **6. Empty-buffer edits recompile an empty program.** A user who `:%d` and saves gets `apply_edit("\n")` → `load_source` returns `Ok(0)` → `session.clauses` is empty → `recompile()` fires with empty source. Whether the engine accepts an empty program is engine-dependent; if it does, you've replaced a useful session with an empty binary. Worth either rejecting with `edit rejected: no clauses` or, more permissively, accepting it as `:reset` (probably surprising). Minor — the cure (revert via `:edit` again) is in the user's hands. **7. Suspend uses `let _ =` for errors, resume uses `?`.** `main.rs:111-113` vs `123-126`. The asymmetry is actually correct (suspend errors are non-fatal; resume errors must abort), but it's worth a one-line comment to keep a future reader from "fixing" the inconsistency. ### What's good - **Ownership split is clean.** `App` doesn't know what a `Terminal` is. `should_edit` is the boundary; `main` owns the rest. Mirrors how `:load` and `:save` don't touch the terminal either. - **Atomic reload via scratch `Session`.** Reusing `load_source`'s validate-whole-then-append semantics instead of re-implementing them is exactly the right move. The PR #10 unification keeps paying off. - **`shlex` for `$EDITOR`.** Handles `EDITOR="code --wait"`, quoted paths, the lot. Filter-on-`Option` correctly falls through to `vi` for empty/whitespace `$EDITOR`. - **`.pl` suffix on the temp file.** Lights up the user's Prolog ftplugin without ceremony. - **Panic hook still covers the suspend window.** Even between `disable_raw_mode` and `enable_raw_mode`, a panic restores the terminal — verified by inspection of `run()`'s hook (re-disables raw + leaves alt screen, idempotently). - **`:edit` shows in `:help`.** Surface now matches behavior. ### Suggested order Items 1 and 2 are silent-data-loss-class for the user's mental model ("did my edit take or not?"). Worth doing before merge — both are <10 lines. 3 and 4 are quick wins. 5, 6, 7 are commentary.

navicore added 1 commit 2026-06-16 23:58:31 +00:00

⏺ The review is useful — both claimed bugs are real, and I confirmed them in my ... 060336af59

own code before fixing. All green: 12 tests (2 new), clippy -D warnings, fmt.

  Bugs fixed:
  - #1 editor exit status ignored — edit_session reloaded on any spawn success.
  Now gated on status.success(): a clean exit with a real change reloads;
  :cq/abnormal exit logs :edit aborted (editor exited N) and leaves the buffer +
  binary untouched.
  - #2 always recompiled — now stashes the pre-edit source() and compares against
  the read-back; an unchanged save logs :edit — no changes and skips both the
  recompile (clang) and the misleading "edited" message. Composes with #1 — both
  short-circuit before recompile.

  Also done:
  - #3 $VISUAL — honored with the conventional VISUAL → EDITOR → vi precedence.
  - #4 tests — refactored apply_edit to expose a pure replace_session core
  (validate-on-scratch, swap-only-on-success, no recompile), so the atomic
  contract is now hermetically unit-tested: a bad edit (garbage() leaves the
  buffer untouched; a good multi-clause edit (foo. bar.) swaps in 2 split
  clauses. This was the right structural nudge — the recompile is no longer
  entangled with the testable reload logic.
  - #7 — added the comment explaining the deliberate suspend(let _)/resume(?)
  asymmetry.

  Deliberately left, with reasons:
  - #5 note is log — keeping it. log is private; note is the intentional public
  boundary for the host (main) to write to the transcript, which it now uses for
  all three :edit status messages. It's an API seam, not cruft.
  - #6 empty-buffer edit — leaving it. :%d + save clears the session (edited. (0
  clause(s))), which is a reasonable "clear via editor" and the message signals
  it; special-casing it would be more surprising than the behavior. The reviewer
  agreed it's minor.

  The reviewer's "what's good" notes also confirmed the architecture (ownership
  split, scratch-session atomicity, panic-hook coverage) holds up — those were
  the parts most worth a second pair of eyes.

navicore merged commit dfd8ae687f into main 2026-06-17 00:00:47 +00:00

navicore deleted branch edit 2026-06-17 00:00:47 +00:00

navicore referenced this pull request from a commit 2026-06-17 00:00:48 +00:00

Merge pull request '⏺ :edit is wired — clippy -D warnings, 10 tests, fmt, and full workspace all' (#12) from edit into main

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels

No items

No labels

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

navicore/patch-prolog!12

No description provided.