From d69e13145040e25807aa9ee147d9de3fa53b5764 Mon Sep 17 00:00:00 2001 From: 132ikl <132@ikl.sh> Date: Tue, 19 Nov 2024 08:04:29 -0500 Subject: [PATCH] Rely on `display_output` hook for formatting values from evaluations (#14361) # Description I was reading through the documentation yesterday, when I stumbled upon [this section](https://www.nushell.sh/book/pipelines.html#behind-the-scenes) explaining how command output is formatted using the `table` command. I was surprised that this section didn't mention the `display_output` hook, so I took a look in the code and was shocked to discovered that the documentation was correct, and the `table` command _is_ automatically applied to printed pipelines. This auto-tabling has two ramifications for the `display_output` hook: 1. The `table` command is called on the output of a pipeline after the `display_output` has run, even if `display_output` contains the table command. This means each pipeline output is roughly equivalent to the following (using `ls` as an example): ```nushell ls | do $config.hooks.display_output | table ``` 2. If `display_output` returns structured data, it will _still_ be formatted through the table command. This PR removes the auto-table when the `display_output` hook is set. The auto-table made sense before `display_output` was introduced, but to me, it now seems like unnecessary "automagic" which can be accomplished using existing Nushell features. This means that you can now pull back the curtain a bit, and replace your `display_output` hook with an empty closure (`$env.config.hooks.display_output = {||}`, setting it to null retains the previous behavior) to see the values printed normally without the table formatting. I think this is a good thing, and makes it easier to understand Nushell fundamentals. It is important to note that this PR does not change how `print` and other commands (well, specifically only `watch`) print out values. They continue to use `table` with no arguments, so changing your config/`display_output` hook won't affect what `print`ing a value does. Rel: [Discord discussion](https://discord.com/channels/601130461678272522/615329862395101194/1307102690848931904) (cc @dcarosone) # User-Facing Changes Pipelines are no longer automatically formatted using the `table` command. Instead, the `display_output` hook is used to format pipeline output. Most users should see no impact, as the default `display_output` hook already uses the `table` command. # Tests + Formatting - :green_circle: `toolkit fmt` - :green_circle: `toolkit clippy` - :green_circle: `toolkit test` - :green_circle: `toolkit test stdlib` # After Submitting Will update mentioned docs page to call out `display_output` hook. --- crates/nu-cli/src/eval_cmds.rs | 4 +- crates/nu-cli/src/eval_file.rs | 4 +- crates/nu-cli/src/print.rs | 10 +++- crates/nu-cli/src/util.rs | 57 ++++++++++--------- crates/nu-command/src/filesystem/watch.rs | 2 +- .../nu-protocol/src/pipeline/pipeline_data.rs | 9 ++- 6 files changed, 50 insertions(+), 36 deletions(-) diff --git a/crates/nu-cli/src/eval_cmds.rs b/crates/nu-cli/src/eval_cmds.rs index f0bebfc884..663185a4a2 100644 --- a/crates/nu-cli/src/eval_cmds.rs +++ b/crates/nu-cli/src/eval_cmds.rs @@ -9,6 +9,8 @@ use nu_protocol::{ }; use std::sync::Arc; +use crate::util::print_pipeline; + #[derive(Default)] pub struct EvaluateCommandsOpts { pub table_mode: Option, @@ -93,7 +95,7 @@ pub fn evaluate_commands( t_mode.coerce_str()?.parse().unwrap_or_default(); } - pipeline.print(engine_state, stack, no_newline, false)?; + print_pipeline(engine_state, stack, pipeline, no_newline)?; info!("evaluate {}:{}:{}", file!(), line!(), column!()); diff --git a/crates/nu-cli/src/eval_file.rs b/crates/nu-cli/src/eval_file.rs index df75f7e4f8..7636adc8d4 100644 --- a/crates/nu-cli/src/eval_file.rs +++ b/crates/nu-cli/src/eval_file.rs @@ -1,4 +1,4 @@ -use crate::util::eval_source; +use crate::util::{eval_source, print_pipeline}; use log::{info, trace}; use nu_engine::{convert_env_values, eval_block}; use nu_parser::parse; @@ -119,7 +119,7 @@ pub fn evaluate_file( }; // Print the pipeline output of the last command of the file. - pipeline.print(engine_state, stack, true, false)?; + print_pipeline(engine_state, stack, pipeline, true)?; // Invoke the main command with arguments. // Arguments with whitespaces are quoted, thus can be safely concatenated by whitespace. diff --git a/crates/nu-cli/src/print.rs b/crates/nu-cli/src/print.rs index ffb0366242..40660ecf13 100644 --- a/crates/nu-cli/src/print.rs +++ b/crates/nu-cli/src/print.rs @@ -65,8 +65,12 @@ Since this command has no output, there is no point in piping it with other comm arg.into_pipeline_data() .print_raw(engine_state, no_newline, to_stderr)?; } else { - arg.into_pipeline_data() - .print(engine_state, stack, no_newline, to_stderr)?; + arg.into_pipeline_data().print_table( + engine_state, + stack, + no_newline, + to_stderr, + )?; } } } else if !input.is_nothing() { @@ -78,7 +82,7 @@ Since this command has no output, there is no point in piping it with other comm if raw { input.print_raw(engine_state, no_newline, to_stderr)?; } else { - input.print(engine_state, stack, no_newline, to_stderr)?; + input.print_table(engine_state, stack, no_newline, to_stderr)?; } } diff --git a/crates/nu-cli/src/util.rs b/crates/nu-cli/src/util.rs index 67bc1ad8ef..3e69657857 100644 --- a/crates/nu-cli/src/util.rs +++ b/crates/nu-cli/src/util.rs @@ -201,6 +201,35 @@ fn gather_env_vars( } } +/// Print a pipeline with formatting applied based on display_output hook. +/// +/// This function should be preferred when printing values resulting from a completed evaluation. +/// For values printed as part of a command's execution, such as values printed by the `print` command, +/// the `PipelineData::print_table` function should be preferred instead as it is not config-dependent. +/// +/// `no_newline` controls if we need to attach newline character to output. +pub fn print_pipeline( + engine_state: &mut EngineState, + stack: &mut Stack, + pipeline: PipelineData, + no_newline: bool, +) -> Result<(), ShellError> { + if let Some(hook) = engine_state.get_config().hooks.display_output.clone() { + let pipeline = eval_hook( + engine_state, + stack, + Some(pipeline), + vec![], + &hook, + "display_output", + )?; + pipeline.print_raw(engine_state, no_newline, false) + } else { + // if display_output isn't set, we should still prefer to print with some formatting + pipeline.print_table(engine_state, stack, no_newline, false) + } +} + pub fn eval_source( engine_state: &mut EngineState, stack: &mut Stack, @@ -281,36 +310,12 @@ fn evaluate_source( eval_block::(engine_state, stack, &block, input) }?; - if let PipelineData::ByteStream(..) = pipeline { - // run the display hook on bytestreams too - run_display_hook(engine_state, stack, pipeline, false) - } else { - run_display_hook(engine_state, stack, pipeline, true) - }?; + let no_newline = matches!(&pipeline, &PipelineData::ByteStream(..)); + print_pipeline(engine_state, stack, pipeline, no_newline)?; Ok(false) } -fn run_display_hook( - engine_state: &mut EngineState, - stack: &mut Stack, - pipeline: PipelineData, - no_newline: bool, -) -> Result<(), ShellError> { - if let Some(hook) = engine_state.get_config().hooks.display_output.clone() { - let pipeline = eval_hook( - engine_state, - stack, - Some(pipeline), - vec![], - &hook, - "display_output", - )?; - pipeline.print(engine_state, stack, no_newline, false) - } else { - pipeline.print(engine_state, stack, no_newline, false) - } -} #[cfg(test)] mod test { use super::*; diff --git a/crates/nu-command/src/filesystem/watch.rs b/crates/nu-command/src/filesystem/watch.rs index 6f022acca8..dd21304173 100644 --- a/crates/nu-command/src/filesystem/watch.rs +++ b/crates/nu-command/src/filesystem/watch.rs @@ -194,7 +194,7 @@ impl Command for Watch { match result { Ok(val) => { - val.print(engine_state, stack, false, false)?; + val.print_table(engine_state, stack, false, false)?; } Err(err) => { let working_set = StateWorkingSet::new(engine_state); diff --git a/crates/nu-protocol/src/pipeline/pipeline_data.rs b/crates/nu-protocol/src/pipeline/pipeline_data.rs index 827f6e31a7..75449f547f 100644 --- a/crates/nu-protocol/src/pipeline/pipeline_data.rs +++ b/crates/nu-protocol/src/pipeline/pipeline_data.rs @@ -203,7 +203,7 @@ impl PipelineData { ) -> Result { match stack.pipe_stdout().unwrap_or(&OutDest::Inherit) { OutDest::Print => { - self.print(engine_state, stack, false, false)?; + self.print_table(engine_state, stack, false, false)?; Ok(Self::Empty) } OutDest::Pipe | OutDest::PipeSeparate => Ok(self), @@ -534,11 +534,14 @@ impl PipelineData { } } - /// Consume and print self data immediately. + /// Consume and print self data immediately, formatted using table command. + /// + /// This does not respect the display_output hook. If a value is being printed out by a command, + /// this function should be used. Otherwise, `nu_cli::util::print_pipeline` should be preferred. /// /// `no_newline` controls if we need to attach newline character to output. /// `to_stderr` controls if data is output to stderr, when the value is false, the data is output to stdout. - pub fn print( + pub fn print_table( self, engine_state: &EngineState, stack: &mut Stack,