From 5b03bca13825f6339c7049fae9411192c41e6e30 Mon Sep 17 00:00:00 2001 From: JT <547158+jntrnr@users.noreply.github.com> Date: Sun, 26 Mar 2023 13:23:54 +1300 Subject: [PATCH] Remove autoprinting of loop block values (#8618) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit # Description This removes autoprinting the final value of a loop, much in the same spirit as not autoprinting values at the end of statements. As we fix these corner cases, it becomes more consistent that to print to the screen in a script, you use the `print` command. This gives a noticeable performance improvement as a bonus. Before: ``` C:\Source\nushell〉 for x in 1..10 { $x } 1 2 3 4 5 6 7 8 9 10 ``` Now: ``` C:\Source\nushell〉 for x in 1..10 { $x } C:\Source\nushell〉 ``` # User-Facing Changes **BREAKING CHANGE** Loops like `for`, `loop`, and `while` will no longer automatically print loop values to the screen. # 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 > **Note** > from `nushell` you can also use the `toolkit` as follows > ```bash > use toolkit.nu # or use an `env_change` hook to activate it automatically > toolkit check pr > ``` # 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-cmd-lang/src/core_commands/for_.rs | 3 +- crates/nu-cmd-lang/src/core_commands/loop_.rs | 2 +- .../nu-cmd-lang/src/core_commands/while_.rs | 3 +- crates/nu-command/tests/commands/for_.rs | 9 ++-- crates/nu-command/tests/commands/loop_.rs | 9 ++-- crates/nu-command/tests/commands/while_.rs | 9 ++-- crates/nu-protocol/src/pipeline_data.rs | 51 ++++++++++++++++--- 7 files changed, 59 insertions(+), 27 deletions(-) diff --git a/crates/nu-cmd-lang/src/core_commands/for_.rs b/crates/nu-cmd-lang/src/core_commands/for_.rs index 3e73fdd208..dad2d3f306 100644 --- a/crates/nu-cmd-lang/src/core_commands/for_.rs +++ b/crates/nu-cmd-lang/src/core_commands/for_.rs @@ -162,8 +162,7 @@ impl Command for For { return Err(err); } Ok(pipeline) => { - let exit_code = - pipeline.print_not_formatted(&engine_state, false, false)?; + let exit_code = pipeline.drain_with_exit_code()?; if exit_code != 0 { break; } diff --git a/crates/nu-cmd-lang/src/core_commands/loop_.rs b/crates/nu-cmd-lang/src/core_commands/loop_.rs index 6aa2c3fc1d..e146082a8b 100644 --- a/crates/nu-cmd-lang/src/core_commands/loop_.rs +++ b/crates/nu-cmd-lang/src/core_commands/loop_.rs @@ -67,7 +67,7 @@ impl Command for Loop { return Err(err); } Ok(pipeline) => { - let exit_code = pipeline.print(engine_state, stack, false, false)?; + let exit_code = pipeline.drain_with_exit_code()?; if exit_code != 0 { break; } diff --git a/crates/nu-cmd-lang/src/core_commands/while_.rs b/crates/nu-cmd-lang/src/core_commands/while_.rs index 34aaa40218..a99dc85ace 100644 --- a/crates/nu-cmd-lang/src/core_commands/while_.rs +++ b/crates/nu-cmd-lang/src/core_commands/while_.rs @@ -77,8 +77,7 @@ impl Command for While { return Err(err); } Ok(pipeline) => { - let exit_code = - pipeline.print_not_formatted(engine_state, false, false)?; + let exit_code = pipeline.drain_with_exit_code()?; if exit_code != 0 { break; } diff --git a/crates/nu-command/tests/commands/for_.rs b/crates/nu-command/tests/commands/for_.rs index 4952f8dd75..9d07555db0 100644 --- a/crates/nu-command/tests/commands/for_.rs +++ b/crates/nu-command/tests/commands/for_.rs @@ -1,7 +1,7 @@ use nu_test_support::nu; #[test] -fn for_auto_print_in_each_iteration() { +fn for_doesnt_auto_print_in_each_iteration() { let actual = nu!( cwd: ".", r#" @@ -9,10 +9,9 @@ fn for_auto_print_in_each_iteration() { echo 1 }"# ); - // Note: nu! macro auto replace "\n" and "\r\n" with "" - // so our output will be `11` - // that's ok, our main concern is it auto print value in each iteration. - assert_eq!(actual.out, "11"); + // Make sure we don't see any of these values in the output + // As we do not auto-print loops anymore + assert!(!actual.out.contains('1')); } #[test] diff --git a/crates/nu-command/tests/commands/loop_.rs b/crates/nu-command/tests/commands/loop_.rs index 16d6542249..f0cfc61397 100644 --- a/crates/nu-command/tests/commands/loop_.rs +++ b/crates/nu-command/tests/commands/loop_.rs @@ -1,7 +1,7 @@ use nu_test_support::nu; #[test] -fn loop_auto_print_in_each_iteration() { +fn loop_doesnt_auto_print_in_each_iteration() { let actual = nu!( cwd: ".", r#" @@ -15,10 +15,9 @@ fn loop_auto_print_in_each_iteration() { echo 1 }"# ); - // Note: nu! macro auto replace "\n" and "\r\n" with "" - // so our output will be `111` - // that's ok, our main concern is it auto print value in each iteration. - assert_eq!(actual.out, "111"); + // Make sure we don't see any of these values in the output + // As we do not auto-print loops anymore + assert!(!actual.out.contains('1')); } #[test] diff --git a/crates/nu-command/tests/commands/while_.rs b/crates/nu-command/tests/commands/while_.rs index 4b87a4c855..d1cdc37975 100644 --- a/crates/nu-command/tests/commands/while_.rs +++ b/crates/nu-command/tests/commands/while_.rs @@ -11,15 +11,14 @@ fn while_sum() { } #[test] -fn while_auto_print_in_each_iteration() { +fn while_doesnt_auto_print_in_each_iteration() { let actual = nu!( cwd: ".", "mut total = 0; while $total < 2 { $total = $total + 1; echo 1 }" ); - // Note: nu! macro auto replace "\n" and "\r\n" with "" - // so our output will be `11` - // that's ok, our main concern is it auto print value in each iteration. - assert_eq!(actual.out, "11"); + // Make sure we don't see any of these values in the output + // As we do not auto-print loops anymore + assert!(!actual.out.contains('1')); } #[test] diff --git a/crates/nu-protocol/src/pipeline_data.rs b/crates/nu-protocol/src/pipeline_data.rs index 4d961dd9a3..c1da1b1553 100644 --- a/crates/nu-protocol/src/pipeline_data.rs +++ b/crates/nu-protocol/src/pipeline_data.rs @@ -220,6 +220,39 @@ impl PipelineData { } } + pub fn drain_with_exit_code(self) -> Result { + match self { + PipelineData::Value(Value::Error { error }, _) => Err(*error), + PipelineData::Value(_, _) => Ok(0), + PipelineData::ListStream(stream, _) => { + stream.drain()?; + Ok(0) + } + PipelineData::ExternalStream { + stdout, + stderr, + exit_code, + .. + } => { + if let Some(stdout) = stdout { + stdout.drain()?; + } + + if let Some(stderr) = stderr { + stderr.drain()?; + } + + if let Some(exit_code) = exit_code { + let result = drain_exit_code(exit_code)?; + Ok(result) + } else { + Ok(0) + } + } + PipelineData::Empty => Ok(0), + } + } + /// Try convert from self into iterator /// /// It returns Err if the `self` cannot be converted to an iterator. @@ -854,18 +887,22 @@ pub fn print_if_stream( // Make sure everything has finished if let Some(exit_code) = exit_code { - let mut exit_codes: Vec<_> = exit_code.into_iter().collect(); - return match exit_codes.pop() { - #[cfg(unix)] - Some(Value::Error { error }) => Err(*error), - Some(Value::Int { val, .. }) => Ok(val), - _ => Ok(0), - }; + return drain_exit_code(exit_code); } Ok(0) } +fn drain_exit_code(exit_code: ListStream) -> Result { + let mut exit_codes: Vec<_> = exit_code.into_iter().collect(); + match exit_codes.pop() { + #[cfg(unix)] + Some(Value::Error { error }) => Err(*error), + Some(Value::Int { val, .. }) => Ok(val), + _ => Ok(0), + } +} + impl Iterator for PipelineIterator { type Item = Value;