From dfdb2b5d317d5c9d9081e069560d349e2b07d97b Mon Sep 17 00:00:00 2001 From: Gwendolyn Date: Fri, 23 Aug 2024 21:08:27 +0200 Subject: [PATCH] Improve help output for scripts (#13445) # Description Currently the parser and the documentation generation use the signature of the command, which means that it doesn't pick up on the changed name of the `main` block, and therefore shows the name of the command as "main" and doesn't find the subcommands. This PR changes the aforementioned places to use the block signature to fix these issues. This closes #13397. Incidentally it also causes input/output types to be shown in the help, which is kinda pointless for scripts since they don't operate on structured data but maybe not worth the effort to remove. # User-Facing Changes ``` # example.nu export def main [] { help main } export def 'main sub' [] { print 'sub' } ``` Before: ![image](https://github.com/user-attachments/assets/49fdcf8d-e56a-4c27-b7c8-7d2902c2a807) ![image](https://github.com/user-attachments/assets/4d1f4faa-5928-4269-b0b5-fd654563bb8b) After: ![image](https://github.com/user-attachments/assets/a7232a1f-f997-4988-808c-8fa957e39bae) ![image](https://github.com/user-attachments/assets/c5628dc6-69b5-443a-b103-9e5faa9bb4ba) # Tests Tests are still missing for the subcommands and the input/output types --------- Co-authored-by: Stefan Holderbach --- crates/nu-engine/src/documentation.rs | 4 +- crates/nu-parser/src/parser.rs | 2 +- .../src/engine/state_working_set.rs | 10 ++- tests/shell/mod.rs | 66 +++++++++++++++++++ 4 files changed, 79 insertions(+), 3 deletions(-) diff --git a/crates/nu-engine/src/documentation.rs b/crates/nu-engine/src/documentation.rs index 370b00e3f9..9d7ea7f8f2 100644 --- a/crates/nu-engine/src/documentation.rs +++ b/crates/nu-engine/src/documentation.rs @@ -25,7 +25,9 @@ pub fn get_full_help( // execution. let stack = &mut stack.start_capture(); - let signature = command.signature().update_from_command(command); + let signature = engine_state + .get_signature(command) + .update_from_command(command); get_documentation( &signature, diff --git a/crates/nu-parser/src/parser.rs b/crates/nu-parser/src/parser.rs index 33ecdadd3a..b9147ed7f9 100644 --- a/crates/nu-parser/src/parser.rs +++ b/crates/nu-parser/src/parser.rs @@ -949,7 +949,7 @@ pub fn parse_internal_call( let _ = working_set.add_span(call.head); let decl = working_set.get_decl(decl_id); - let signature = decl.signature(); + let signature = working_set.get_signature(decl); let output = signature.get_output_type(); // storing the var ID for later due to borrowing issues diff --git a/crates/nu-protocol/src/engine/state_working_set.rs b/crates/nu-protocol/src/engine/state_working_set.rs index 2ffcfa023b..eb60f9df8b 100644 --- a/crates/nu-protocol/src/engine/state_working_set.rs +++ b/crates/nu-protocol/src/engine/state_working_set.rs @@ -5,7 +5,7 @@ use crate::{ StateDelta, Variable, VirtualPath, Visibility, }, BlockId, Category, CompileError, Config, DeclId, FileId, GetSpan, Module, ModuleId, ParseError, - ParseWarning, Span, SpanId, Type, Value, VarId, VirtualPathId, + ParseWarning, Signature, Span, SpanId, Type, Value, VarId, VirtualPathId, }; use core::panic; use std::{ @@ -710,6 +710,14 @@ impl<'a> StateWorkingSet<'a> { } } + pub fn get_signature(&self, decl: &dyn Command) -> Signature { + if let Some(block_id) = decl.block_id() { + *self.get_block(block_id).signature.clone() + } else { + decl.signature() + } + } + pub fn find_commands_by_predicate( &self, predicate: impl Fn(&[u8]) -> bool, diff --git a/tests/shell/mod.rs b/tests/shell/mod.rs index 62f79a59c1..bedfe8304e 100644 --- a/tests/shell/mod.rs +++ b/tests/shell/mod.rs @@ -334,3 +334,69 @@ fn source_empty_file() { assert!(actual.out.is_empty()); }) } + +#[test] +fn main_script_help_uses_script_name1() { + // Note: this test is somewhat fragile and might need to be adapted if the usage help message changes + Playground::setup("main_filename", |dirs, sandbox| { + sandbox.mkdir("main_filename"); + sandbox.with_files(&[FileWithContent( + "script.nu", + r#"def main [] {} + "#, + )]); + let actual = nu!(cwd: dirs.test(), pipeline("nu script.nu --help")); + assert!(actual.out.contains("> script.nu")); + assert!(!actual.out.contains("> main")); + }) +} + +#[test] +fn main_script_help_uses_script_name2() { + // Note: this test is somewhat fragile and might need to be adapted if the usage help message changes + Playground::setup("main_filename", |dirs, sandbox| { + sandbox.mkdir("main_filename"); + sandbox.with_files(&[FileWithContent( + "script.nu", + r#"def main [foo: string] {} + "#, + )]); + let actual = nu!(cwd: dirs.test(), pipeline("nu script.nu")); + assert!(actual.err.contains("Usage: script.nu")); + assert!(!actual.err.contains("Usage: main")); + }) +} + +#[test] +fn main_script_subcommand_help_uses_script_name1() { + // Note: this test is somewhat fragile and might need to be adapted if the usage help message changes + Playground::setup("main_filename", |dirs, sandbox| { + sandbox.mkdir("main_filename"); + sandbox.with_files(&[FileWithContent( + "script.nu", + r#"def main [] {} + def 'main foo' [] {} + "#, + )]); + let actual = nu!(cwd: dirs.test(), pipeline("nu script.nu foo --help")); + assert!(actual.out.contains("> script.nu foo")); + assert!(!actual.out.contains("> main foo")); + }) +} + +#[test] +fn main_script_subcommand_help_uses_script_name2() { + // Note: this test is somewhat fragile and might need to be adapted if the usage help message changes + Playground::setup("main_filename", |dirs, sandbox| { + sandbox.mkdir("main_filename"); + sandbox.with_files(&[FileWithContent( + "script.nu", + r#"def main [] {} + def 'main foo' [bar: string] {} + "#, + )]); + let actual = nu!(cwd: dirs.test(), pipeline("nu script.nu foo")); + assert!(actual.err.contains("Usage: script.nu foo")); + assert!(!actual.err.contains("Usage: main foo")); + }) +}