- History data: stores multiline expressions with real \n — correct… #74

Merged

navicore merged 1 commit from newline into main

2026-04-05 00:33:13 +00:00

navicore commented

2026-04-05 00:26:55 +00:00

(Migrated from github.com)

…, untouched

History file: escapes \n as \n — round-trips correctly across restarts
Editor buffer: flatten-newlines replaces \n with space on recall — vi motions work correctly on a single line
Renderer: original single-line renderer — no bugs

The multiline display with ... and indentation is deferred to the full multiline editor project (the 5-7 day design doc). For now, recalled expressions
are editable single lines with all vi motions working.

…, untouched - History file: escapes \n as \\n — round-trips correctly across restarts - Editor buffer: flatten-newlines replaces \n with space on recall — vi motions work correctly on a single line - Renderer: original single-line renderer — no bugs The multiline display with ... and indentation is deferred to the full multiline editor project (the 5-7 day design doc). For now, recalled expressions are editable single lines with all vi motions working.

claude[bot] commented

2026-04-05 00:29:59 +00:00

(Migrated from github.com)

PR Review: History Data vs Presentation Separation

This is a well-motivated fix. The design doc clearly articulates the problem (history entries containing real newlines being corrupted by the newline-as-delimiter storage format), and Option A (escape embedded newlines as \n, backslashes as \\) is a pragmatic choice that keeps the file human-readable.

The stack manipulation logic in the new routines traces correctly. Here are my observations:

Missing Tests

The biggest gap. test_history.seq tests basic add/get operations but has no coverage for the new functionality. Expected cases:

escape-newlines / unescape-newlines round-trip, including strings with literal backslashes
Multiline expression persistence round-trip: add a string with an embedded newline, call history-to-string, then history-load-lines, and verify the original string is recovered
flatten-newlines: verify a string with embedded newlines becomes space-separated
string-strip-leading: leading spaces/tabs/newlines, already-clean strings, all-whitespace string

The checkpoints in the design doc map directly to test cases -- translating them into assertions in test_history.seq would be a natural next step.

Backward Compatibility Gap

Existing ~/.seqlisp_history files that contain backslash sequences (e.g. a history entry like (display "\n")) will be misread on first load: the two-char \n will be unescaped to a real newline, corrupting that entry. This is a one-time migration issue that self-heals after the next save/load cycle, but worth a brief note in the design doc or a code comment near history-load-lines.

Comment Placement

string-strip-leading (line 338) was inserted immediately after this comment:

# Add entry to history (prepends to front = newest first)
# Skips empty strings and duplicates of the most recent entry

This makes the comment appear to describe the helper, not history-add. Moving the helper before that comment block (or giving it its own section header) would be cleaner.

Performance (minor)

All four new loop functions build strings one character at a time, giving O(n^2) behaviour due to string concatenation. For REPL history lines this is fine in practice, but worth noting if these routines are ever reused on larger inputs.

Logic Looks Correct

Key paths traced:

unescape-newlines-loop correctly handles \n -> newline, \\ -> backslash, and the single-char remainder edge case
escape-newlines-loop replaces \ -> \\ before checking newlines -> \n (order matters, and it is right)
string-strip-leading-loop correctly uses dup/2 pick to call string.char-at without consuming Str off the stack
The flatten-newlines calls at the two recall sites are in the right place: history data stays clean, only the editor buffer gets the space-flattened version

Summary

The approach is sound and the implementation is correct. Main ask before merging: add tests, especially the persistence round-trip. The backward compatibility note is minor but worth documenting somewhere.

## PR Review: History Data vs Presentation Separation This is a well-motivated fix. The design doc clearly articulates the problem (history entries containing real newlines being corrupted by the newline-as-delimiter storage format), and Option A (escape embedded newlines as `\n`, backslashes as `\\`) is a pragmatic choice that keeps the file human-readable. The stack manipulation logic in the new routines traces correctly. Here are my observations: --- ### Missing Tests The biggest gap. `test_history.seq` tests basic add/get operations but has no coverage for the new functionality. Expected cases: - `escape-newlines` / `unescape-newlines` round-trip, including strings with literal backslashes - Multiline expression persistence round-trip: add a string with an embedded newline, call `history-to-string`, then `history-load-lines`, and verify the original string is recovered - `flatten-newlines`: verify a string with embedded newlines becomes space-separated - `string-strip-leading`: leading spaces/tabs/newlines, already-clean strings, all-whitespace string The checkpoints in the design doc map directly to test cases -- translating them into assertions in `test_history.seq` would be a natural next step. --- ### Backward Compatibility Gap Existing `~/.seqlisp_history` files that contain backslash sequences (e.g. a history entry like `(display "\n")`) will be misread on first load: the two-char `\n` will be unescaped to a real newline, corrupting that entry. This is a one-time migration issue that self-heals after the next save/load cycle, but worth a brief note in the design doc or a code comment near `history-load-lines`. --- ### Comment Placement `string-strip-leading` (line 338) was inserted immediately after this comment: # Add entry to history (prepends to front = newest first) # Skips empty strings and duplicates of the most recent entry This makes the comment appear to describe the helper, not `history-add`. Moving the helper before that comment block (or giving it its own section header) would be cleaner. --- ### Performance (minor) All four new loop functions build strings one character at a time, giving O(n^2) behaviour due to string concatenation. For REPL history lines this is fine in practice, but worth noting if these routines are ever reused on larger inputs. --- ### Logic Looks Correct Key paths traced: - `unescape-newlines-loop` correctly handles `\n` -> newline, `\\` -> backslash, and the single-char remainder edge case - `escape-newlines-loop` replaces `\` -> `\\` before checking newlines -> `\n` (order matters, and it is right) - `string-strip-leading-loop` correctly uses `dup`/`2 pick` to call `string.char-at` without consuming `Str` off the stack - The `flatten-newlines` calls at the two recall sites are in the right place: history data stays clean, only the editor buffer gets the space-flattened version --- ### Summary The approach is sound and the implementation is correct. Main ask before merging: add tests, especially the persistence round-trip. The backward compatibility note is minor but worth documenting somewhere.