From 41f4d0dcbcd07401a303dfd939046bdd00c2b3ab Mon Sep 17 00:00:00 2001 From: zc he Date: Sun, 6 Apr 2025 21:37:59 +0800 Subject: [PATCH] refactor(lsp): align markdown doc string with output of --help (#15508) #15499 reminds me of the discrepancies between lsp hover docs and `--help` outputs. # Description # User-Facing Changes Before: image After: image Output of `if -h` as a reference: ``` Usage: > if (else ) Flags: -h, --help: Display the help message for this command Parameters: cond : Condition to check. then_block : Block to run if check succeeds. "else" + : Expression or block to run when the condition is false. (optional) ``` # Tests + Formatting Refined # After Submitting --- crates/nu-lsp/src/hover.rs | 83 ++++++------ crates/nu-lsp/src/notification.rs | 2 +- crates/nu-lsp/src/signature.rs | 173 ++++++++++++++++++-------- tests/fixtures/lsp/hints/signature.nu | 4 +- 4 files changed, 157 insertions(+), 105 deletions(-) diff --git a/crates/nu-lsp/src/hover.rs b/crates/nu-lsp/src/hover.rs index 4971ada75a..b2eee0f60d 100644 --- a/crates/nu-lsp/src/hover.rs +++ b/crates/nu-lsp/src/hover.rs @@ -1,7 +1,10 @@ use lsp_types::{Hover, HoverContents, HoverParams, MarkupContent, MarkupKind}; -use nu_protocol::engine::Command; +use nu_protocol::{engine::Command, PositionalArg}; -use crate::{Id, LanguageServer}; +use crate::{ + signature::{display_flag, doc_for_arg, get_signature_label}, + Id, LanguageServer, +}; impl LanguageServer { pub(crate) fn get_decl_description(decl: &dyn Command, skip_description: bool) -> String { @@ -19,35 +22,27 @@ impl LanguageServer { // Usage description.push_str("---\n### Usage \n```nu\n"); let signature = decl.signature(); - description.push_str(&Self::get_signature_label(&signature)); + description.push_str(&get_signature_label(&signature, true)); description.push_str("\n```\n"); // Flags if !signature.named.is_empty() { description.push_str("\n### Flags\n\n"); let mut first = true; - for named in &signature.named { + for named in signature.named { if first { first = false; } else { description.push('\n'); } description.push_str(" "); - if let Some(short_flag) = &named.short { - description.push_str(&format!("`-{short_flag}`")); - } - if !named.long.is_empty() { - if named.short.is_some() { - description.push_str(", "); - } - description.push_str(&format!("`--{}`", named.long)); - } - if let Some(arg) = &named.arg { - description.push_str(&format!(" `<{}>`", arg.to_type())); - } - if !named.desc.is_empty() { - description.push_str(&format!(" - {}", named.desc)); - } + description.push_str(&display_flag(&named, true)); + description.push_str(&doc_for_arg( + named.arg, + named.desc, + named.default_value, + false, + )); description.push('\n'); } description.push('\n'); @@ -60,46 +55,38 @@ impl LanguageServer { { description.push_str("\n### Parameters\n\n"); let mut first = true; - for required_arg in &signature.required_positional { + let mut write_arg = |arg: PositionalArg, optional: bool| { if first { first = false; } else { description.push('\n'); } - description.push_str(&format!( - " `{}: {}`", - required_arg.name, - required_arg.shape.to_type() + description.push_str(&format!(" `{}`", arg.name)); + description.push_str(&doc_for_arg( + Some(arg.shape), + arg.desc, + arg.default_value, + optional, )); - if !required_arg.desc.is_empty() { - description.push_str(&format!(" - {}", required_arg.desc)); - } description.push('\n'); + }; + for required_arg in signature.required_positional { + write_arg(required_arg, false); } - for optional_arg in &signature.optional_positional { - if first { - first = false; - } else { - description.push('\n'); - } - description.push_str(&format!( - " `{}: {}`", - optional_arg.name, - optional_arg.shape.to_type() - )); - if !optional_arg.desc.is_empty() { - description.push_str(&format!(" - {}", optional_arg.desc)); - } - description.push('\n'); + for optional_arg in signature.optional_positional { + write_arg(optional_arg, true); } - if let Some(arg) = &signature.rest_positional { + if let Some(arg) = signature.rest_positional { if !first { description.push('\n'); } - description.push_str(&format!(" `...{}: {}`", arg.name, arg.shape.to_type())); - if !arg.desc.is_empty() { - description.push_str(&format!(" - {}", arg.desc)); - } + description.push_str(&format!(" `...{}`", arg.name)); + description.push_str(&doc_for_arg( + Some(arg.shape), + arg.desc, + arg.default_value, + false, + )); description.push('\n'); } description.push('\n'); @@ -378,7 +365,7 @@ mod hover_tests { serde_json::json!({ "contents": { "kind": "markdown", - "value": "Concatenate multiple strings into a single string, with an optional separator between each.\n---\n### Usage \n```nu\n str join {flags} \n```\n\n### Flags\n\n `-h`, `--help` - Display the help message for this command\n\n\n### Parameters\n\n `separator: string` - Optional separator to use when creating string.\n\n\n### Input/output types\n\n```nu\n list | string\n string | string\n\n```\n### Example(s)\n Create a string from input\n```nu\n ['nu', 'shell'] | str join\n```\n Create a string from input with a separator\n```nu\n ['nu', 'shell'] | str join '-'\n```\n" + "value": "Concatenate multiple strings into a single string, with an optional separator between each.\n---\n### Usage \n```nu\n str join {flags} (separator)\n```\n\n### Flags\n\n `-h`, `--help` - Display the help message for this command\n\n\n### Parameters\n\n `separator`: `` - Optional separator to use when creating string. (optional)\n\n\n### Input/output types\n\n```nu\n list | string\n string | string\n\n```\n### Example(s)\n Create a string from input\n```nu\n ['nu', 'shell'] | str join\n```\n Create a string from input with a separator\n```nu\n ['nu', 'shell'] | str join '-'\n```\n" } }) ); diff --git a/crates/nu-lsp/src/notification.rs b/crates/nu-lsp/src/notification.rs index d3c4c8ac3e..32073747b8 100644 --- a/crates/nu-lsp/src/notification.rs +++ b/crates/nu-lsp/src/notification.rs @@ -162,7 +162,7 @@ mod tests { serde_json::json!({ "contents": { "kind": "markdown", - "value": "Create a variable and give it a value.\n\nThis command is a parser keyword. For details, check:\n https://www.nushell.sh/book/thinking_in_nu.html\n---\n### Usage \n```nu\n let {flags} \n```\n\n### Flags\n\n `-h`, `--help` - Display the help message for this command\n\n\n### Parameters\n\n `var_name: any` - Variable name.\n\n `initial_value: any` - Equals sign followed by value.\n\n\n### Input/output types\n\n```nu\n any | nothing\n\n```\n### Example(s)\n Set a variable to a value\n```nu\n let x = 10\n```\n Set a variable to the result of an expression\n```nu\n let x = 10 + 100\n```\n Set a variable based on the condition\n```nu\n let x = if false { -1 } else { 1 }\n```\n" + "value": "Create a variable and give it a value.\n\nThis command is a parser keyword. For details, check:\n https://www.nushell.sh/book/thinking_in_nu.html\n---\n### Usage \n```nu\n let {flags} = \n```\n\n### Flags\n\n `-h`, `--help` - Display the help message for this command\n\n\n### Parameters\n\n `var_name`: `` - Variable name.\n\n `initial_value`: `` - Equals sign followed by value.\n\n\n### Input/output types\n\n```nu\n any | nothing\n\n```\n### Example(s)\n Set a variable to a value\n```nu\n let x = 10\n```\n Set a variable to the result of an expression\n```nu\n let x = 10 + 100\n```\n Set a variable based on the condition\n```nu\n let x = if false { -1 } else { 1 }\n```\n" } }) ); diff --git a/crates/nu-lsp/src/signature.rs b/crates/nu-lsp/src/signature.rs index d431531d8c..6264e2736a 100644 --- a/crates/nu-lsp/src/signature.rs +++ b/crates/nu-lsp/src/signature.rs @@ -5,7 +5,7 @@ use lsp_types::{ use nu_protocol::{ ast::{Argument, Call, Expr, Expression, FindMapResult, Traverse}, engine::StateWorkingSet, - PositionalArg, Signature, + Flag, PositionalArg, Signature, SyntaxShape, Value, }; use crate::{uri_to_path, LanguageServer}; @@ -35,34 +35,85 @@ fn find_active_internal_call<'a>( } } -impl LanguageServer { - pub(crate) fn get_signature_label(signature: &Signature) -> String { - let mut label = String::new(); - label.push_str(&format!(" {}", signature.name)); - if !signature.named.is_empty() { - label.push_str(" {flags}"); - } - for required_arg in &signature.required_positional { - label.push_str(&format!(" <{}>", required_arg.name)); - } - for optional_arg in &signature.optional_positional { - let value_info = if let Some(value) = optional_arg - .default_value - .as_ref() - .and_then(|v| v.coerce_str().ok()) - { - format!("={}", value) - } else { - String::new() - }; - label.push_str(&format!(" <{}?{}>", optional_arg.name, value_info)); - } - if let Some(arg) = &signature.rest_positional { - label.push_str(&format!(" <...{}>", arg.name)); - } - label +pub(crate) fn display_flag(flag: &Flag, verbitam: bool) -> String { + let md_backtick = if verbitam { "`" } else { "" }; + let mut text = String::new(); + if let Some(short_flag) = flag.short { + text.push_str(&format!("{md_backtick}-{short_flag}{md_backtick}")); } + if !flag.long.is_empty() { + if flag.short.is_some() { + text.push_str(", "); + } + text.push_str(&format!("{md_backtick}--{}{md_backtick}", flag.long)); + } + text +} +pub(crate) fn doc_for_arg( + syntax_shape: Option, + desc: String, + default_value: Option, + optional: bool, +) -> String { + let mut text = String::new(); + if let Some(mut shape) = syntax_shape { + if let SyntaxShape::Keyword(_, inner_shape) = shape { + shape = *inner_shape; + } + text.push_str(&format!(": `<{}>`", shape)); + } + if !(desc.is_empty() && default_value.is_none()) || optional { + text.push_str(" -") + }; + if !desc.is_empty() { + text.push_str(&format!(" {}", desc)); + }; + if let Some(value) = default_value.as_ref().and_then(|v| v.coerce_str().ok()) { + text.push_str(&format!( + " ({}default: `{value}`)", + if optional { "optional, " } else { "" } + )); + } else if optional { + text.push_str(" (optional)"); + } + text +} + +pub(crate) fn get_signature_label(signature: &Signature, indent: bool) -> String { + let expand_keyword = |arg: &PositionalArg, optional: bool| match &arg.shape { + SyntaxShape::Keyword(kwd, _) => { + format!("{} <{}>", String::from_utf8_lossy(kwd), arg.name) + } + _ => { + if optional { + arg.name.clone() + } else { + format!("<{}>", arg.name) + } + } + }; + let mut label = String::new(); + if indent { + label.push_str(" "); + } + label.push_str(&signature.name); + if !signature.named.is_empty() { + label.push_str(" {flags}"); + } + for required_arg in &signature.required_positional { + label.push_str(&format!(" {}", expand_keyword(required_arg, false))); + } + for optional_arg in &signature.optional_positional { + label.push_str(&format!(" ({})", expand_keyword(optional_arg, true))); + } + if let Some(arg) = &signature.rest_positional { + label.push_str(&format!(" ...({})", arg.name)); + } + label +} + +impl LanguageServer { pub(crate) fn get_signature_help( &mut self, params: &SignatureHelpParams, @@ -120,6 +171,7 @@ impl LanguageServer { find_active_internal_call(expr, &working_set, pos_to_search) })?; let active_signature = working_set.get_decl(active_call.decl_id).signature(); + let label = get_signature_label(&active_signature, false); let mut param_num_before_pos = 0; for arg in active_call.arguments.iter() { @@ -133,39 +185,51 @@ impl LanguageServer { break; } } + let str_to_doc = |s: String| { Some(Documentation::MarkupContent(MarkupContent { kind: MarkupKind::Markdown, value: s, })) }; - let arg_to_param_info = |arg: &PositionalArg| ParameterInformation { - label: lsp_types::ParameterLabel::Simple(arg.name.to_owned()), - documentation: str_to_doc(format!( - ": `<{}>` - {}", - arg.shape.to_type(), - arg.desc.to_owned() + let arg_to_param_info = |arg: PositionalArg, optional: bool| ParameterInformation { + label: lsp_types::ParameterLabel::Simple(arg.name), + documentation: str_to_doc(doc_for_arg( + Some(arg.shape), + arg.desc, + arg.default_value, + optional, )), }; + let flag_to_param_info = |flag: Flag| ParameterInformation { + label: lsp_types::ParameterLabel::Simple(display_flag(&flag, false)), + documentation: str_to_doc(doc_for_arg(flag.arg, flag.desc, flag.default_value, false)), + }; + + // positional args let mut parameters: Vec = active_signature .required_positional - .iter() - .map(arg_to_param_info) + .into_iter() + .map(|arg| arg_to_param_info(arg, false)) .chain( active_signature .optional_positional - .iter() - .map(arg_to_param_info), + .into_iter() + .map(|arg| arg_to_param_info(arg, true)), ) .collect(); - if let Some(rest_arg) = &active_signature.rest_positional { - parameters.push(arg_to_param_info(rest_arg)); + if let Some(rest_arg) = active_signature.rest_positional { + parameters.push(arg_to_param_info(rest_arg, false)); } + let max_idx = parameters.len().saturating_sub(1) as u32; let active_parameter = Some(param_num_before_pos.min(max_idx)); + // also include flags in the end, just for documentation + parameters.extend(active_signature.named.into_iter().map(flag_to_param_info)); + Some(SignatureHelp { signatures: vec![SignatureInformation { - label: Self::get_signature_label(&active_signature), + label, documentation: str_to_doc(active_signature.description), parameters: Some(parameters), active_parameter, @@ -233,7 +297,7 @@ mod tests { actual: result_from_message(resp), expected: serde_json::json!({ "signatures": [{ - "label": " str substring {flags} <...rest>", + "label": "str substring {flags} ...(rest)", "parameters": [ ], "activeParameter": 0 }], @@ -263,7 +327,7 @@ mod tests { assert_json_include!( actual: result_from_message(resp), expected: serde_json::json!({ "signatures": [{ - "label": " str substring {flags} <...rest>", + "label": "str substring {flags} ...(rest)", "activeParameter": 1 }]}) ); @@ -272,7 +336,7 @@ mod tests { assert_json_include!( actual: result_from_message(resp), expected: serde_json::json!({ "signatures": [{ - "label": " str substring {flags} <...rest>", + "label": "str substring {flags} ...(rest)", "activeParameter": 0 }]}) ); @@ -281,7 +345,7 @@ mod tests { assert_json_include!( actual: result_from_message(resp), expected: serde_json::json!({ "signatures": [{ - "label": " echo {flags} <...rest>", + "label": "echo {flags} ...(rest)", "activeParameter": 0 }]}) ); @@ -291,8 +355,8 @@ mod tests { fn signature_help_on_custom_commands() { let config_str = r#"export def "foo bar" [ p1: int - p2: string, - p3?: int = 1 # doc + p2: string, # doc + p3?: int = 1 ] {}"#; let (client_connection, _recv) = initialize_language_server(Some(config_str), None); @@ -308,11 +372,11 @@ mod tests { actual: result_from_message(resp), expected: serde_json::json!({ "signatures": [{ - "label": " foo bar {flags} ", + "label": "foo bar {flags} (p3)", "parameters": [ - {"label": "p1", "documentation": {"value": ": `` - "}}, - {"label": "p2", "documentation": {"value": ": `` - "}}, - {"label": "p3", "documentation": {"value": ": `` - doc"}}, + {"label": "p1", "documentation": {"value": ": ``"}}, + {"label": "p2", "documentation": {"value": ": `` - doc"}}, + {"label": "p3", "documentation": {"value": ": `` - (optional, default: `1`)"}}, ], "activeParameter": 1 }], @@ -326,11 +390,12 @@ mod tests { actual: result_from_message(resp), expected: serde_json::json!({ "signatures": [{ - "label": " foo baz {flags} ", + "label": "foo baz {flags} (p3)", "parameters": [ - {"label": "p1", "documentation": {"value": ": `` - "}}, - {"label": "p2", "documentation": {"value": ": `` - "}}, - {"label": "p3", "documentation": {"value": ": `` - doc"}}, + {"label": "p1", "documentation": {"value": ": ``"}}, + {"label": "p2", "documentation": {"value": ": `` - doc"}}, + {"label": "p3", "documentation": {"value": ": `` - (optional, default: `1`)"}}, + {"label": "-h, --help", "documentation": {"value": " - Display the help message for this command"}}, ], "activeParameter": 2 }], diff --git a/tests/fixtures/lsp/hints/signature.nu b/tests/fixtures/lsp/hints/signature.nu index 65ee6732f3..da2aba7b7a 100644 --- a/tests/fixtures/lsp/hints/signature.nu +++ b/tests/fixtures/lsp/hints/signature.nu @@ -11,7 +11,7 @@ foo bar 1 2 3 foo baz 1 2 3 def "foo baz" [ p1: int - p2: string, - p3?: int = 1 # doc + p2: string, # doc + p3?: int = 1 ] {} echo