differentiating between --x and --x: bool (nushell#10456)

# Description Fixes: nushell#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 nushell#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 nushell#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 <stevan.antoine@gmail.com>
paaul5 · Sep 23, 2023 · d2c87ad · d2c87ad
1 parent a26a01c
commit d2c87ad
Show file tree

Hide file tree

Showing 6 changed files with 51 additions and 60 deletions.
diff --git 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
@@ -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
@@ -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
@@ -67,30 +67,14 @@ 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",
     )
 }
 
 #[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
@@ -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
@@ -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<string> # 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<string> # 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<string> # 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