differentiating between `--x` and `--x: bool` by WindSoilder · Pull Request #10456 · nushell/nushell

WindSoilder · 2023-09-21T10:01:06Z

Description

This pr differentiating between --x: bool and --x

Here are examples which demostrate difference between them:

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:

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:

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

crates/nu-parser/src/parser.rs

amtoine · 2023-09-21T17:56:51Z

ooh i see what this is doing 😮
thanks for the quick response @WindSoilder 🙏

i'm not sure i am a big fan of requiring to pass true or false to -x: bool or -x = false, but i think i get the idea and i could buy that 😋

as soon as something has a type, either explicitely with x: foo or inferred with x = foo, it needs a value after, even though it's a boolean one 😮

boolean flags, a.k.a. switches, a.k.a. flags, are then special and can only be defined with --option, without any default value 🤔

yeah, i can buy that 👍

❗ however this is a breaking change, all the options / switches that were declared with : bool or = false to give them a default value, will break 😱

fdncred · 2023-09-21T18:36:57Z

Just adding this here because it helped me understand the changes.

❯ def foo [-a, --long, --both(-b), -c: bool, -d: bool = true, --eee(-e): bool = true] {
  echo $"-a = ($a)"
  echo $"--long = ($long)"
  echo $"--both(-b) = ($both)"
  echo $"-c = ($c)"
  echo $"-d = ($d)"
  echo $"--eee = ($eee)"
}

This PR now requires that -c, -d, and --eee/-e now have true or false passed in on the command line if you want to change the default. e.g. foo -d true

I think the reason they require a parameter on the flag now is because they have a predefined type. e.g. -c has a predefined type of bool, -d has a predefined type of bool with a default value, --eee/-e has a long and short form of flag with a predefined type of bool and a default value.

fdncred · 2023-09-22T13:12:37Z

Doesn't it seem like having this -c: bool default to null is going to be confusing?
Running my script above, I get this

> foo
-a = false
--long = false
--both-b = false
-c =
-d = true
--eee = true

I'm trying to figure out the rules. This is what I have so far.

-a will default to false
--long will default to false
--both(-b) will default to false
-c: bool will default to null
-d: bool = true will default to specified default value
--eee(-e): bool = true will default to specified default value

Correct me if I'm wrong, maybe the reasoning that -c: bool defaults to null is because -f: int defaults to null?

WindSoilder · 2023-09-22T15:12:33Z

maybe the reasoning that -c: bool defaults to null is because -f: int defaults to null?

Yeah, you're right, so in this way, we have two type of flag, one is switch(which works the same), one is something like keyword argument(once it have explicit type like -c: bool)

amtoine · 2023-09-22T16:38:48Z

i think -b: bool or -s: string defaulting to null is ok 😋

amtoine

@WindSoilder
i went and fixed the toolkit.nu because it was broken 👍

i was also able to fix all my scripts with this new PR and quite like the change 👌

i personally aprove this 👌

amtoine · 2023-09-22T16:59:50Z

ooh i hit a wall try to fix another one of my scripts, what does that mean @WindSoilder? 😕

