-
Notifications
You must be signed in to change notification settings - Fork 1.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Make it possible to pass boolean flags dynamically #7260
Comments
Do you have other use cases for flags that accepts booleans or is it just to solve passing them to external commands? |
I don't think |
Maybe I was not clear but I meant that this shoud be doable in def do_it [--push] {
let flags = []
let flags = if ($push) {
$flags | append "--push"
} else {
$flags
}
run-external "echo" $flags
} Obviously not ideal I agree |
Oh my bad, while first reading the issue I skipped exactly this part of your issue (where you suggest the same solution without |
Regarding the uses case for non-external commands. I have a development script that has commands:
I need to forward the |
Related to #7033 due to quote & the whole issue in its entirety. This whole forced to String conversion problem is a huge super-topic. That it is really hard to dynamically generate command chains is one of its biggest effects. |
@Veetaha def build-imp [--push: bool = false, --release: bool = false] {
echo $"Doing a build with push: ($push) and release: ($release)"
}
def build [--push, --release] {
build-imp --push $push --release $release
} which gives >_ build
Doing a build with push: false and release: false
>_ build --push
Doing a build with push: true and release: false
>_ build --release
Doing a build with push: false and release: true
>_ build --push --release
Doing a build with push: true and release: true |
@amtoine Honestly I kind of expected something like |
the however, the rest looks sensible to me def foo [--bar] {
if $bar {
print "bar!"
} else {
print "no bar..."
}
} does give
which is expected, isn't it? 😋 |
I think the OP wants to conditionally set flags for a wrapper. Hence the need for a value flag (--b:bool). If I understand right this is to wrap externals. Afaik there is no fix for all that yet |
The suggestion from @amtoine regarding the build-imp --push --release You can only call it this way now
|
this is very true @Veetaha 👍 i use this trick inside modules, when the but yeah for interactive usage, that might not be ideal 😕 |
I ran into this too! Glad to see there's already an issue on it. This isn't a general solution, but it's what I came up with to soften the pain of this: export def "append-if" [cond: bool, target: any] -> list {
if $cond {
$in | append $target
} else {
$in
}
}
# List installed unit files
export def "sctl list unit-files" [
--all: bool # List all unit files
--user: bool # Run as non-root
] {
let args = ['-o' 'json'] | append-if $all "--all" | append-if $user "--user"
systemctl list-unit-files $args | from json
} Unfortunately, it doesn't work on Nushell's custom commands |
I think if you make a flag for a builtin, it should be possible to transparently handle it the following ways:
It's noteworthy that this could create surprising behavior if the type is not explicit -- the user could be trying to pass in a string, forget the string, and then suddenly somehow it's a bool. :P To avoid that, it might be best if this only happens if you explicitly type the argument. Side note: This proposed solution does not fix some of the other potential pains of working with this system -- for example, wrapping Nushell custom commands with other Nushell custom commands. You'd still end up having to do a lot of boilerplate, if there's a lot of flags. Whatever solution we adopt, I hope it'll be future-proof against addressing this use case as well. My suggestion for that would be some kind of splat syntax. If it's specific to Nushell custom commands, it could also include other desirable behaviors, like copying the signature information from the child function to the parent -- though I'm not sure how to implement that in a fully-general way. |
For what its worth, the syntactic sugar suggested above has precedent in PowerShell, which is enabled by a distinct "switch" parameter type: param (
# A boolean parameter, which must be given a value
[bool]
$Meow,
# A switch, which looks like a bool for most intents and purposes
[switch]
$Woof,
# A positional
[string]
$Greeting
) Some-Function -Meow $true -Woof 'hello' The way pwsh handles the above invocation depends on the types of the parameters. Since Some-Function -Woof:$some_boolean I would imagine introducing a new similar explicit This would have the unfortunate effect of complicating parsing, depending on how/when commands are parsed. i.e. One cannot fully interpret the meaning of The underlying task of "include a string in an exec() array depending on a condition" is something I stub my toe on constantly in every programming language and library where I need to invoke external processes, and really boils down to this very problem: the overwhelming majority of external programs do not provide a way to explicitly set the value of a toggle other than the presence/absence of a specific token in the argv array. Having Nu break this troublesome Unix-ish mold would be a breath of fresh air 🙂. |
I think that's overcomplicated. We already have the syntax, it just needs to be made to work.
Nushell's parser is able to recognise that the string We don't need a distinction between booleans and switches; we just need the In my opinion flags should just be very minor surface-level syntactic sugar on top of the same logic that works for named options of any type. |
Your response led me down a rabbit hole. I had a misunderstanding in that I believed the following were different: def f [--x] {}
def g [--y: bool] {} My intuition was that def h [--z: T] {} meant "declare That got me curious to try this: def m [--u: bool = "Hello"] {
print "Hello"
} which checks and executes, while giving I then went on: def foo [--xyz: any = 42] {}
foo --xyz "string" Until recently, this code would fail, as the parser would override the IIUC: Whether a flag consumes a value is determined by its The syntax shape of a flag is set when the parser encounters the type or when it encounters a default value. Despite not changing the type, a The reason However, as of the recent changes in #10424, an additional guard was added, and I hypothesize that However ×2, this still leaves In summary: # Creates an value-less switch "--xyz", with $xyz: bool
def foo [--xyz] {}
# Same. (IMO, an unintuitive special case for only `bool`)
def foo [--xyz: bool] {}
# Creates a named flag "--xyz=..." which expects a `bool`
def foo [--xyz = false] {}
# As of yesterday: Same as above (broken)
# As of #10424: Creates a named flag "--xyz" which takes any value
def foo [--xyz: any = false] {}
# As of yesterday: Creates a named flag "--xyz=..." which expects a `string` (broken)
# As of #10424: Creates a named flag "--xyz=..." which expects any value (should probably be an error)
def foo [--xyz: bool = "hey"] {} Regardless, there's still a question of whether back-compat is significant in this weird case. i.e. if |
Is there a separate "bug" with (If that had been done from the start, then Or am I misunderstanding and It seems that the original/current behaviour of boolean flags should have a syntax shape that can never accept arguments. And to resolve this enhancement issue we just need a new syntax shape that accepts an optional value (which presumably means the argument can only be provided by the |
# 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 <stevan.antoine@gmail.com>
As of nu 0.86.0, those are different in exactly the way you intuited. FWIW, I had the opposite intuition. Type annotations shouldn’t change semantics. Anyway, the changes in #10456 didn’t help with the present issue. As @amtoine said in #10456 (comment):
So yeah, you can now define options that can explicitly be passed a boolean value for your private APIs. But for public APIs and 3rd party commands, one should still do And IMHO once we have that, the changes in #10456 are nothing but an unnecessary second way to do it that has no reason for existing anymore. |
I was thinking we need a way of concisely expressing that something is true ( |
Precisely. That is exactly, what is needed. It's intuitive. |
Agreed, that's exactly what I proposed in this thread a while back. I'm actually a little disappointed we've moved in the direction of differentiating between "switches" and arguments that happen to have type The parser even already accepted |
@cumber Yeah, that's exactly how I feel too! It seems very intuitive to have absence be false by default, presence being true by default, and |
And we can't really evolve this new non-switch boolean argument in that direction, because it (uniformly with other types) accepts a space-separated value as in So once there's any usage of these new boolean arguments out there it would be backwards incompatible to start accepting bare |
Is the reason this was done purely a matter of syntactic uniformity? It seems the community is leaning in the direction of special behavior for boolean flags, anyway. (Which could be later made uniform by having flags accept either |
That would be a great solution if we lived in a vacuum (i.e. it was a great solution for React when they were free to design JSX however they liked: But as I said above: It doesn’t help in the slightest with 3rd party commands. There I still have to do stuff like let args = ([(if ($info) { ['-i'] } else []), $pkg] | flatten)
^my-cmd $args |
Already pointed as well, that this is actually a bigger, essential issue, which crosses a lot of Nushell surface.
This specific super-issue is already tracked in #7033. |
Thanks for all your comments! Let me try to summarize the path we're trying to move: switchFor the given example: def foo [--xyz] { print $xyz } It needs to support: It doesn't support: flag which takes boolean valueFor the given example: def foo2 [--xyz: bool] { print $xyz }
def foo2 [--xyz = false] { print $xyz } I'd like to make it as normal as other flags which takes a value for consistency (I have the same institution to #7260 (comment)). Which is handled by #10456 |
The default will be set to |
Yup, if we call |
Thanks @WindSoilder, that sounds exactly right.
I assume you intended this, but just to be explicit: it should also support |
# Description Closes: #7260 About the change: When we make an internalcall, and meet a `switch` (Flag.arg is None), nushell will try to see if the switch is called like `--xyz=false` , if that is true, `parse_long_flag` will return relative value. # User-Facing Changes So after the pr, the following would be possible: ```nushell def build-imp [--push, --release] { echo $"Doing a build with push: ($push) and release: ($release)" } def build [--push, --release] { build-imp --push=$push --release=$release } build --push --release=false build --push=false --release=true build --push=false --release=false build --push --release build ``` # Tests + Formatting Done # After Submitting Needs to submit a doc update, mentioned about the difference between `def a [--x] {}` and `def a [--x: bool] {}`
# 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>
# Description Closes: nushell#7260 About the change: When we make an internalcall, and meet a `switch` (Flag.arg is None), nushell will try to see if the switch is called like `--xyz=false` , if that is true, `parse_long_flag` will return relative value. # User-Facing Changes So after the pr, the following would be possible: ```nushell def build-imp [--push, --release] { echo $"Doing a build with push: ($push) and release: ($release)" } def build [--push, --release] { build-imp --push=$push --release=$release } build --push --release=false build --push=false --release=true build --push=false --release=false build --push --release build ``` # Tests + Formatting Done # After Submitting Needs to submit a doc update, mentioned about the difference between `def a [--x] {}` and `def a [--x: bool] {}`
# Description Closes: nushell#7260 About the change: When we make an internalcall, and meet a `switch` (Flag.arg is None), nushell will try to see if the switch is called like `--xyz=false` , if that is true, `parse_long_flag` will return relative value. # User-Facing Changes So after the pr, the following would be possible: ```nushell def build-imp [--push, --release] { echo $"Doing a build with push: ($push) and release: ($release)" } def build [--push, --release] { build-imp --push=$push --release=$release } build --push --release=false build --push=false --release=true build --push=false --release=false build --push --release build ``` # Tests + Formatting Done # After Submitting Needs to submit a doc update, mentioned about the difference between `def a [--x] {}` and `def a [--x: bool] {}`
Related problem
Suppose we have a command that wraps another command where we want to forward
--push
and--release
boolean flags to the inner command calledbuild-imp
.At the time of this writing, it isn't possible to do this naturally. The following doesn't work, because
nu
complains that there are excessive positional parameters$push
and$release
:Error message
This also doesn't work, although it compiles. Any invocation of
build
(even with all flags unset) results in$push
and$release
arguments insidebuild-imp
set totrue
.Demo of the variant higher
Workarounds
Combinatorial explosion
Handle all possible combinations of all flags with an
if-else
chain. This solution preserves the desired command signature but requires a huge amount of boilerplate. This solution suffers from copy-paste exponentially (pow(2, number_of_flags)
) with the total number of flags to forward. The situation is worsened if there are other arguments to pass other than the flags that will also need to be copy-pastedA variation of the solution that uses a bit nicer grouping of combinations with bitflags
Use non-boolean type for flags
The downside of this approach is that we make the API uglier, although the boilerplate doesn't grow exponentially here. We can define the underlying command with
int
arguments, but also make a wrapper that provides the same nice boolean flag API and just converts its arguments to integers.Describe the solution you'd like
The easiest solution that may probably be implemented quickly is to allow using
--flag=boolean_expression
syntax for boolean flags. The equals sign (=
) between the argument name and the boolean expression disambiguates that the argument following the boolean flag isn't a positional parameter.Right now we can pass literally any expression using
--flag=expression
, but that results in the boolean$flag
variable to be set totrue
unconditionally. Maybe this was the intended solution of this problem at all, but it doesn't work today. Maybe it's a bug then?Describe alternatives you've considered
A more general solution to the problem may also encompass solving the problem of passing optional flags to external commands.
For example, today we can't do this:
Because nutshell will complain that it can't convert
null
to string. If we were to use an empty string instead ofnull
nushell
would forward it to the external binary as a separate empty string positional parameter. So instead we have to do this today:This works because an array is spread to positional parameters when invoking an external binary from
nushell
, but it also looks like some kind of inconvenient dark magic.A more general problem is omitting named parameters to commands. I think it deserves a special syntax like this:
With the question mark operator appended
nushell
would omit the parameter if the value isfalse
ornull
for example. This is quite a naive first-comes-to-mind solution. I bet there could be some limitations with this, so feel free to share better proposals.The text was updated successfully, but these errors were encountered: