Accept Command with args for BufferEditor

[horasal]

Thin pr contains the reedline-side change for nushell/nushell#10210

BufferEditor in current reedline only use a big string to represent editor and it's arguments, which is broken when editor's path contains any spaces. This pr make BufferEditor also accept list<string> from $nu.EDITOR just like config env.

(To avoid breaking current version of nushell, this pr add a new api with_argumented_buffer_editor instead of directly modify with_buffer_editor.)

[sholderbach]

Thanks for taking care of this!

Thanks for being considerate about breaking changes. In this case I think to make errors less likely I would be open to one.

To keep things simple and tidy, we could pass std::process::Command directly. It communicates the requirements very clearly, is Send/Sync, and you can add the file just by .arg() and then .spawn()

[horasal]

Personally I think just store a set of string is more flexible so reedline can implement its own feature like hints for argument (e.g. throw warning when call vscode without -w) or editor selection (show a list of editor if no one is set).
(I'm no sure if it's in reedline's scope or not, though.)

and I also agree that Command sounds good enough for open_editor.

[sholderbach]

I think as a library reedline doesn't have to take full responsibility for that. If you want to implement additional checks for the editor you would also have to implement them on the nushell (or other app) side if there is any use of an editor.
Command makes this clean, that it is the responsibility of the caller.

[horasal]

Command makes this clean,

I don't think it's true.

Command does not implement Clone there's no way to insert temp_file without affecting current instance. Of course you can construct another Command by Command::new(old_command.get_program().to_string_lossy()) but why.

[sholderbach]

Mhh open_editor currently only ever uses one temp_file through extension.
So taking the Command by value would be fine with the current behavior. Yes there is a slight burden on the user of reedline to properly set this up.

fn with_buffer_editor(mut self, editor: Command, ....) -> Self

Instead of extension we could also have

fn with_buffer_editor(mut self, editor: Command, tempfile: impl AsRef<OsStr>) -> Self

so the file name is not dependent on reedline and can be configured to please the application. But then the responsibility to deconflict multiple Reedline instances is on the caller.

A strong argument for Command is that this would allow us to resolve nushell/nushell#10241 by also managing the environment through the same interface.

[horasal]

I get your point and now the new api only accepts Command

• Command should always contains full arguments, including the path of temp file
• temp_file used in Command should match the one passed in with_buffer_editor_command

[sholderbach]

Thanks for the quick turnaround, with a cargo fmt run we are pretty much good to go.

I think, as we will bump the leading version number, we would be fine with just making the breaking change to with_buffer_editor, so we don't need to maintain two functions.

temp_file used in Command should match the one passed in with_buffer_editor_command

We could check that .get_args() contains temp_file :)

[codecov]

Codecov Report

Merging #630 (5874cd6) into main (6739c6a) will increase coverage by 0.00%.
Report is 5 commits behind head on main.
The diff coverage is 0.00%.

@@           Coverage Diff           @@
##             main     #630   +/-   ##
=======================================
  Coverage   49.80%   49.80%           
=======================================
  Files          41       41           
  Lines        7805     7821   +16     
=======================================
+ Hits         3887     3895    +8     
- Misses       3918     3926    +8     

Files	Coverage Δ
src/engine.rs	5.13% <0.00%> (+<0.01%)	⬆️

... and 2 files with indirect coverage changes

[sholderbach]

I took the liberty to move the new flexible but explicit behavior into the .with_buffer_editor method.
Added a check to see if the file name is already added.

Thanks for getting us in a good shape here @horasal!