From d2c87ad4b4119aa87f98f0ac5c1c5d9cad90b73e Mon Sep 17 00:00:00 2001 From: WindSoilder Date: Sat, 23 Sep 2023 16:20:48 +0800 Subject: [PATCH] differentiating between `--x` and `--x: bool` (#10456) # Description Fixes: #10450 This pr differentiating between `--x: bool` and `--x` Here are examples which demostrate difference between them: ```nushell def a [--x: bool] { $x }; a --x # not allowed, you need to parse a value to the flag. a # it's allowed, and the value of `$x` is false, which behaves the same to `def a [--x] { $x }; a` ``` For boolean flag with default value, it works a little bit different to #10450 mentioned: ```nushell def foo [--option: bool = false] { $option } foo # output false foo --option # not allowed, you need to parse a value to the flag. foo --option true # output true ``` # User-Facing Changes After the pr, the following code is not allowed: ```nushell def a [--x: bool] { $x }; a --x ``` Instead, you have to pass a value to flag `--x` like `a --x false`. But bare flag works in the same way as before. ## Update: one more breaking change to help on #7260 ``` def foo [--option: bool] { $option == null } foo ``` After the pr, if we don't use a boolean flag, the value will be `null` instead of `true`. Because here `--option: bool` is treated as a flag rather than a switch --------- Co-authored-by: amtoine --- crates/nu-command/tests/commands/def.rs | 13 +++++++ crates/nu-parser/src/parser.rs | 48 +++++++++++-------------- crates/nu-std/std/help.nu | 2 +- src/tests/test_custom_commands.rs | 18 +--------- src/tests/test_parser.rs | 2 +- toolkit.nu | 28 +++++++-------- 6 files changed, 51 insertions(+), 60 deletions(-) diff --git a/crates/nu-command/tests/commands/def.rs b/crates/nu-command/tests/commands/def.rs index e149c159fc..869df46790 100644 --- a/crates/nu-command/tests/commands/def.rs +++ b/crates/nu-command/tests/commands/def.rs @@ -180,3 +180,16 @@ fn def_default_value_should_restrict_implicit_type() { let actual2 = nu!("def foo2 [--x = 3] { $x }; foo2 --x 3.0"); assert!(actual2.err.contains("expected int")); } + +#[test] +fn def_boolean_flags() { + let actual = nu!("def foo [--x: bool] { $x }; foo --x"); + assert!(actual.err.contains("flag missing bool argument")); + let actual = nu!("def foo [--x: bool = false] { $x }; foo"); + assert_eq!(actual.out, "false"); + let actual = nu!("def foo [--x: bool = false] { $x }; foo --x"); + assert!(actual.err.contains("flag missing bool argument")); + // boolean flags' default value should be null + let actual = nu!("def foo [--x: bool] { $x == null }; foo"); + assert_eq!(actual.out, "true"); +} diff --git a/crates/nu-parser/src/parser.rs b/crates/nu-parser/src/parser.rs index 2bf1b650dd..3dddc0a8eb 100644 --- a/crates/nu-parser/src/parser.rs +++ b/crates/nu-parser/src/parser.rs @@ -3720,11 +3720,8 @@ pub fn parse_signature_helper(working_set: &mut StateWorkingSet, span: Span) -> *shape = syntax_shape; } Arg::Flag(Flag { arg, var_id, .. }) => { - // Flags with a boolean type are just present/not-present switches - if syntax_shape != SyntaxShape::Boolean { - working_set.set_variable_type(var_id.expect("internal error: all custom parameters must have var_ids"), syntax_shape.to_type()); - *arg = Some(syntax_shape) - } + working_set.set_variable_type(var_id.expect("internal error: all custom parameters must have var_ids"), syntax_shape.to_type()); + *arg = Some(syntax_shape); } } arg_explicit_type = true; @@ -3818,31 +3815,28 @@ pub fn parse_signature_helper(working_set: &mut StateWorkingSet, span: Span) -> let var_type = &working_set.get_variable(var_id).ty; let expression_ty = expression.ty.clone(); - // Flags with a boolean type are just present/not-present switches - if var_type != &Type::Bool { - match var_type { - Type::Any => { - if !arg_explicit_type { - *arg = Some(expression_ty.to_shape()); - working_set.set_variable_type( - var_id, - expression_ty, - ); - } + // Flags with no TypeMode are just present/not-present switches + // in the case, `var_type` is any. + match var_type { + Type::Any => { + if !arg_explicit_type { + *arg = Some(expression_ty.to_shape()); + working_set + .set_variable_type(var_id, expression_ty); } - t => { - if t != &expression_ty { - working_set.error( - ParseError::AssignmentMismatch( - "Default value is the wrong type" - .into(), - format!( + } + t => { + if t != &expression_ty { + working_set.error( + ParseError::AssignmentMismatch( + "Default value is the wrong type" + .into(), + format!( "expected default value to be `{t}`" ), - expression_span, - ), - ) - } + expression_span, + ), + ) } } } diff --git a/crates/nu-std/std/help.nu b/crates/nu-std/std/help.nu index c8de4103b1..7de5e04338 100644 --- a/crates/nu-std/std/help.nu +++ b/crates/nu-std/std/help.nu @@ -100,7 +100,7 @@ def "nu-complete list-externs" [] { def build-help-header [ text: string - --no-newline (-n): bool + --no-newline (-n) ] { let header = $"(ansi green)($text)(ansi reset):" diff --git a/src/tests/test_custom_commands.rs b/src/tests/test_custom_commands.rs index 5c26c8f382..ed65ddf1f2 100644 --- a/src/tests/test_custom_commands.rs +++ b/src/tests/test_custom_commands.rs @@ -67,22 +67,6 @@ fn do_rest_args() -> TestResult { #[test] fn custom_switch1() -> TestResult { - run_test( - r#"def florb [ --dry-run: bool ] { if ($dry_run) { "foo" } else { "bar" } }; florb --dry-run"#, - "foo", - ) -} - -#[test] -fn custom_switch2() -> TestResult { - run_test( - r#"def florb [ --dry-run: bool ] { if ($dry_run) { "foo" } else { "bar" } }; florb"#, - "bar", - ) -} - -#[test] -fn custom_switch3() -> TestResult { run_test( r#"def florb [ --dry-run ] { if ($dry_run) { "foo" } else { "bar" } }; florb --dry-run"#, "foo", @@ -90,7 +74,7 @@ fn custom_switch3() -> TestResult { } #[test] -fn custom_switch4() -> TestResult { +fn custom_switch2() -> TestResult { run_test( r#"def florb [ --dry-run ] { if ($dry_run) { "foo" } else { "bar" } }; florb"#, "bar", diff --git a/src/tests/test_parser.rs b/src/tests/test_parser.rs index a384a98122..a199d45913 100644 --- a/src/tests/test_parser.rs +++ b/src/tests/test_parser.rs @@ -440,7 +440,7 @@ fn string_escape_interpolation2() -> TestResult { #[test] fn proper_rest_types() -> TestResult { run_test( - r#"def foo [--verbose(-v): bool, # my test flag + r#"def foo [--verbose(-v), # my test flag ...rest: int # my rest comment ] { if $verbose { print "verbose!" } else { print "not verbose!" } }; foo"#, "not verbose!", diff --git a/toolkit.nu b/toolkit.nu index 5c3f0c8049..fe84dd5578 100644 --- a/toolkit.nu +++ b/toolkit.nu @@ -8,8 +8,8 @@ # check standard code formatting and apply the changes export def fmt [ - --check: bool # do not apply the format changes, only check the syntax - --verbose: bool # print extra information about the command's progress + --check # do not apply the format changes, only check the syntax + --verbose # print extra information about the command's progress ] { if $verbose { print $"running ('toolkit fmt' | pretty-format-command)" @@ -32,9 +32,9 @@ export def fmt [ # # > it is important to make `clippy` happy :relieved: export def clippy [ - --verbose: bool # print extra information about the command's progress + --verbose # print extra information about the command's progress --features: list # the list of features to run *Clippy* on - --workspace: bool # run the *Clippy* command on the whole workspace (overrides `--features`) + --workspace # run the *Clippy* command on the whole workspace (overrides `--features`) ] { if $verbose { print $"running ('toolkit clippy' | pretty-format-command)" @@ -63,9 +63,9 @@ export def clippy [ # check that all the tests pass export def test [ - --fast: bool # use the "nextext" `cargo` subcommand to speed up the tests (see [`cargo-nextest`](https://nexte.st/) and [`nextest-rs/nextest`](https://github.com/nextest-rs/nextest)) + --fast # use the "nextext" `cargo` subcommand to speed up the tests (see [`cargo-nextest`](https://nexte.st/) and [`nextest-rs/nextest`](https://github.com/nextest-rs/nextest)) --features: list # the list of features to run the tests on - --workspace: bool # run the *Clippy* command on the whole workspace (overrides `--features`) + --workspace # run the *Clippy* command on the whole workspace (overrides `--features`) ] { if $fast { if $workspace { @@ -104,11 +104,11 @@ def pretty-format-command [] { # otherwise, the truth values will be incremental, following # the order above. def report [ - --fail-fmt: bool - --fail-clippy: bool - --fail-test: bool - --fail-test-stdlib: bool - --no-fail: bool + --fail-fmt + --fail-clippy + --fail-test + --fail-test-stdlib + --no-fail ] { [fmt clippy test "test stdlib"] | wrap stage @@ -227,7 +227,7 @@ def report [ # # now the whole `toolkit check pr` passes! :tada: export def "check pr" [ - --fast: bool # use the "nextext" `cargo` subcommand to speed up the tests (see [`cargo-nextest`](https://nexte.st/) and [`nextest-rs/nextest`](https://github.com/nextest-rs/nextest)) + --fast # use the "nextext" `cargo` subcommand to speed up the tests (see [`cargo-nextest`](https://nexte.st/) and [`nextest-rs/nextest`](https://github.com/nextest-rs/nextest)) --features: list # the list of features to check the current PR on ] { $env.NU_TEST_LOCALE_OVERRIDE = 'en_US.utf8'; @@ -297,7 +297,7 @@ def build-plugin [] { # build Nushell and plugins with some features export def build [ ...features: string@"nu-complete list features" # a space-separated list of feature to install with Nushell - --all: bool # build all plugins with Nushell + --all # build all plugins with Nushell ] { build-nushell ($features | str join ",") @@ -335,7 +335,7 @@ def install-plugin [] { # install Nushell and features you want export def install [ ...features: string@"nu-complete list features" # a space-separated list of feature to install with Nushell - --all: bool # install all plugins with Nushell + --all # install all plugins with Nushell ] { touch crates/nu-cmd-lang/build.rs # needed to make sure `version` has the correct `commit_hash` cargo install --path . --features ($features | str join ",") --locked --force