From 9e9fe83bfd13433eddeb5fd92ecdefd100a9168f Mon Sep 17 00:00:00 2001 From: Bob Hyman Date: Thu, 11 May 2023 14:59:56 -0400 Subject: [PATCH] Parameter defaults to $nu.scope.commands (#9152) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit (*third* try at posting this PR, #9104, like #9084, got polluted with unrelated commits. I'm never going to pull from the github feature branch again!) # Description Show parameter defaults in scope command signature, where they're available for display by help. per https://github.com/nushell/nushell/issues/8928. I found unexpected ramifications in one completer (NuHelpCompleter) and plugins, which both use the flag-formatting routine from builtin help. For the moment I made the minimum necessary changes to get the mainline scenario to pass tests and run. But we should circle back on what to do with plugins and help completer.. # User-Facing Changes 1. New `parameter_default` column to `signatures` table in `$nu.scope.commands` It is populated with whatever parameters can be defaulted: currently positional args and named flags. 2. Built in help (both `help ` and ` --help` will display the defaults 3. Help completer will display defaults for flags, but not for positionals. Example: A custom command with some default parameters: ``` 〉cat ~/work/dflts.nu # sample function to show defaults in help export def main [ arg1: string # mandatory positional arg2:string=abc # optional positional --switch # no default here --named:int # named flag, no default --other:string=def # flag --hard:record # default can be compound type = {foo:22, bar:"other worlds", bas:false} ] { {arg1: $arg1, arg2: $arg2, switch: $switch, named: $named, other: $other, hard: $hard, } } 〉use ~/work/dflts.nu 〉$nu.scope.commands | where name == 'dflts' | get signatures.0.any | reject short_flag description custom_completion ╭───┬────────────────┬────────────────┬──────────────────────────────────────────┬─────────────┬───────────────────────────╮ │ # │ parameter_name │ parameter_type │ syntax_shape │ is_optional │ parameter_default │ ├───┼────────────────┼────────────────┼──────────────────────────────────────────┼─────────────┼───────────────────────────┤ │ 0 │ │ input │ any │ false │ │ │ 1 │ arg1 │ positional │ string │ false │ │ │ 2 │ arg2 │ positional │ string │ true │ abc │ │ 3 │ switch │ switch │ │ true │ │ │ 4 │ named │ named │ int │ true │ │ │ 5 │ other │ named │ string │ true │ def │ │ 6 │ hard │ named │ record │ true │ ╭───────┬───────────────╮ │ │ │ │ │ │ │ │ foo │ 22 │ │ │ │ │ │ │ │ │ bar │ other worlds │ │ │ │ │ │ │ │ │ bas │ false │ │ │ │ │ │ │ │ ╰───────┴───────────────╯ │ │ 7 │ │ output │ any │ false │ │ ╰───┴────────────────┴────────────────┴──────────────────────────────────────────┴─────────────┴───────────────────────────╯ 〉help dflts sample function to show defaults in help Usage: > dflts {flags} (arg2) Flags: --switch - switch -- no default here --named - named flag, typed, but no default --other - flag with default (default: 'def') --hard - default can be compound type (default: {foo: 22, bar: 'other worlds', bas: false}) -h, --help - Display the help message for this command Parameters: arg1 : mandatory positional arg2 : optional positional (optional, default: 'abc') ``` Compared to (relevant bits of) help output previously: ``` Flags: -h, --help - Display the help message for this command -, --switch - no default here -, --named - named flag, no default -, --other - flag -, --hard > - default can be compound type Signatures: | dflts -> Parameters: arg1 : mandatory positional (optional) arg2 : optional positional ``` # Tests + Formatting # After Submitting --- crates/nu-cli/src/menus/help_completions.rs | 16 ++- .../tests/format_conversions/html.rs | 4 +- crates/nu-engine/src/documentation.rs | 102 ++++++++++++++---- crates/nu-engine/src/scope.rs | 15 +++ crates/nu-plugin/src/plugin/mod.rs | 2 +- crates/nu-protocol/src/value/mod.rs | 30 ++++++ crates/nu-std/lib/help.nu | 45 +++++--- crates/nu_plugin_query/src/query.rs | 2 +- src/tests.rs | 1 + src/tests/test_engine.rs | 17 +++ src/tests/test_help.rs | 27 +++++ 11 files changed, 221 insertions(+), 40 deletions(-) create mode 100644 src/tests/test_help.rs diff --git a/crates/nu-cli/src/menus/help_completions.rs b/crates/nu-cli/src/menus/help_completions.rs index 6b28262c1..de381e080 100644 --- a/crates/nu-cli/src/menus/help_completions.rs +++ b/crates/nu-cli/src/menus/help_completions.rs @@ -57,7 +57,9 @@ impl NuHelpCompleter { let _ = write!(long_desc, "Usage:\r\n > {}\r\n", sig.call_signature()); if !sig.named.is_empty() { - long_desc.push_str(&get_flags_section(sig)) + long_desc.push_str(&get_flags_section(sig, |v| { + v.into_string_parsable(", ", &self.0.config) + })) } if !sig.required_positional.is_empty() @@ -69,10 +71,18 @@ impl NuHelpCompleter { let _ = write!(long_desc, " {}: {}\r\n", positional.name, positional.desc); } for positional in &sig.optional_positional { + let opt_suffix = if let Some(value) = &positional.default_value { + format!( + " (optional, default: {})", + &value.into_string_parsable(", ", &self.0.config), + ) + } else { + (" (optional)").to_string() + }; let _ = write!( long_desc, - " (optional) {}: {}\r\n", - positional.name, positional.desc + " (optional) {}: {}{}\r\n", + positional.name, positional.desc, opt_suffix ); } diff --git a/crates/nu-command/tests/format_conversions/html.rs b/crates/nu-command/tests/format_conversions/html.rs index 9a1e18c6a..89182fe7a 100644 --- a/crates/nu-command/tests/format_conversions/html.rs +++ b/crates/nu-command/tests/format_conversions/html.rs @@ -72,8 +72,8 @@ fn test_no_color_flag() { ); assert_eq!( actual.out, - r"Change directory.

Usage:
> cd (path)

Flags:
-h, --help - Display the help message for this command

Signatures:
<nothing> | cd <string?> -> <nothing>
<string> | cd <string?> -> <nothing>

Parameters:
(optional) path <directory>: the path to change to

Examples:
Change to your home directory
> cd ~

Change to a directory via abbreviations
> cd d/s/9

Change to the previous working directory ($OLDPWD)
> cd -

" - ); + r"Change directory.

Usage:
> cd (path)

Flags:
-h, --help - Display the help message for this command

Signatures:
<nothing> | cd <string?> -> <nothing>
<string> | cd <string?> -> <nothing>

Parameters:
path <directory>: the path to change to (optional)

Examples:
Change to your home directory
> cd ~

Change to a directory via abbreviations
> cd d/s/9

Change to the previous working directory ($OLDPWD)
> cd -

" + ) } #[test] diff --git a/crates/nu-engine/src/documentation.rs b/crates/nu-engine/src/documentation.rs index cec962e30..fdd4ec536 100644 --- a/crates/nu-engine/src/documentation.rs +++ b/crates/nu-engine/src/documentation.rs @@ -35,6 +35,26 @@ struct DocumentationConfig { brief: bool, } +// Utility returns nu-highlighted string +fn nu_highlight_string(code_string: &str, engine_state: &EngineState, stack: &mut Stack) -> String { + if let Some(highlighter) = engine_state.find_decl(b"nu-highlight", &[]) { + let decl = engine_state.get_decl(highlighter); + + if let Ok(output) = decl.run( + engine_state, + stack, + &Call::new(Span::unknown()), + Value::string(code_string, Span::unknown()).into_pipeline_data(), + ) { + let result = output.into_value(Span::unknown()); + if let Ok(s) = result.as_string() { + return s; // successfully highlighted string + } + } + } + code_string.to_string() +} + #[allow(clippy::cognitive_complexity)] fn get_documentation( sig: &Signature, @@ -45,9 +65,11 @@ fn get_documentation( is_parser_keyword: bool, ) -> String { // Create ansi colors + //todo make these configurable -- pull from enginestate.config const G: &str = "\x1b[32m"; // green const C: &str = "\x1b[36m"; // cyan - const BB: &str = "\x1b[1;34m"; // bold blue + // was const BB: &str = "\x1b[1;34m"; // bold blue + const BB: &str = "\x1b[94m"; // light blue (nobold, should be bolding the *names*) const RESET: &str = "\x1b[0m"; // reset let cmd_name = &sig.name; @@ -98,7 +120,13 @@ fn get_documentation( } if !sig.named.is_empty() { - long_desc.push_str(&get_flags_section(sig)) + long_desc.push_str(&get_flags_section(sig, |v| { + nu_highlight_string( + &v.into_string_parsable(", ", &engine_state.config), + engine_state, + stack, + ) + })) } if !is_parser_keyword && !sig.input_output_types.is_empty() { @@ -142,18 +170,32 @@ fn get_documentation( let text = match &positional.shape { SyntaxShape::Keyword(kw, shape) => { format!( - " (optional) {C}\"{}\" + {RESET}<{BB}{}{RESET}>: {}", + " {C}\"{}\" + {RESET}<{BB}{}{RESET}>: {} (optional)", String::from_utf8_lossy(kw), document_shape(*shape.clone()), positional.desc ) } _ => { + let opt_suffix = if let Some(value) = &positional.default_value { + format!( + " (optional, default: {})", + nu_highlight_string( + &value.into_string_parsable(", ", &engine_state.config), + engine_state, + stack + ) + ) + } else { + (" (optional)").to_string() + }; + format!( - " (optional) {C}{}{RESET} <{BB}{}{RESET}>: {}", + " {C}{}{RESET} <{BB}{}{RESET}>: {}{}", positional.name, document_shape(positional.shape.clone()), - positional.desc + positional.desc, + opt_suffix, ) } }; @@ -254,21 +296,35 @@ pub fn document_shape(shape: SyntaxShape) -> SyntaxShape { } } -pub fn get_flags_section(signature: &Signature) -> String { +pub fn get_flags_section( + signature: &Signature, + mut value_formatter: F, // format default Value (because some calls cant access config or nu-highlight) +) -> String +where + F: FnMut(&nu_protocol::Value) -> String, +{ + //todo make these configurable -- pull from enginestate.config const G: &str = "\x1b[32m"; // green const C: &str = "\x1b[36m"; // cyan - const BB: &str = "\x1b[1;34m"; // bold blue + // was const BB: &str = "\x1b[1;34m"; // bold blue + const BB: &str = "\x1b[94m"; // light blue (nobold, should be bolding the *names*) const RESET: &str = "\x1b[0m"; // reset const D: &str = "\x1b[39m"; // default let mut long_desc = String::new(); let _ = write!(long_desc, "\n{G}Flags{RESET}:\n"); for flag in &signature.named { + let default_str = if let Some(value) = &flag.default_value { + format!(" (default: {BB}{}{RESET})", &value_formatter(value)) + } else { + "".to_string() + }; + let msg = if let Some(arg) = &flag.arg { if let Some(short) = flag.short { if flag.required { format!( - " {C}-{}{}{RESET} (required parameter) {:?} - {}\n", + " {C}-{}{}{RESET} (required parameter) {:?} - {}{}\n", short, if !flag.long.is_empty() { format!("{D},{RESET} {C}--{}", flag.long) @@ -276,11 +332,12 @@ pub fn get_flags_section(signature: &Signature) -> String { "".into() }, arg, - flag.desc + flag.desc, + default_str, ) } else { format!( - " {C}-{}{}{RESET} <{BB}{:?}{RESET}> - {}\n", + " {C}-{}{}{RESET} <{BB}{:?}{RESET}> - {}{}\n", short, if !flag.long.is_empty() { format!("{D},{RESET} {C}--{}", flag.long) @@ -288,48 +345,51 @@ pub fn get_flags_section(signature: &Signature) -> String { "".into() }, arg, - flag.desc + flag.desc, + default_str, ) } } else if flag.required { format!( - " {C}--{}{RESET} (required parameter) <{BB}{:?}{RESET}> - {}\n", - flag.long, arg, flag.desc + " {C}--{}{RESET} (required parameter) <{BB}{:?}{RESET}> - {}{}\n", + flag.long, arg, flag.desc, default_str, ) } else { format!( - " {C}--{}{RESET} <{BB}{:?}{RESET}> - {}\n", - flag.long, arg, flag.desc + " {C}--{}{RESET} <{BB}{:?}{RESET}> - {}{}\n", + flag.long, arg, flag.desc, default_str, ) } } else if let Some(short) = flag.short { if flag.required { format!( - " {C}-{}{}{RESET} (required parameter) - {}\n", + " {C}-{}{}{RESET} (required parameter) - {}{}\n", short, if !flag.long.is_empty() { format!("{D},{RESET} {C}--{}", flag.long) } else { "".into() }, - flag.desc + flag.desc, + default_str, ) } else { format!( - " {C}-{}{}{RESET} - {}\n", + " {C}-{}{}{RESET} - {}{}\n", short, if !flag.long.is_empty() { format!("{D},{RESET} {C}--{}", flag.long) } else { "".into() }, - flag.desc + flag.desc, + default_str ) } } else if flag.required { format!( - " {C}--{}{RESET} (required parameter) - {}\n", - flag.long, flag.desc + " {C}--{}{RESET} (required parameter) - {}{}\n", + flag.long, flag.desc, default_str, ) } else { format!(" {C}--{}{RESET} - {}\n", flag.long, flag.desc) diff --git a/crates/nu-engine/src/scope.rs b/crates/nu-engine/src/scope.rs index 747369ce3..78e98ca86 100644 --- a/crates/nu-engine/src/scope.rs +++ b/crates/nu-engine/src/scope.rs @@ -327,6 +327,7 @@ impl<'e, 's> ScopeData<'e, 's> { "short_flag".to_string(), "description".to_string(), "custom_completion".to_string(), + "parameter_default".to_string(), ]; // input @@ -340,6 +341,7 @@ impl<'e, 's> ScopeData<'e, 's> { Value::nothing(span), Value::nothing(span), Value::nothing(span), + Value::nothing(span), ], span, }); @@ -357,6 +359,7 @@ impl<'e, 's> ScopeData<'e, 's> { extract_custom_completion_from_arg(self.engine_state, &req.shape), span, ), + Value::nothing(span), ]; sig_records.push(Value::Record { @@ -379,6 +382,11 @@ impl<'e, 's> ScopeData<'e, 's> { extract_custom_completion_from_arg(self.engine_state, &opt.shape), span, ), + if let Some(val) = &opt.default_value { + val.clone() + } else { + Value::nothing(span) + }, ]; sig_records.push(Value::Record { @@ -401,6 +409,7 @@ impl<'e, 's> ScopeData<'e, 's> { extract_custom_completion_from_arg(self.engine_state, &rest.shape), span, ), + Value::nothing(span), // rest_positional does have default, but parser prohibits specifying it?! ]; sig_records.push(Value::Record { @@ -444,6 +453,11 @@ impl<'e, 's> ScopeData<'e, 's> { short_flag, Value::string(&named.desc, span), Value::string(custom_completion_command_name, span), + if let Some(val) = &named.default_value { + val.clone() + } else { + Value::nothing(span) + }, ]; sig_records.push(Value::Record { @@ -464,6 +478,7 @@ impl<'e, 's> ScopeData<'e, 's> { Value::nothing(span), Value::nothing(span), Value::nothing(span), + Value::nothing(span), ], span, }); diff --git a/crates/nu-plugin/src/plugin/mod.rs b/crates/nu-plugin/src/plugin/mod.rs index f7d206158..f470e83d6 100644 --- a/crates/nu-plugin/src/plugin/mod.rs +++ b/crates/nu-plugin/src/plugin/mod.rs @@ -320,7 +320,7 @@ fn print_help(plugin: &mut impl Plugin, encoder: impl PluginEncoder) { } }) .and_then(|_| { - let flags = get_flags_section(&signature.sig); + let flags = get_flags_section(&signature.sig, |v| format!("{:#?}", v)); write!(help, "{flags}") }) .and_then(|_| writeln!(help, "\nParameters:")) diff --git a/crates/nu-protocol/src/value/mod.rs b/crates/nu-protocol/src/value/mod.rs index 81a92430c..096577c29 100644 --- a/crates/nu-protocol/src/value/mod.rs +++ b/crates/nu-protocol/src/value/mod.rs @@ -643,6 +643,36 @@ impl Value { format!("{self:#?}") } + /// Convert Value into a parsable string (quote strings) + /// bugbug other, rarer types not handled + + pub fn into_string_parsable(&self, separator: &str, config: &Config) -> String { + match self { + // give special treatment to the simple types to make them parsable + Value::String { val, .. } => format!("'{}'", val), + + // recurse back into this function for recursive formatting + Value::List { vals: val, .. } => format!( + "[{}]", + val.iter() + .map(|x| x.into_string_parsable(", ", config)) + .collect::>() + .join(separator) + ), + Value::Record { cols, vals, .. } => format!( + "{{{}}}", + cols.iter() + .zip(vals.iter()) + .map(|(x, y)| format!("{}: {}", x, y.into_string_parsable(", ", config))) + .collect::>() + .join(separator) + ), + + // defer to standard handling for types where standard representation is parsable + _ => self.into_string(separator, config), + } + } + /// Convert Value into string. Note that Streams will be consumed. pub fn debug_string(&self, separator: &str, config: &Config) -> String { match self { diff --git a/crates/nu-std/lib/help.nu b/crates/nu-std/lib/help.nu index ba5c7aad1..5dd981daf 100644 --- a/crates/nu-std/lib/help.nu +++ b/crates/nu-std/lib/help.nu @@ -569,14 +569,27 @@ def show-command [command: record] { print "" print-help-header "Flags" - print $" (ansi teal)-h(ansi reset), (ansi teal)--help(ansi reset) - Display the help message for this command" for flag in $flags { - print -n $" (ansi teal)-($flag.short_flag)(ansi reset), (ansi teal)--($flag.parameter_name)(ansi reset)" - if not ($flag.syntax_shape | is-empty) { - print -n $" <(ansi light_blue)($flag.syntax_shape)(ansi reset)>" - } - print $" - ($flag.description)" + let flag_parts = [ " ", + (if ($flag.short_flag | is-empty) { "" } else { + $"-(ansi teal)($flag.short_flag)(ansi reset), " + }), + (if ($flag.parameter_name | is-empty) { "" } else { + $"--(ansi teal)($flag.parameter_name)(ansi reset)" + }), + (if ($flag.syntax_shape | is-empty) { "" } else { + $": <(ansi light_blue)($flag.syntax_shape)(ansi reset)>" + }), + (if ($flag.description | is-empty) { "" } else { + $" - ($flag.description)" + }), + (if ($flag.parameter_default | is-empty) { "" } else { + $" \(default: ($flag.parameter_default)\)" + }), + ] + print ($flag_parts | str join "") } + print $" (ansi teal)-h(ansi reset), --(ansi teal)help(ansi reset) - Display the help message for this command" print "" print-help-header "Signatures" @@ -595,16 +608,24 @@ def show-command [command: record] { print "" print-help-header "Parameters" for positional in $positionals { - print -n " " - if ($positional.is_optional) { - print -n "(optional) " - } - print $"(ansi teal)($positional.parameter_name)(ansi reset) <(ansi light_blue)($positional.syntax_shape)(ansi reset)>: ($positional.description)" + let arg_parts = [ " ", + $"(ansi teal)($positional.parameter_name)(ansi reset)", + (if ($positional.syntax_shape | is-empty) { "" } else { + $": <(ansi light_blue)($positional.syntax_shape)(ansi reset)>" + }), + (if ($positional.description | is-empty) { "" } else { + $" ($positional.description)" + }), + (if ($positional.parameter_default | is-empty) { "" } else { + $" \(optional, default: ($positional.parameter_default)\)" + }) + ] + print ($arg_parts | str join "") } if $is_rest { let rest = ($parameters | where parameter_type == rest | get 0) - print $" ...(ansi teal)rest(ansi reset) <(ansi light_blue)($rest.syntax_shape)(ansi reset)>: ($rest.description)" + print $" ...(ansi teal)rest(ansi reset): <(ansi light_blue)($rest.syntax_shape)(ansi reset)> ($rest.description)" } } } diff --git a/crates/nu_plugin_query/src/query.rs b/crates/nu_plugin_query/src/query.rs index 651df7873..2a374598f 100644 --- a/crates/nu_plugin_query/src/query.rs +++ b/crates/nu_plugin_query/src/query.rs @@ -71,6 +71,6 @@ pub fn get_brief_subcommand_help(sigs: &[PluginSignature]) -> String { let _ = writeln!(help, " {} - {}", x.1.sig.name, x.1.sig.usage); } - help.push_str(&get_flags_section(&sigs[0].sig)); + help.push_str(&get_flags_section(&sigs[0].sig, |v| format!("{:#?}", v))); help } diff --git a/src/tests.rs b/src/tests.rs index 9fd40444d..4e1f984e4 100644 --- a/src/tests.rs +++ b/src/tests.rs @@ -7,6 +7,7 @@ mod test_converters; mod test_custom_commands; mod test_engine; mod test_env; +mod test_help; mod test_hiding; mod test_ide; mod test_iteration; diff --git a/src/tests/test_engine.rs b/src/tests/test_engine.rs index 2c23d431f..636c86083 100644 --- a/src/tests/test_engine.rs +++ b/src/tests/test_engine.rs @@ -1,4 +1,5 @@ use crate::tests::{fail_test, run_test, TestResult}; +use rstest::rstest; #[test] fn concrete_variable_assignment() -> TestResult { @@ -63,6 +64,22 @@ fn scope_variable() -> TestResult { "int", ) } +#[rstest] +#[case("a", "<> nothing")] +#[case("b", "<1.23> float")] +#[case("flag1", "<> nothing")] +#[case("flag2", "<4.56> float")] + +fn scope_command_defaults(#[case] var: &str, #[case] exp_result: &str) -> TestResult { + run_test( + &format!( + r#"def t1 [a:int b?:float=1.23 --flag1:string --flag2:float=4.56] {{ true }}; + let rslt = ($nu.scope.commands | where name == 't1' | get signatures.0.any | where parameter_name == '{var}' | get parameter_default.0); + $"<($rslt)> ($rslt | describe)""# + ), + &format!("{exp_result}"), + ) +} #[test] fn earlier_errors() -> TestResult { diff --git a/src/tests/test_help.rs b/src/tests/test_help.rs new file mode 100644 index 000000000..d52a518ef --- /dev/null +++ b/src/tests/test_help.rs @@ -0,0 +1,27 @@ +use crate::tests::{run_test, TestResult}; +use rstest::rstest; + +#[rstest] +// avoid feeding strings containing parens to regex. Does not end well. +#[case(": arga help")] +#[case("argb help")] +#[case("optional, default: 20")] +#[case("- f1 switch")] +#[case("- f2 named no default")] +#[case("- f3 named default 3")] +#[case("default: 33")] +#[case("--help - Display the help message")] +fn can_get_help(#[case] exp_result: &str) -> TestResult { + run_test( + &format!( + r#"def t [a:string, # arga help + b:int=20, # argb help + --f1, # f1 switch help + --f2:string, # f2 named no default + --f3:int=33 # f3 named default 3 + ] {{ true }}; + help t | ansi strip | find `{exp_result}` | get 0 | str replace -a '^(.*({exp_result}).*)$' '$2'"#, + ), + exp_result, + ) +}