From 5d98a727ca093a18db28cb61591c63dac1a8eeea Mon Sep 17 00:00:00 2001 From: WindSoilder Date: Thu, 21 Dec 2023 17:07:08 +0800 Subject: [PATCH] Deprecate `--flag: bool` in custom command (#11365) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit # Description While #11057 is merged, it's hard to tell the difference between `--flag: bool` and `--flag`, and it makes user hard to read custom commands' signature, and hard to use them correctly. After discussion, I think we can deprecate `--flag: bool` usage, and encourage using `--flag` instead. # User-Facing Changes The following code will raise warning message, but don't stop from running. ```nushell ❯ def florb [--dry-run: bool, --another-flag] { "aaa" }; florb Error: × Deprecated: --flag: bool ╭─[entry #7:1:1] 1 │ def florb [--dry-run: bool, --another-flag] { "aaa" }; florb · ──┬─ · ╰── `--flag: bool` is deprecated. Please use `--flag` instead, more info: https://www.nushell.sh/book/custom_commands.html ╰──── aaa ``` cc @kubouch # Tests + Formatting Done # After Submitting - [ ] Add more information under https://www.nushell.sh/book/custom_commands.html to indicate `--dry-run: bool` is not allowed, - [ ] remove `: bool` from custom commands between 0.89 and 0.90 --------- Co-authored-by: Antoine Stevan <44101798+amtoine@users.noreply.github.com> --- crates/nu-cli/src/eval_cmds.rs | 4 ++++ crates/nu-cli/src/util.rs | 4 ++++ crates/nu-parser/src/parser.rs | 11 +++++++-- .../src/engine/state_working_set.rs | 8 ++++++- crates/nu-protocol/src/lib.rs | 2 ++ crates/nu-protocol/src/parse_warning.rs | 23 +++++++++++++++++++ src/tests/test_custom_commands.rs | 7 ++++++ 7 files changed, 56 insertions(+), 3 deletions(-) create mode 100644 crates/nu-protocol/src/parse_warning.rs diff --git a/crates/nu-cli/src/eval_cmds.rs b/crates/nu-cli/src/eval_cmds.rs index b5f85a9944f3..83e546fd5bac 100644 --- a/crates/nu-cli/src/eval_cmds.rs +++ b/crates/nu-cli/src/eval_cmds.rs @@ -35,6 +35,10 @@ pub fn evaluate_commands( let mut working_set = StateWorkingSet::new(engine_state); let output = parse(&mut working_set, None, commands.item.as_bytes(), false); + if let Some(warning) = working_set.parse_warnings.first() { + report_error(&working_set, warning); + } + if let Some(err) = working_set.parse_errors.first() { report_error(&working_set, err); diff --git a/crates/nu-cli/src/util.rs b/crates/nu-cli/src/util.rs index c49f1a099b54..7945bc23df55 100644 --- a/crates/nu-cli/src/util.rs +++ b/crates/nu-cli/src/util.rs @@ -220,6 +220,10 @@ pub fn eval_source( source, false, ); + if let Some(warning) = working_set.parse_warnings.first() { + report_error(&working_set, warning); + } + if let Some(err) = working_set.parse_errors.first() { set_last_exit_code(stack, 1); report_error(&working_set, err); diff --git a/crates/nu-parser/src/parser.rs b/crates/nu-parser/src/parser.rs index eb8c92231919..5dd42aeec34a 100644 --- a/crates/nu-parser/src/parser.rs +++ b/crates/nu-parser/src/parser.rs @@ -18,8 +18,8 @@ use nu_protocol::{ }, engine::StateWorkingSet, eval_const::eval_constant, - span, BlockId, DidYouMean, Flag, ParseError, PositionalArg, Signature, Span, Spanned, - SyntaxShape, Type, Unit, VarId, ENV_VARIABLE_ID, IN_VARIABLE_ID, + span, BlockId, DidYouMean, Flag, ParseError, ParseWarning, PositionalArg, Signature, Span, + Spanned, SyntaxShape, Type, Unit, VarId, ENV_VARIABLE_ID, IN_VARIABLE_ID, }; use crate::parse_keywords::{ @@ -3520,6 +3520,13 @@ pub fn parse_signature_helper(working_set: &mut StateWorkingSet, span: Span) -> type_annotated, } => { working_set.set_variable_type(var_id.expect("internal error: all custom parameters must have var_ids"), syntax_shape.to_type()); + if syntax_shape == SyntaxShape::Boolean { + working_set.warning(ParseWarning::DeprecatedWarning( + "--flag: bool".to_string(), + "--flag".to_string(), + span, + )); + } *arg = Some(syntax_shape); *type_annotated = true; } diff --git a/crates/nu-protocol/src/engine/state_working_set.rs b/crates/nu-protocol/src/engine/state_working_set.rs index 679f201b48ea..f74293096ead 100644 --- a/crates/nu-protocol/src/engine/state_working_set.rs +++ b/crates/nu-protocol/src/engine/state_working_set.rs @@ -6,7 +6,7 @@ use crate::ast::Block; use crate::{ BlockId, Config, DeclId, FileId, Module, ModuleId, Span, Type, VarId, Variable, VirtualPathId, }; -use crate::{Category, ParseError, Value}; +use crate::{Category, ParseError, ParseWarning, Value}; use core::panic; use std::collections::{HashMap, HashSet}; use std::path::PathBuf; @@ -27,6 +27,7 @@ pub struct StateWorkingSet<'a> { /// Whether or not predeclarations are searched when looking up a command (used with aliases) pub search_predecls: bool, pub parse_errors: Vec, + pub parse_warnings: Vec, } impl<'a> StateWorkingSet<'a> { @@ -39,6 +40,7 @@ impl<'a> StateWorkingSet<'a> { parsed_module_files: vec![], search_predecls: true, parse_errors: vec![], + parse_warnings: vec![], } } @@ -50,6 +52,10 @@ impl<'a> StateWorkingSet<'a> { self.parse_errors.push(parse_error) } + pub fn warning(&mut self, parse_warning: ParseWarning) { + self.parse_warnings.push(parse_warning) + } + pub fn num_files(&self) -> usize { self.delta.num_files() + self.permanent_state.num_files() } diff --git a/crates/nu-protocol/src/lib.rs b/crates/nu-protocol/src/lib.rs index f9d4879240f9..9b8083a22194 100644 --- a/crates/nu-protocol/src/lib.rs +++ b/crates/nu-protocol/src/lib.rs @@ -12,6 +12,7 @@ mod id; mod lev_distance; mod module; mod parse_error; +mod parse_warning; mod pipeline_data; #[cfg(feature = "plugin")] mod plugin_signature; @@ -35,6 +36,7 @@ pub use id::*; pub use lev_distance::levenshtein_distance; pub use module::*; pub use parse_error::{DidYouMean, ParseError}; +pub use parse_warning::ParseWarning; pub use pipeline_data::*; #[cfg(feature = "plugin")] pub use plugin_signature::*; diff --git a/crates/nu-protocol/src/parse_warning.rs b/crates/nu-protocol/src/parse_warning.rs new file mode 100644 index 000000000000..1e1f969daabc --- /dev/null +++ b/crates/nu-protocol/src/parse_warning.rs @@ -0,0 +1,23 @@ +use crate::Span; +use miette::Diagnostic; +use serde::{Deserialize, Serialize}; +use thiserror::Error; + +#[derive(Clone, Debug, Error, Diagnostic, Serialize, Deserialize)] +pub enum ParseWarning { + #[error("Deprecated: {0}")] + DeprecatedWarning( + String, + String, + #[label = "`{0}` is deprecated and will be removed in 0.90. Please use `{1}` instead, more info: https://www.nushell.sh/book/custom_commands.html"] + Span, + ), +} + +impl ParseWarning { + pub fn span(&self) -> Span { + match self { + ParseWarning::DeprecatedWarning(_, _, s) => *s, + } + } +} diff --git a/src/tests/test_custom_commands.rs b/src/tests/test_custom_commands.rs index bc6200681282..d2a562b1f6d6 100644 --- a/src/tests/test_custom_commands.rs +++ b/src/tests/test_custom_commands.rs @@ -146,6 +146,13 @@ fn custom_flag2() -> TestResult { ) } +#[test] +fn deprecated_boolean_flag() { + let actual = nu!(r#"def florb [--dry-run: bool, --another-flag] { "aaa" }; florb"#); + assert!(actual.err.contains("Deprecated")); + assert_eq!(actual.out, "aaa"); +} + #[test] fn simple_var_closing() -> TestResult { run_test("let $x = 10; def foo [] { $x }; foo", "10")