From 19beafa865cb00c2edcbf7d53b5988372288af0c Mon Sep 17 00:00:00 2001 From: Artemiy Date: Fri, 17 Mar 2023 01:53:46 +0300 Subject: [PATCH] Disable pipeline echo (#8292) # Description Change behavior of block evaluation to not print result of intermediate commands. Previously result of every but last pipeline in a block was printed to stdout, and last one was returned ![image](https://user-images.githubusercontent.com/17511668/222550110-3f62fbed-432c-4b46-b9b1-4cb45a1f893e.png) With this change results of intermediate pipelines are discarded after they finish and the last one is returned as before: ![image](https://user-images.githubusercontent.com/17511668/222550346-f1e74f80-f6b6-4aa3-98d6-888ea4cb4915.png) Now one should use `print` explicitly to print something to stdout ![image](https://user-images.githubusercontent.com/17511668/222923955-fda0d77b-41b4-4f91-a80f-12b0a1880c05.png) **Note, that this behavior is not limited to functions!** The scope of this change are all blocks. All of the below are executed as blocks and thus exibited this behavior in the same way: ![image](https://user-images.githubusercontent.com/17511668/222924062-342c15de-4273-4bf5-8b39-fe6e3aa96076.png) With this change outputs for all types of blocks are cleaned: ![image](https://user-images.githubusercontent.com/17511668/222924118-7d51c27e-04bb-43e5-8efe-38b484683bfe.png) # User-Facing Changes All types of blocks (function bodies, closures, `if` branches, `for` and `loop` bodies e.t.c.) no longer print result of intermediate pipelines. # Tests + Formatting Don't forget to add tests that cover your changes. Make sure you've run and fixed any issues with these commands: - `cargo fmt --all -- --check` to check standard code formatting (`cargo fmt --all` applies these changes) - `cargo clippy --workspace -- -D warnings -D clippy::unwrap_used -A clippy::needless_collect` to check that you're using the standard code style - `cargo test --workspace` to check that all tests pass # After Submitting If your PR had any user-facing changes, update [the documentation](https://github.com/nushell/nushell.github.io) after the PR is merged, if necessary. This will help us keep the docs up to date. --- crates/nu-command/tests/commands/for_.rs | 2 +- crates/nu-command/tests/commands/loop_.rs | 2 +- crates/nu-command/tests/commands/open.rs | 8 +-- crates/nu-command/tests/commands/try_.rs | 4 +- crates/nu-command/tests/commands/while_.rs | 2 +- crates/nu-command/tests/commands/with_env.rs | 10 +-- crates/nu-engine/src/eval.rs | 74 ++------------------ crates/nu-protocol/src/pipeline_data.rs | 20 ++++++ crates/nu-protocol/src/value/stream.rs | 23 ++++++ tests/fixtures/formats/script_multiline.nu | 4 +- tests/shell/pipeline/commands/internal.rs | 26 +++---- 11 files changed, 76 insertions(+), 99 deletions(-) diff --git a/crates/nu-command/tests/commands/for_.rs b/crates/nu-command/tests/commands/for_.rs index e321a15cb1..4952f8dd75 100644 --- a/crates/nu-command/tests/commands/for_.rs +++ b/crates/nu-command/tests/commands/for_.rs @@ -21,7 +21,7 @@ fn for_break_on_external_failed() { cwd: ".", r#" for i in 1..2 { - echo 1; + print 1; nu --testbin fail }"# ); diff --git a/crates/nu-command/tests/commands/loop_.rs b/crates/nu-command/tests/commands/loop_.rs index b27c3e34b8..16d6542249 100644 --- a/crates/nu-command/tests/commands/loop_.rs +++ b/crates/nu-command/tests/commands/loop_.rs @@ -33,7 +33,7 @@ fn loop_break_on_external_failed() { } else { $total += 1; } - echo 1; + print 1; nu --testbin fail; }"# ); diff --git a/crates/nu-command/tests/commands/open.rs b/crates/nu-command/tests/commands/open.rs index bd7652cebf..5de381e78e 100644 --- a/crates/nu-command/tests/commands/open.rs +++ b/crates/nu-command/tests/commands/open.rs @@ -262,10 +262,10 @@ fn test_open_block_command() { r#" def "from blockcommandparser" [] { lines | split column ",|," } let values = (open sample.blockcommandparser) - echo ($values | get column1 | get 0) - echo ($values | get column2 | get 0) - echo ($values | get column1 | get 1) - echo ($values | get column2 | get 1) + print ($values | get column1 | get 0) + print ($values | get column2 | get 0) + print ($values | get column1 | get 1) + print ($values | get column2 | get 1) "# ); diff --git a/crates/nu-command/tests/commands/try_.rs b/crates/nu-command/tests/commands/try_.rs index 0af8df34da..064c897af8 100644 --- a/crates/nu-command/tests/commands/try_.rs +++ b/crates/nu-command/tests/commands/try_.rs @@ -54,7 +54,7 @@ fn external_failed_should_be_caught() { fn loop_try_break_should_be_successful() { let output = nu!( cwd: ".", - "loop { try { echo 'successful'; break } catch { echo 'failed'; continue } }" + "loop { try { print 'successful'; break } catch { print 'failed'; continue } }" ); assert_eq!(output.out, "successful"); @@ -66,7 +66,7 @@ fn loop_catch_break_should_show_failed() { cwd: ".", "loop { try { invalid 1; - continue; } catch { echo 'failed'; break } + continue; } catch { print 'failed'; break } } " ); diff --git a/crates/nu-command/tests/commands/while_.rs b/crates/nu-command/tests/commands/while_.rs index fc7a3643a9..4b87a4c855 100644 --- a/crates/nu-command/tests/commands/while_.rs +++ b/crates/nu-command/tests/commands/while_.rs @@ -26,7 +26,7 @@ fn while_auto_print_in_each_iteration() { fn while_break_on_external_failed() { let actual = nu!( cwd: ".", - "mut total = 0; while $total < 2 { $total = $total + 1; echo 1; nu --testbin fail }" + "mut total = 0; while $total < 2 { $total = $total + 1; print 1; nu --testbin fail }" ); // Note: nu! macro auto replace "\n" and "\r\n" with "" // so our output will be `1` diff --git a/crates/nu-command/tests/commands/with_env.rs b/crates/nu-command/tests/commands/with_env.rs index 1947627ca9..eb899a0279 100644 --- a/crates/nu-command/tests/commands/with_env.rs +++ b/crates/nu-command/tests/commands/with_env.rs @@ -71,11 +71,11 @@ fn with_env_hides_variables_in_parent_scope() { cwd: "tests/fixtures/formats", r#" let-env FOO = "1" - echo $env.FOO + print $env.FOO with-env [FOO null] { echo $env.FOO } - echo $env.FOO + print $env.FOO "# ); @@ -88,9 +88,9 @@ fn with_env_shorthand_can_not_hide_variables() { cwd: "tests/fixtures/formats", r#" let-env FOO = "1" - echo $env.FOO - FOO=null echo $env.FOO - echo $env.FOO + print $env.FOO + FOO=null print $env.FOO + print $env.FOO "# ); diff --git a/crates/nu-engine/src/eval.rs b/crates/nu-engine/src/eval.rs index ac61081aac..57a705148d 100644 --- a/crates/nu-engine/src/eval.rs +++ b/crates/nu-engine/src/eval.rs @@ -6,10 +6,9 @@ use nu_protocol::{ Operator, PathMember, PipelineElement, Redirection, }, engine::{EngineState, ProfilingConfig, Stack}, - Config, DataSource, IntoInterruptiblePipelineData, IntoPipelineData, PipelineData, - PipelineMetadata, Range, ShellError, Span, Spanned, Unit, Value, VarId, ENV_VARIABLE_ID, + DataSource, IntoInterruptiblePipelineData, IntoPipelineData, PipelineData, PipelineMetadata, + Range, ShellError, Span, Spanned, Unit, Value, VarId, ENV_VARIABLE_ID, }; -use nu_utils::stdout_write_all_and_flush; use std::collections::HashMap; use std::time::Instant; @@ -1112,24 +1111,7 @@ pub fn eval_block( } => { let exit_code = exit_code.take(); - // Drain the input to the screen via tabular output - let config = engine_state.get_config(); - - match engine_state.find_decl("table".as_bytes(), &[]) { - Some(decl_id) => { - let table = engine_state.get_decl(decl_id).run( - engine_state, - stack, - &Call::new(Span::new(0, 0)), - input, - )?; - - print_or_return(table, config)?; - } - None => { - print_or_return(input, config)?; - } - }; + input.drain()?; if let Some(exit_code) = exit_code { let mut v: Vec<_> = exit_code.collect(); @@ -1139,40 +1121,7 @@ pub fn eval_block( } } } - _ => { - // Drain the input to the screen via tabular output - let config = engine_state.get_config(); - - match engine_state.find_decl("table".as_bytes(), &[]) { - Some(decl_id) => { - let table = engine_state.get_decl(decl_id); - - if let Some(block_id) = table.get_block_id() { - let block = engine_state.get_block(block_id); - eval_block( - engine_state, - stack, - block, - input, - redirect_stdout, - redirect_stderr, - )?; - } else { - let table = table.run( - engine_state, - stack, - &Call::new(Span::new(0, 0)), - input, - )?; - - print_or_return(table, config)?; - } - } - None => { - print_or_return(input, config)?; - } - }; - } + _ => input.drain()?, } input = PipelineData::empty() @@ -1187,21 +1136,6 @@ pub fn eval_block( } } -fn print_or_return(pipeline_data: PipelineData, config: &Config) -> Result<(), ShellError> { - for item in pipeline_data { - if let Value::Error { error } = item { - return Err(*error); - } - - let mut out = item.into_string("\n", config); - out.push('\n'); - - stdout_write_all_and_flush(out)?; - } - - Ok(()) -} - pub fn eval_subexpression( engine_state: &EngineState, stack: &mut Stack, diff --git a/crates/nu-protocol/src/pipeline_data.rs b/crates/nu-protocol/src/pipeline_data.rs index 2ca731d7dd..804eff9675 100644 --- a/crates/nu-protocol/src/pipeline_data.rs +++ b/crates/nu-protocol/src/pipeline_data.rs @@ -200,6 +200,26 @@ impl PipelineData { } } + pub fn drain(self) -> Result<(), ShellError> { + match self { + PipelineData::Value(Value::Error { error }, _) => Err(*error), + PipelineData::Value(_, _) => Ok(()), + PipelineData::ListStream(stream, _) => stream.drain(), + PipelineData::ExternalStream { stdout, stderr, .. } => { + if let Some(stdout) = stdout { + stdout.drain()?; + } + + if let Some(stderr) = stderr { + stderr.drain()?; + } + + Ok(()) + } + PipelineData::Empty => Ok(()), + } + } + /// Try convert from self into iterator /// /// It returns Err if the `self` cannot be converted to an iterator. diff --git a/crates/nu-protocol/src/value/stream.rs b/crates/nu-protocol/src/value/stream.rs index f00e90b9c8..4a39cef667 100644 --- a/crates/nu-protocol/src/value/stream.rs +++ b/crates/nu-protocol/src/value/stream.rs @@ -75,6 +75,20 @@ impl RawStream { known_size: self.known_size, } } + + pub fn drain(self) -> Result<(), ShellError> { + for next in self { + match next { + Ok(val) => { + if let Value::Error { error } = val { + return Err(*error); + } + } + Err(err) => return Err(err), + } + } + Ok(()) + } } impl Debug for RawStream { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { @@ -201,6 +215,15 @@ impl ListStream { .join(separator) } + pub fn drain(self) -> Result<(), ShellError> { + for next in self { + if let Value::Error { error } = next { + return Err(*error); + } + } + Ok(()) + } + pub fn from_stream( input: impl Iterator + Send + 'static, ctrlc: Option>, diff --git a/tests/fixtures/formats/script_multiline.nu b/tests/fixtures/formats/script_multiline.nu index 389a3148a5..96d8e2a8a8 100644 --- a/tests/fixtures/formats/script_multiline.nu +++ b/tests/fixtures/formats/script_multiline.nu @@ -1,2 +1,2 @@ -echo [1 4] | length -echo [2 3 5] | length +print ([1 4] | length) +print (echo [2 3 5] | length) diff --git a/tests/shell/pipeline/commands/internal.rs b/tests/shell/pipeline/commands/internal.rs index 8ee00d662c..bfb2701682 100644 --- a/tests/shell/pipeline/commands/internal.rs +++ b/tests/shell/pipeline/commands/internal.rs @@ -48,7 +48,7 @@ fn treats_dot_dot_as_path_not_range() { r#" mkdir temp; cd temp; - echo (open ../nu_times.csv).name.0 | table; + print (open ../nu_times.csv).name.0 | table; cd ..; rmdir temp "# @@ -385,9 +385,9 @@ fn let_env_hides_variable() { cwd: ".", r#" let-env TESTENVVAR = "hello world" - echo $env.TESTENVVAR + print $env.TESTENVVAR hide-env TESTENVVAR - echo $env.TESTENVVAR + print $env.TESTENVVAR "# ); @@ -401,12 +401,12 @@ fn let_env_hides_variable_in_parent_scope() { cwd: ".", r#" let-env TESTENVVAR = "hello world" - echo $env.TESTENVVAR + print $env.TESTENVVAR do { hide-env TESTENVVAR - echo $env.TESTENVVAR + print $env.TESTENVVAR } - echo $env.TESTENVVAR + print $env.TESTENVVAR "# ); @@ -446,14 +446,14 @@ fn unlet_variable_in_parent_scope() { cwd: ".", r#" let-env DEBUG = "1" - echo $env.DEBUG + print $env.DEBUG do { let-env DEBUG = "2" - echo $env.DEBUG + print $env.DEBUG hide-env DEBUG - echo $env.DEBUG + print $env.DEBUG } - echo $env.DEBUG + print $env.DEBUG "# ); @@ -477,7 +477,7 @@ fn proper_shadow_let_env_aliases() { let actual = nu!( cwd: ".", r#" - let-env DEBUG = "true"; echo $env.DEBUG | table; do { let-env DEBUG = "false"; echo $env.DEBUG } | table; echo $env.DEBUG + let-env DEBUG = "true"; print $env.DEBUG | table; do { let-env DEBUG = "false"; print $env.DEBUG } | table; print $env.DEBUG "# ); assert_eq!(actual.out, "truefalsetrue"); @@ -526,7 +526,7 @@ fn proper_shadow_load_env_aliases() { let actual = nu!( cwd: ".", r#" - let-env DEBUG = "true"; echo $env.DEBUG | table; do { echo {DEBUG: "false"} | load-env; echo $env.DEBUG } | table; echo $env.DEBUG + let-env DEBUG = "true"; print $env.DEBUG | table; do { echo {DEBUG: "false"} | load-env; print $env.DEBUG } | table; print $env.DEBUG "# ); assert_eq!(actual.out, "truefalsetrue"); @@ -576,7 +576,7 @@ fn proper_shadow_let_aliases() { let actual = nu!( cwd: ".", r#" - let DEBUG = false; echo $DEBUG | table; do { let DEBUG = true; echo $DEBUG } | table; echo $DEBUG + let DEBUG = false; print $DEBUG | table; do { let DEBUG = true; print $DEBUG } | table; print $DEBUG "# ); assert_eq!(actual.out, "falsetruefalse");