feat: commandline edit --accept to instantly execute command
#13728
Conversation
|
CI can probably not build because of relative paths for reedline, right? We will have to wait until that one is merged until these jobs can succeed. |
There was a problem hiding this comment.
Thanks for giving this a try, as I mentioned on nushell/reedline#822 (comment) the change on reedline appears a bit overgeneralized to me.
| repl.buffer = str; | ||
| repl.cursor_pos = repl.buffer.len(); | ||
| } | ||
| if call.has_flag(engine_state, stack, "accept")? { | ||
| if let Ok(mut queue) = engine_state.reedline_event_queue.lock() { | ||
| queue.push(ReedlineEvent::Enter); |
There was a problem hiding this comment.
ReedlineEvent::Enter is not solitary confined to running the command.
- if a menu is active it will perform its accepting action instead
- it triggers a run of the
!history expression expansions if necessary - it will run the syntax validation to decide if it should insert a newline due to open expressions
- if not it will return the entered content
https://github.com/nushell/reedline/blob/020142f6ee857353c289119a04d8fc71c474ce59/src/engine.rs#L1085-L1120
so this implementation choice may have surprising behavior based on the state of the line editor (see nushell/reedline#821 for folks wanting to have an always open menu)
There was a problem hiding this comment.
I see, what do you suggest? Submit?
There was a problem hiding this comment.
Ooh and where do I find the code that does the history expansion? I would also give it a shot to use this feature to instantly accept a !! expansion.
nvm, I found it in reedline.
| @@ -137,7 137,7 @@ rand = "0.8" | |||
| rand_chacha = "0.3.1" | |||
| ratatui = "0.26" | |||
| rayon = "1.10" | |||
| reedline = "0.34.0" | |||
There was a problem hiding this comment.
If you use a git dependency pointing to your fork and branch, things can be tested both in CI and more easily by folks just fetching for a functional shakedown.
In general we tend to specify our non-crates-io dependencies only in the patch section at the bottom of the main Cargo.toml (to centralize those dependencies that we can't merge or publish like that, and as the workspace dependency only became a thing post 1.64)
There was a problem hiding this comment.
Yeah, I wouldn't spend too much time on this and clean it up completely when/if reedline is merged. Then I would only need to bump its version.
| fn commandline_test_accept() -> TestResult { | ||
| // Explanation: run_test captures a command's output. | ||
| // But `--accept` executes a new command that then outputs "accepted" | ||
| run_test("commandline edit --accept \"print accepted\"", "") |
There was a problem hiding this comment.
Shouldn't the expected value contain accepted?
There was a problem hiding this comment.
As stated in the comment above the code: commandline edit runs successfully without any output. Instead, it creates a new command that runs print accepted. Therefore, the command's output is empty and the successor's output will be "accepted".
There was a problem hiding this comment.
I can understand this but when I saw it, it seemed a bit weird that my history would have "commandline edit --accept "print accepted"" and then another entry for "print accepted". I mean, I understand it, but it was unexpected.
|
Oh and I completely forgot: thanks for your review! ❤️ |
|
I refactored to use a channel and use |
Example use case: atuin
I primarily wanted to see how difficult this feature is to implement. I would appreciate some feedback/review.
Description
This feature had to be split in two separate PRs because I had to also touch reedline.
The concept is fairly simple: I added a queue to which commands can write
ReedlineEvents to.Reedline however, was not developed with asynchronous events in mind.
So it waits for input with
event::read()without checking for new events in its queue.So I added a check whether the queue is empty before blocking and waiting for new input.
This way
commandline editsimply accepts a new flag, namely--acceptand whenever it detects this flag, it writes aReedlineEvent::Enterto the queue.To use:
gh repo clone mrdgo/nushellcd nushell; cargo runcommandline edit --accept "print hello"This is the reason, why I committed the versioning foo in all the
*.tomlfiles. These should of course not be merged to main.User-Facing Changes
New feature:
commandline editnow accepts a flag--accept/-Athat immediately executes the resulting command. As an atuin user, I would greatly appreciate.Tests Formatting
I added a simple test case and it only shows the absence of a regression because
run_testonly captures stdout from the command it runs.--acceptruns a new command whose output will not be captured byrun_test.How relevant will a test be? Couldn't we also run a
.nuscript to test this? Or maybe I access the event queue in the test and assert presence of aReedlineEvent::Enter?After Submitting