Skip to content

Commit

Permalink
Deprecate --flag: bool in custom command (#11365)
Browse files Browse the repository at this point in the history
# 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>
  • Loading branch information
WindSoilder and amtoine committed Dec 21, 2023
1 parent 109f629 commit 5d98a72
Show file tree
Hide file tree
Showing 7 changed files with 56 additions and 3 deletions.
4 changes: 4 additions & 0 deletions crates/nu-cli/src/eval_cmds.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down
4 changes: 4 additions & 0 deletions crates/nu-cli/src/util.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
11 changes: 9 additions & 2 deletions crates/nu-parser/src/parser.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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::{
Expand Down Expand Up @@ -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;
}
Expand Down
8 changes: 7 additions & 1 deletion crates/nu-protocol/src/engine/state_working_set.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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<ParseError>,
pub parse_warnings: Vec<ParseWarning>,
}

impl<'a> StateWorkingSet<'a> {
Expand All @@ -39,6 +40,7 @@ impl<'a> StateWorkingSet<'a> {
parsed_module_files: vec![],
search_predecls: true,
parse_errors: vec![],
parse_warnings: vec![],
}
}

Expand All @@ -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()
}
Expand Down
2 changes: 2 additions & 0 deletions crates/nu-protocol/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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::*;
Expand Down
23 changes: 23 additions & 0 deletions crates/nu-protocol/src/parse_warning.rs
Original file line number Diff line number Diff line change
@@ -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,
}
}
}
7 changes: 7 additions & 0 deletions src/tests/test_custom_commands.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Expand Down

0 comments on commit 5d98a72

Please sign in to comment.