navicore/patch-prolog
1
0
Fork 0
Code Issues Pull requests Releases 11 Activity Actions

Done — all green (9 tests, clippy -D warnings, fmt). #10

Merged
navicore merged 1 commit from pr-9-followup into main 2026-06-16 16:06:06 +00:00
Conversation 1 Commits 1 Files changed 2 +14 -18
navicore commented 2026-06-16 16:03:41 +00:00
Owner
Copy link

Interactive clause entry and :load now share one path: the Clause dispatch arm
calls session.load_source(&c), which validates-whole → splits → appends
individual ordered entries. So:

  • Typing foo. bar. at the prompt now lands as two numbered buffer entries,
    exactly like consulting a file with those two clauses — the inconsistency
    review #1 flagged is gone.
  • A single cat(whiskers). still lands as one entry (split of one clause = one
    entry), so nothing changes for the common case.
  • The capitalization hint still fires on a typed typo (Fact(1). → whole-text
    parse fails → Err → hint on the leading identifier), and error line/col stays
    relative to what was typed since the entry is the whole text.

Session::add_clause is removed — load_source fully subsumes it, so there's now
a single, well-tested clause-ingestion path instead of two that could drift
(which was the underlying smell). Its doc note records that it serves both
:load and interactive entry.

That closes out the PR #9 review: block-comment test added, the false 0'. doc
claim corrected, and the add_clause/load_source divergence unified. The
remaining review notes (#4 fallback, #5 list width) I'm deliberately leaving
as-is per the earlier assessment.

Interactive clause entry and :load now share one path: the Clause dispatch arm calls session.load_source(&c), which validates-whole → splits → appends individual ordered entries. So: - Typing foo. bar. at the prompt now lands as two numbered buffer entries, exactly like consulting a file with those two clauses — the inconsistency review #1 flagged is gone. - A single cat(whiskers). still lands as one entry (split of one clause = one entry), so nothing changes for the common case. - The capitalization hint still fires on a typed typo (Fact(1). → whole-text parse fails → Err → hint on the leading identifier), and error line/col stays relative to what was typed since the entry is the whole text. Session::add_clause is removed — load_source fully subsumes it, so there's now a single, well-tested clause-ingestion path instead of two that could drift (which was the underlying smell). Its doc note records that it serves both :load and interactive entry. That closes out the PR #9 review: block-comment test added, the false 0'. doc claim corrected, and the add_clause/load_source divergence unified. The remaining review notes (#4 fallback, #5 list width) I'm deliberately leaving as-is per the earlier assessment.
Done — all green (9 tests, clippy -D warnings, fmt).
All checks were successful
CI - Linux / CI - Linux x86_64 (pull_request) Successful in 1m40s
Details
e54f5c9602
 
Interactive clause entry and :load now share one path: the Clause dispatch arm
  calls session.load_source(&c), which validates-whole → splits → appends
  individual ordered entries. So:

  - Typing foo. bar. at the prompt now lands as two numbered buffer entries,
  exactly like consulting a file with those two clauses — the inconsistency
  review #1 flagged is gone.
  - A single cat(whiskers). still lands as one entry (split of one clause = one
  entry), so nothing changes for the common case.
  - The capitalization hint still fires on a typed typo (Fact(1). → whole-text
  parse fails → Err → hint on the leading identifier), and error line/col stays
  relative to what was typed since the entry is the whole text.

  Session::add_clause is removed — load_source fully subsumes it, so there's now
  a single, well-tested clause-ingestion path instead of two that could drift
  (which was the underlying smell). Its doc note records that it serves both
  :load and interactive entry.

  That closes out the PR #9 review: block-comment test added, the false 0'. doc
  claim corrected, and the add_clause/load_source divergence unified. The
  remaining review notes (#4 fallback, #5 list width) I'm deliberately leaving
  as-is per the earlier assessment.
navicore deleted branch pr-9-followup 2026-06-16 16:06:06 +00:00
navicore commented 2026-06-16 23:42:44 +00:00
Author
Owner
Copy link

Review — :load/interactive unification (PR #9 follow-up)

Small, focused, and the right call. add_clause and load_source were doing nearly the same work via two paths — exactly the drift smell that the PR #9 review flagged. Collapsing the Clause arm onto load_source removes one of those paths instead of patching it, which is the better fix.

What's right

  • One ingestion path. A typed foo. bar. and a :loaded file with those two clauses now produce identical buffer state. :list reads consistently from both sources.
  • add_clause removed, not deprecated. No lingering second path to drift against. The load_source doc explicitly records that it serves both consumers — future-you will see that and not be tempted to add a new shortcut.
  • Block-comment test landed. block_comment_does_not_split_and_rides_with_clause covers both the between-clauses and the inside-paren cases. The behavior I asked for is now pinned.
  • Honest handling of the 0'. claim. Rather than asserting it with a test (which would have failed) or quietly ignoring it, the doc comment was rewritten to drop the unfounded claim. That's the right call — keep the contract honest, expand it only when there's a test to back it up.
  • Error path unchanged. load_source parses the whole text first, so Fact(1). still errors with line/col relative to what was typed, and capitalization_hint still fires. Verified in app.rs:217-220.

Small observations

1. dirty is now conditional on n > 0. load_source sets dirty = true only if split_clauses returned at least one chunk. The old add_clause set dirty unconditionally on a successful parse. Difference is observable only when a typed entry parses cleanly and split_clauses returns empty — e.g. comment-only input like % just a note. that classify routes to Clause. That input now: parses OK → returns Ok(0) → no clause added, no dirty, no recompile, no log line about "defined" (the !self.session.dirty branch fires and prints defined. (N in session)). Probably fine, but worth a deliberate test or a one-line comment in load_source documenting that 0 clauses is a no-op rather than a quiet success.

2. split_clauses doc still mentions only line/block comments and floats. The new doc trimmed 0'. (good) but the doc comment now reads like a complete enumeration of "things the tokenizer handles correctly." A reader might assume e.g. backquoted strings or 0'\n literals are also fine. Either say "via the real tokenizer, so anything the tokenizer treats as a non-Dot token doesn't split" (the actual invariant) or leave it implicit. Pedantic.

3. :list width and the unreachable split_clauses fallback — explicitly deferred per the commit note. Fine. Both remain in the followup pile from PR #9's review.

Net

The underlying smell (two clause-ingestion paths) is gone, the new path is exercised by the existing :load tests and every interactive Clause submission, and the one new test pins the case the prior review identified as untested. Ready to merge as-is; the dirty-when-empty edge case is the only thing I'd consider documenting before moving on.

## Review — `:load`/interactive unification (PR #9 follow-up) Small, focused, and the right call. `add_clause` and `load_source` were doing nearly the same work via two paths — exactly the drift smell that the PR #9 review flagged. Collapsing the Clause arm onto `load_source` removes one of those paths instead of patching it, which is the better fix. ### What's right - **One ingestion path.** A typed `foo. bar.` and a `:load`ed file with those two clauses now produce identical buffer state. `:list` reads consistently from both sources. - **`add_clause` removed, not deprecated.** No lingering second path to drift against. The `load_source` doc explicitly records that it serves both consumers — future-you will see that and not be tempted to add a new shortcut. - **Block-comment test landed.** `block_comment_does_not_split_and_rides_with_clause` covers both the between-clauses and the inside-paren cases. The behavior I asked for is now pinned. - **Honest handling of the `0'.` claim.** Rather than asserting it with a test (which would have failed) or quietly ignoring it, the doc comment was rewritten to drop the unfounded claim. That's the right call — keep the contract honest, expand it only when there's a test to back it up. - **Error path unchanged.** `load_source` parses the whole text first, so `Fact(1).` still errors with line/col relative to what was typed, and `capitalization_hint` still fires. Verified in `app.rs:217-220`. ### Small observations **1. `dirty` is now conditional on `n > 0`.** `load_source` sets `dirty = true` only if `split_clauses` returned at least one chunk. The old `add_clause` set dirty unconditionally on a successful parse. Difference is observable only when a typed entry parses cleanly *and* `split_clauses` returns empty — e.g. comment-only input like `% just a note.` that classify routes to `Clause`. That input now: parses OK → returns `Ok(0)` → no clause added, no dirty, no recompile, no log line about "defined" (the `!self.session.dirty` branch fires and prints `defined. (N in session)`). Probably fine, but worth a deliberate test or a one-line comment in `load_source` documenting that 0 clauses is a no-op rather than a quiet success. **2. `split_clauses` doc still mentions only line/block comments and floats.** The new doc trimmed `0'.` (good) but the doc comment now reads like a complete enumeration of "things the tokenizer handles correctly." A reader might assume e.g. backquoted strings or `0'\n` literals are also fine. Either say "via the real tokenizer, so anything the tokenizer treats as a non-Dot token doesn't split" (the actual invariant) or leave it implicit. Pedantic. **3. `:list` width and the unreachable `split_clauses` fallback** — explicitly deferred per the commit note. Fine. Both remain in the followup pile from PR #9's review. ### Net The underlying smell (two clause-ingestion paths) is gone, the new path is exercised by the existing `:load` tests *and* every interactive Clause submission, and the one new test pins the case the prior review identified as untested. Ready to merge as-is; the `dirty`-when-empty edge case is the only thing I'd consider documenting before moving on.
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
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!10
No description provided.