From b7a3e5989d2ff1d2c7c30656e1613605b391df6b Mon Sep 17 00:00:00 2001 From: Reilly Wood <26268125+rgwood@users.noreply.github.com> Date: Mon, 12 Dec 2022 15:23:04 -0800 Subject: [PATCH] Make hook execution stream instead of collecting (#7440) Closes #7431. In a nutshell: - `run_hook_block()` in repl.rs was collecting all input into a `Value` instead of handling streaming input properly - this was a problem because now we have a default `display_output` hook that _everything_ gets piped to - this PR fixes the problem by tweaking `run_hook_block()` to return a `PipelineData` instead of a `Value` After this change, individual pages are rendered as they finish. This is a little easier to see if I tweak `STREAM_PAGE_SIZE` in table.rs to 10: ![image](https://user-images.githubusercontent.com/26268125/206935370-412b2ee9-9401-4222-bc93-5bd5a9adc13b.png) ## Future work This does _not_ fix https://github.com/nushell/nushell/issues/7342. --- crates/nu-cli/src/repl.rs | 104 ++++++++++++++++++++------------------ 1 file changed, 54 insertions(+), 50 deletions(-) diff --git a/crates/nu-cli/src/repl.rs b/crates/nu-cli/src/repl.rs index ea9a6cdb4..2fb12d162 100644 --- a/crates/nu-cli/src/repl.rs +++ b/crates/nu-cli/src/repl.rs @@ -709,6 +709,13 @@ pub fn eval_hook( ) -> Result { let value_span = value.span()?; + // Hooks can optionally be a record in this form: + // { + // condition: {|before, after| ... } # block that evaluates to true/false + // code: # block or a string + // } + // The condition block will be run to check whether the main hook (in `code`) should be run. + // If it returns true (the default if a condition block is not specified), the hook should be run. let condition_path = PathMember::String { val: "condition".to_string(), span: value_span, @@ -748,16 +755,19 @@ pub fn eval_hook( arguments.clone(), block_span, ) { - Ok(value) => match value { - Value::Bool { val, .. } => val, - other => { + Ok(pipeline_data) => { + if let PipelineData::Value(Value::Bool { val, .. }, ..) = + pipeline_data + { + val + } else { return Err(ShellError::UnsupportedConfigValue( "boolean output".to_string(), - format!("{}", other.get_type()), - other.span()?, + "other PipelineData variant".to_string(), + block_span, )); } - }, + } Err(err) => { return Err(err); } @@ -880,34 +890,28 @@ pub fn eval_hook( span: block_span, .. } => { - output = PipelineData::Value( - run_hook_block( - engine_state, - stack, - *block_id, - input, - arguments, - *block_span, - )?, - None, - ); + output = run_hook_block( + engine_state, + stack, + *block_id, + input, + arguments, + *block_span, + )?; } Value::Closure { val: block_id, span: block_span, .. } => { - output = PipelineData::Value( - run_hook_block( - engine_state, - stack, - *block_id, - input, - arguments, - *block_span, - )?, - None, - ); + output = run_hook_block( + engine_state, + stack, + *block_id, + input, + arguments, + *block_span, + )?; } other => { return Err(ShellError::UnsupportedConfigValue( @@ -931,7 +935,7 @@ pub fn run_hook_block( optional_input: Option, arguments: Vec<(String, Value)>, span: Span, -) -> Result { +) -> Result { let block = engine_state.get_block(block_id); let input = optional_input.unwrap_or_else(PipelineData::empty); @@ -955,28 +959,28 @@ pub fn run_hook_block( match eval_block_with_early_return(engine_state, &mut callee_stack, block, input, false, false) { - Ok(pipeline_data) => match pipeline_data.into_value(span) { - Value::Error { error } => Err(error), - val => { - // If all went fine, preserve the environment of the called block - let caller_env_vars = stack.get_env_var_names(engine_state); - - // remove env vars that are present in the caller but not in the callee - // (the callee hid them) - for var in caller_env_vars.iter() { - if !callee_stack.has_env_var(engine_state, var) { - stack.remove_env_var(engine_state, var); - } - } - - // add new env vars from callee to caller - for (var, value) in callee_stack.get_stack_env_vars() { - stack.add_env_var(var, value); - } - - Ok(val) + Ok(pipeline_data) => { + if let PipelineData::Value(Value::Error { error }, _) = pipeline_data { + return Err(error); } - }, + + // If all went fine, preserve the environment of the called block + let caller_env_vars = stack.get_env_var_names(engine_state); + + // remove env vars that are present in the caller but not in the callee + // (the callee hid them) + for var in caller_env_vars.iter() { + if !callee_stack.has_env_var(engine_state, var) { + stack.remove_env_var(engine_state, var); + } + } + + // add new env vars from callee to caller + for (var, value) in callee_stack.get_stack_env_vars() { + stack.add_env_var(var, value); + } + Ok(pipeline_data) + } Err(err) => Err(err), } }