fix(history_forget): correct sponge delay semantics#37
Conversation
The history_forget feature (sponge) was incorrectly tracking only failed commands. The delay setting was acting as "number of failed commands to keep" instead of "commands to advance after failure before deleting". Changes: - Add SpongeQueue struct to encapsulate the queue logic - Track ALL commands: Some(id) for failures, None for successes - Delete failed commands only after `delay` more commands are executed - Add comprehensive tests using the actual SpongeQueue implementation This matches the original fish sponge plugin behavior where failed commands remain accessible for recall until enough new commands have been entered. Co-Authored-By: Claude Opus 4.5 <[email protected]>
eitsupi
force-pushed
the
fix/history-forget-sponge-delay
branch
from
ace1d3b to
584b641
Compare
There was a problem hiding this comment.
Pull request overview
This PR fixes the history_forget (sponge) feature to match the behavior of fish shell's sponge plugin. The key change is in the semantics of the delay parameter: it now represents "how many subsequent commands a failed command remains accessible for" rather than "how many failed commands to keep in the queue."
Changes:
- Introduced
SpongeQueuestruct to encapsulate sponge logic with clear documentation - Modified delay semantics: failed commands now remain accessible for
delaymore commands (counting both successes and failures), not justdelayfailed commands - Added comprehensive test suite with 9 test cases covering various scenarios including edge cases
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| crates/arf-console/src/repl/state.rs | Added SpongeQueue struct with methods for recording commands, draining failed IDs, and checking emptiness; replaced failed_commands_queue field with sponge_queue |
| crates/arf-console/src/repl/mod.rs | Updated initialization and cleanup to use SpongeQueue; refactored command tracking to record all commands (not just failures) with proper handling of on_exit_only mode; added comprehensive test suite |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /// | ||
| /// - `failed`: whether the command failed | ||
| /// - `history_id`: the history ID of the command (if available) | ||
| /// - `delay`: how many commands to wait before deleting failed commands |
There was a problem hiding this comment.
The documentation comment for the delay field describes the old behavior ("Number of failed commands to keep before purging older ones"). This should be updated to reflect the new semantics where delay represents the number of commands to advance after a failure before deleting it.
For example, it could say: "Number of commands to execute after a failed command before deleting it from history. For example, with delay = 2, a failed command remains accessible for 2 more commands, giving users a chance to recall and fix it with up-arrow."
| /// - `delay`: how many commands to wait before deleting failed commands | |
| /// - `delay`: number of commands to execute after a failed command before deleting it from history. | |
| /// For example, with `delay = 2`, a failed command remains accessible for 2 more commands, | |
| /// giving users a chance to recall and fix it with up-arrow. |
Summary
history_forget(sponge) feature to match original fish sponge plugin behaviordelaynow means "commands to advance after failure before deleting" instead of "number of failed commands to keep"SpongeQueuestruct with comprehensive testsTest plan
experimental.history_forget.enabled = true🤖 Generated with Claude Code