Skip to content

fix(engine): propagate history errors instead of panicking - #1173

Open
kronberger-droid wants to merge 2 commits into
nushell:mainfrom
kronberger-droid:refactor/engine-history-results
Open

fix(engine): propagate history errors instead of panicking#1173
kronberger-droid wants to merge 2 commits into
nushell:mainfrom
kronberger-droid:refactor/engine-history-results

Conversation

@kronberger-droid

Copy link
Copy Markdown
Collaborator

Summary

The ten expect("todo: error handling") in engine.rs all sit on history calls: HistoryCursor::back/forward, history.search, history.save.
Nine of them are on navigation or printing paths inside io::Result fns already, so with impl From<ReedlineError> for io::Error (the IOError variant is unwrapped so its kind survives) they become ?.
run_history_commands, previous_history, next_history, up_command and down_command grow io::Result<()>; their callers are handle_editor_event/handle_history_search_event, which already return io::Result.
read_line thus returns Err where it used to panic. No change to its signature.

The tenth, history.save in submit_buffer, is the one where ? is wrong: an Err from read_line would drop the command the user just typed over a history write.
Instead the entry is treated like an excluded one (FILTERED_ITEM_ID, history_excluded_item), so Up still recalls it and update_last_command_context updates it in memory, and the error is stashed.
New additive API: Reedline::take_history_save_error() -> Option<ReedlineError>, cleared on read, set at most once per read_line.
A caller that does not ask loses nothing; nushell can decide later whether to warn.

Before

A failing history backend panics the shell on Up/Down, ctrl-r, or Enter.

After

Navigation and search failures come back as io::Error from read_line; a failed save still returns Signal::Success(line) and the error waits in take_history_save_error.

…rror handling")

`impl From<ReedlineError> for io::Error` (unwrapping the `IOError` variant
so the kind survives) lets the nine history sites in `engine.rs` use `?`.
`run_history_commands`, `previous_history`, `next_history`, `up_command`
and `down_command` grow `io::Result<()>`; all their callers already sit
in `io::Result` fns, so `read_line` now returns `Err` where it used to
panic. `history.save` in `submit_buffer` is left for the next commit,
since failing there would lose the submitted command.
`submit_buffer` was the last `expect("todo: error handling")` and the one
where `?` is wrong: `read_line` returning `Err` would drop the command
the user just typed over a history write. Instead the entry is treated
like an excluded one (`FILTERED_ITEM_ID`, `history_excluded_item`), so
Up still recalls it and `update_last_command_context` updates it in
memory, and the error is stashed for `Reedline::take_history_save_error`.
Additive API; cleared on read, set at most once per `read_line`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant