Add support for batch execution of command #360
Conversation
| cmd.arg(arg.generate("").as_ref()); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
If we add the following lines here:
cmd.stdin(Stdio::inherit());
cmd.stdout(Stdio::inherit());
cmd.stderr(Stdio::inherit());we can use --exec-batch with interactive terminal commands such as vim.
There was a problem hiding this comment.
It's indeed much better with these lines added.
| @@ -44,3 +44,16 @@ pub fn job( | |||
| cmd.generate_and_execute(&value, Arc::clone(&out_perm)); | |||
| } | |||
| } | |||
|
|
|||
| pub fn batch(rx: Receiver<WorkerResult>, cmd: &CommandTemplate, show_filesystem_errors: bool) { | |||
| let paths = rx.iter().filter_map(|value| match value { | |||
src/exec/mod.rs
Outdated
| /// Execution mode of the command | ||
| #[derive(Debug, Clone, Copy, PartialEq)] | ||
| pub enum ExecutionMode { | ||
| /// Command is executed for each path found |
src/exec/mod.rs
Outdated
| CommandTemplate { args, mode } | ||
| } | ||
|
|
||
| fn tokens_number(&self) -> usize { |
There was a problem hiding this comment.
Can we rename this to number_of_tokens or something similar? It sounds like it returns the number of a specific token.
src/exec/mod.rs
Outdated
| execute_command(cmd, &out_perm) | ||
| } | ||
|
|
||
| pub fn is_batch(&self) -> bool { |
There was a problem hiding this comment.
Maybe rename this to batch_mode or in_batch_mode?
tests/testenv/mod.rs
Outdated
| // Check for exit status. | ||
| if output.status.success() { | ||
| panic!( | ||
| "fd exited successfully. Expected error {} did not occur.", |
There was a problem hiding this comment.
can we use error '{}' did not occur. here?
|
|
||
| /// Assert that calling *fd* in the specified path under the root working directory, | ||
| /// and with the specified arguments produces an error with the expected message. | ||
| fn assert_error_subdirectory<P: AsRef<Path>>(&self, path: P, args: &[&str], expected: &str) { |
tests/testenv/mod.rs
Outdated
|
|
||
| // Compare actual output to expected output. | ||
| let actual = String::from_utf8_lossy(&output.stderr); | ||
| if expected.len() <= actual.len() && expected != &actual[..expected.len()] { |
There was a problem hiding this comment.
Why not just expected != actual? Is this stripping the [fd error] prefix?
There was a problem hiding this comment.
The errors regarding the conflict between exec and exec-batch is generated by clap… With the usage message.
This allows to have just the error message in the test.
There was a problem hiding this comment.
Okay. Could we also use ends_with(..) here?
There was a problem hiding this comment.
That would actually be start_with (as usage is after the error). And indeed, it would be cleaner!
tests/tests.rs
Outdated
| assert_exec_batch_output("--exec-batch"); | ||
| } | ||
|
|
||
| // Shell script execution using -x |
There was a problem hiding this comment.
I think this is redundant. If you want to keep it, please modify the docstring to use -X instead of -x.
tests/tests.rs
Outdated
| let (te, abs_path) = get_test_env_with_abs_path(DEFAULT_DIRS, DEFAULT_FILES); | ||
| let te = te.normalize_line(true); | ||
|
|
||
| // TODO Windows tests: D:file.txt \file.txt \\server\share\file.txt ... |
There was a problem hiding this comment.
I'm okay with leaving the Windows tests as To Do, but what does this comment describe?
There was a problem hiding this comment.
I must admit to a copy/paste (incl. this comment).
The cfg!(windows) clause is already present in the exec test, and being on linux right now, I couldn't try it out.
I guess the comment is mainly linked to the first test (absolute paths), which may have several forms on windows depending of file location (drive, network,…). But this is just a theory.
There was a problem hiding this comment.
And I must admit that I forgot about this part 😄. Let's just say TODO: Tests for Windows here.
|
Thanks for the review! Looking at: I only read from the I see a few options to handle this case:
Any opinions? I would be tempted by option 2, but not sure if there are some weird cases to consider. |
I would just go with option three. I think it doesn't make sense to allow placeholders in the first argument for |
|
Thank you very much for the updates! |
|
You're welcome! |
This aims at implementing #274
Note: for this first version, arg_max is not taken into account.