> def foo [-x: bool] { if not $x { print "ok" }}; foo
Error: nu::shell::type_mismatch

  × Type mismatch.
   ╭─[entry #3:1:1]
 1 │ def foo [-x: bool] { if not $x { print "ok" }}; foo
   ·                             ─┬
   ·                              ╰── bool
   ╰────

EDIT: even adding : bool = false does not fix it... it's in a script, not in the REPL btw
it's the first hunk of this commit, i've added the default value but it still gives me this error 😕

> cargo run -- ~/documents/repos/github.com/goatfiles/scripts/nu_scripts/scripts/tmux-sessionizer.nu list-sessions
Error: nu::shell::type_mismatch

  × Type mismatch.
     ╭─[/home/amtoine/documents/repos/github.com/goatfiles/scripts/nu_scripts/scripts/tmux-sessionizer.nu:211:1]
 211 │
 212 │     if not $more {
     ·            ──┬──
     ·              ╰── bool
 213 │         return $sessions
     ╰────

related to nushell/nushell#10456

sophiajt · 2023-09-22T17:15:51Z

@WindSoilder - I think for your example:

def foo [--bar: bool] { $bar }

If you call:

foo

Then $bar should be null as it is with other flags with arguments.

amtoine · 2023-09-22T17:18:04Z

@jntrnr
it is with this PR 😋

> def foo [--bar: bool] { $bar | describe }; foo
nothing

fdncred

+1 from me to land this PR. Thanks @WindSoilder!!!

amtoine · 2023-09-22T17:39:43Z

@WindSoilder
it's up to you if you wanna look at my bug above or address it later when i can investigate more after landing 😌 😉

fdncred · 2023-09-22T19:29:08Z

@amtoine I couldn't use if not $x either and had to change it to something like this

❯ def foo [-x: bool] { if ($x != true) { print "ok" }}

You would think that since $x is a bool that we'd be able to say not $x. I wonder why we cannot?

WindSoilder · 2023-09-23T01:30:02Z

@amtoine It's because when you ran tmux-sessionizer.nu list-sessions, you ran main list-sessions function without -m flag, then list-sessions --more null is called. Currently it's allowed to pass null to nushell in flag.

So we need to handle null value in list-sessions

amtoine · 2023-09-23T08:20:28Z

ooh wow, appreciate you investigating the actual script @WindSoilder 🙏

this one is actually quite subtle 😮
the general rule of thumb is never define --switch: bool in the public API 👍

let's go ahead and land this, i'll file an issue with the unhelpful error just after i upgrade 😉

amtoine · 2023-09-23T08:36:38Z

because the changes can be a bit tricky when not used to them, i think we need to mention this in the release notes of 0.86.0 😌

related to nushell/nushell#10456

related to - #10456 # Description this PR will fix the public API of the standard library by removing the type annotations from public boolean switches. 1. the signature before ```nushell clip [--silent: bool, --no-notify: bool, --no-strip: bool, --expand (-e): bool, --codepage (-c): int] ``` 2. the signature after ```nushell clip [--silent, --no-notify, --no-strip, --expand (-e), --codepage (-c): int] ``` # User-Facing Changes ### before ```nushell > "foo" | clip Error: nu::shell::cant_convert × Can't convert to bool. ╭─[NU_STDLIB_VIRTUAL_DIR/std/mod.nu:148:1] 148 │ $in 149 │ | if $expand { table --expand } else { table } · ───┬─── · ╰── can't convert nothing to bool 150 │ | into string ╰──── ``` ### after ```nushell > "foo" | clip foo saved to clipboard ``` # Tests + Formatting # After Submitting

related to nushell/nushell#10456

related to - nushell/nushell#10456 - nushell/nushell.github.io#1071 ## description after the changes on boolean switches from nushell/nushell#10456, we need to not annotate then with `: bool` when part of a public API. this PR is required for nushell/nushell.github.io#1071 to move forward.

# Description Fixes: #11033 Sorry for the issue, it's a regression which introduce by this pr: #10456. And this pr is going to fix it. About the change: create a new field named `type_annotated` for `Arg::Flag` and `Arg::Signature` instead of `arg_explicit_type` variable. When we meet a type in `TypeMode`, we set `type_annotated` field of the argument to be true, then we know that if the arg have a annotated type easily

# 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>

related to - nushell#10456 # Description this PR will fix the public API of the standard library by removing the type annotations from public boolean switches. 1. the signature before ```nushell clip [--silent: bool, --no-notify: bool, --no-strip: bool, --expand (-e): bool, --codepage (-c): int] ``` 2. the signature after ```nushell clip [--silent, --no-notify, --no-strip, --expand (-e), --codepage (-c): int] ``` # User-Facing Changes ### before ```nushell > "foo" | clip Error: nu::shell::cant_convert × Can't convert to bool. ╭─[NU_STDLIB_VIRTUAL_DIR/std/mod.nu:148:1] 148 │ $in 149 │ | if $expand { table --expand } else { table } · ───┬─── · ╰── can't convert nothing to bool 150 │ | into string ╰──── ``` ### after ```nushell > "foo" | clip foo saved to clipboard ``` # Tests + Formatting # After Submitting

# Description Fixes: nushell#11033 Sorry for the issue, it's a regression which introduce by this pr: nushell#10456. And this pr is going to fix it. About the change: create a new field named `type_annotated` for `Arg::Flag` and `Arg::Signature` instead of `arg_explicit_type` variable. When we meet a type in `TypeMode`, we set `type_annotated` field of the argument to be true, then we know that if the arg have a annotated type easily

related to - nushell/nushell#10456 - nushell/nushell.github.io#1071 ## description after the changes on boolean switches from nushell/nushell#10456, we need to not annotate then with `: bool` when part of a public API. this PR is required for nushell/nushell.github.io#1071 to move forward.

differentiating between --x and --x: bool

a3acadf

vector-of-bool reviewed Sep 21, 2023

View reviewed changes

crates/nu-parser/src/parser.rs Outdated Show resolved Hide resolved

amtoine added status:needs-design this feature requires design notes:breaking-changes This PR implies a change affecting users and has to be noted in the release notes status:needs-core-team-attention An issue than needs the attention of other core-team members labels Sep 21, 2023

make boolean flag's default value to null

9f707f6

remove the bool annotations for switches

a440d4c

amtoine approved these changes Sep 22, 2023

View reviewed changes

amtoine added a commit to amtoine/scripts that referenced this pull request Sep 22, 2023

fix the switches / boolean options in hx.nu

b292181

related to nushell/nushell#10456

amtoine added a commit to amtoine/scripts that referenced this pull request Sep 22, 2023

fix the switches / boolean options in tmux-sessionizer.nu

de0aa08

related to nushell/nushell#10456

fdncred approved these changes Sep 22, 2023

View reviewed changes

amtoine merged commit d2c87ad into nushell:main Sep 23, 2023

amtoine added the notes:mention Include the release notes summary in the "Hall of Fame" section label Sep 23, 2023

amtoine added a commit to amtoine/nu-git-manager that referenced this pull request Sep 23, 2023

do not declare switches as : bool in public APIs

e2cf3cf

related to nushell/nushell#10456

This was referenced Sep 23, 2023

mismatched type with unset bool flags #10476

Closed

fix public boolean switches in the standard library #10477

Merged

amtoine added a commit to amtoine/scripts that referenced this pull request Sep 23, 2023

fix the boolean switches in nu-logout and nu-clippy

b7d0f7e

related to nushell/nushell#10456

amtoine mentioned this pull request Sep 24, 2023

Refactor nupm install nushell/nupm#18

Merged

amtoine added a commit to amtoine/scripts that referenced this pull request Sep 24, 2023

fix more boolean switches in public APIs

3eafd81

related to nushell/nushell#10456

amtoine added a commit to amtoine/kickstart.nvim that referenced this pull request Sep 26, 2023

do not annotate boolean switches in public APIs

f7ab9e1

related to nushell/nushell#10456

amtoine mentioned this pull request Oct 14, 2023

make_release: do not annotate boolean switches in public API nushell/nu_scripts#635

Merged

WindSoilder deleted the boolean_flag branch October 26, 2023 08:30

WindSoilder mentioned this pull request Nov 13, 2023

fix custom command's default value #11043

Merged

flying-sheep mentioned this pull request Nov 13, 2023

Make it possible to pass boolean flags dynamically #7260

Closed

Conversation

WindSoilder commented Sep 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

User-Facing Changes

Update: one more breaking change to help on #7260

Uh oh!

Uh oh!

amtoine commented Sep 21, 2023

Uh oh!

fdncred commented Sep 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fdncred commented Sep 22, 2023

Uh oh!

WindSoilder commented Sep 22, 2023

Uh oh!

amtoine commented Sep 22, 2023

Uh oh!

amtoine left a comment

Choose a reason for hiding this comment

Uh oh!

amtoine commented Sep 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sophiajt commented Sep 22, 2023

Uh oh!

amtoine commented Sep 22, 2023

Uh oh!

fdncred left a comment

Choose a reason for hiding this comment

Uh oh!

amtoine commented Sep 22, 2023

Uh oh!

fdncred commented Sep 22, 2023

Uh oh!

WindSoilder commented Sep 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

amtoine commented Sep 23, 2023

Uh oh!

amtoine commented Sep 23, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

WindSoilder commented Sep 21, 2023 •

edited

Loading

fdncred commented Sep 21, 2023 •

edited

Loading

amtoine commented Sep 22, 2023 •

edited

Loading

WindSoilder commented Sep 23, 2023 •

edited

Loading