nushell

nushell copied to clipboard

Closes https://github.com/nushell/nushell/issues/6956. Relates to https://github.com/nushell/nushell/issues/6617

@webbedspace How about this?

So, wait, this renames the long version of the --ignore-errors switch? Usually compat breaks deserve at least a little discussion first.

Y'know, looking closely at the examples, it made me realise something else… Using GNU ^false instead of nu -c "exit 1", I notice that do { ^false }; echo "A" early-exits, but do { ^false } | echo "A" (that is, piping do's "result" into a command) doesn't. An interesting quirk that almost replicates what this switch does, which probably means we don't actually need to add it as written here.

On another topic, I don't like -e as the short version of the "program errors" switch. Typically, short versions of switches are initials of a verb or adjective, like rm -f for "force" or save -a for "append". -e for "error" is too easily confused with the other ignore errors switch.

Finally, you didn't add any tests to crates\nu-command\tests\commands\do_.rs ? As a rule, you gotta add tests when you add functionality.

By the way, I'm not a core developer and only first heard about Nushell two weeks ago, so my opinion isn't necessarily worth anything 🐣

So, wait, this renames the long version of the --ignore-errors switch? Usually compat breaks deserve at least a little discussion first.

I'm not attached to this change, happy to revert if it'll make the PR go through. I just did that for consistency's sake.

An interesting quirk that almost replicates what this switch does, which probably means we don't actually need to add it as written here.

Disagree. As you've noticed, pipelines will continue regardless of a program's exit code (hence the -c option). However, this is about executing blocks that don't carry stdout forward (I'm pretty sure I fucked this up actually, will check tomorrow).

I don't like -e as the short version of the "program errors" switch.

Agreed. I think using -s (shell) and -p (program) might make more sense. But I also don't really like those, so not sure.

Finally, you didn't add any tests

Yup, my bad will fix.

so my opinion isn't necessarily worth anything

Don't sell yourself short. :) I've found that new blood is able to see the weird idiocracies of a long-standing project much more easily.

However, this is about executing blocks that don't carry stdout forward (I'm pretty sure I fucked this up actually, will check tomorrow).

Ok, apparently do blocks forward their stdout through the pipeline so doing the same while ignoring errors should be fine. To the point about using | somehow, I'm still not sure that's a good idea since that is explicitly forwarding stdout along. We'd need some sink command that would swallow the output which seems overcomplicated.

Fixed the other stuff.

We'd need some sink command that would swallow the output which seems overcomplicated.

You just pipe it "into" null (as in, an expression that consists of just the constant value "null", thus ignoring the output). It couldn't be simpler:

do { command_returning_1_plus_stdout } | null | some_other_command

There's also already a command which does the same thing called ignore (which I maintain is unnecessary and less succinct than just | null).

Hmmm, well sink isn't the right word I guess. The output needs to be printed to the console instead of passing it down the pipeline. So in this example, nu -c 'echo a; exit 0'; nu -c 'echo b; exit 1' | what_goes_here | echo c, how can you get all of abc to print?

Well, as it turns out, print returns null as well, so you can just do nu -c 'echo a; exit 0'; nu -c 'echo b; exit 1' | print $in | echo c.

Neat, thanks! I think that pretty much covers the desired behavior, but it doesn't express the intent properly, so this PR is still needed. Though now I'm wondering if we should instead add another operator? Maybe ;; would mean ; but ignore errors? Or ;!?

Ok, I tried using print $in and that doesn't work because the pipeline appears to swallow stdin (and it also messes up stdout). For example, do { sudo apt update; sudo apt upgrade; sudo apt autoremove } | print $in doesn't work properly. If we were to add a language feature, I think ? instead of ; might make sense, but I also don't see the value in having this be built in: the only time you'd want to ignore the exit code is if you're using ; which means you'll have multiple statements. Hence a do block makes sense. I'm back to thinking this PR is the right approach.

@sholderbach Fixed the test

I'm missing the point here. Why do we now have two parameters -p and -s? Why can't -i suffice?

I'm missing the point here. Why do we now have two parameters -p and -s? Why can't -i suffice?

Well, consider this (using GNU false representing a command with exit code 1):

def foo [] {
  do { ^false };
  "A"
}

Because the external command returns exit code 1, the "A" is never returned.

def foo [] {
  do -i { ^false };
  "A"
}

The "A" is also never returned here, either, because -i only ignores in-shell errors (such as [].0). The proposed -p would presumably fix this.

Speaking for myself, I'd rather not struggle to remember which ignore pattern to use. I'd rather just add the -p functionality to -i and be done with it. Is there really times when we'd want one and not the other?

I notice in this pull request that there's an intent(?) to add Bash && and || style pipeline operators in 0.80. Those sound like they'd fit this use-case better (although I don't know if the existing boolean && and || should be supplanted, and I think overloading them is just flat-out not possible).

Speaking for myself, I'd rather not struggle to remember which ignore pattern to use.

I actually think this distinction is an improvement over bash: you can assert that the shell command was understood correctly vs. the program failing. Bash doesn't do this so if you have a syntax error or the command doesn't exist that's considered the same issue as running the command successfully and it failing.

We can probably support stacking the two flags so you can pass them in together and ignore all types of errors.

I notice in this pull request that there's an intent(?) to add Bash && and || style pipeline operators in 0.80.

That's exactly what this PR is addressing. && is already handled by ; and || would now be handled by ignoring program errors.

ok, let's wait to see what others think. thanks.

Ideally, I'd like us to use && and || instead of coming up with our own way of doing it.

I made sure we covered it in the draft of the changes for 0.80 https://github.com/nushell/nushell.github.io/pull/656/files as well as the experimental parser: https://github.com/jntrnr/nu_parser2 Happy to get any help or feedback on those. I think going what what most shell folks would expect would help people make the transition to nushell.

Ideally, I'd like us to use && and || instead of coming up with our own way of doing it.

How would that work with boolean expressions? Also, while I agree that it would help the transition, I don't think that's a good argument. Using do -p { a; b; c }; d is clearer to me than a && b && c || d since the structure of the program is made explicit. Having been transitioning from bash this past month-ish, the biggest problem wasn't learning new syntax, but rather missing docs (which I've been slowly adding). I think that as long as https://www.nushell.sh/book/coming_from_bash.html is filled out properly, the transition won't be too hard.

@SUPERCILEX - for some folks, taking the time to learn the differences from what they're used to isn't so bad. For other folks, who have spent a lot of time with something, it'll be a bit harder.

Unless we've got something that's significantly better than what people would expect, I don't think the friction of asking them to learn something new ends up being worth it. We tried it with a few things (looking at you save) but turns out how we did save confused a lot of people. Same with how we did complete and redirection. In the end, after talking about it for months, it just felt like we should embrace being a shell and with that embrace some (but not all) of the muscle memory of being a shell.

To your point, do -p { a; b; c }; d is also kinda confusing to me. It'd be easier for me if we added try and had something like:

try {
  a; b; c
}
d

Now I feel like I could read it relatively okay. We could add something like that... and even have a catch that lets you get to the error.

Huh, ok that's a pretty convincing argument: I have found save and complete kind of annoying to work with. I guess my question now is how long will the transition to more bash-isms take? (And is there an RFC? I'm curious. Some bash-isms like fd redirection are horrible because they're impossible to remember, so hopefully we wouldn't copy that.)

Regarding this PR, it's currently impossible to accomplish the || behavior of bash which is really annoying, so unless the bash-isms stuff is going to be ready for the next release, I still think this PR should go through accepting that it'll be deprecated at some point (but without the breaking changes, since there's no point in breaking people for something we know is dead on arrival). WDYT?

I think we could add try independently, which I think gives you that bit of functionality?

I'd be okay with us adding something different than do just for the purpose of being a bit more tolerant of errors rather than continuing to add more flags to do.

We can still add || in the future, but I think having that feature in a readable form (like you mention in an earlier comment) would be a nice nushell way of doing it.

Ok, that seems reasonable, but I'm not sure how to handle differentiating between shell and program errors. Or are you suggesting try just be a catch all? I'm not a big fan of that idea since I'd like to be able to express the desire to crash if a program doesn't exist (for example) but continue if it fails. Or would try just be for program errors and you'd need to wrap stuff in try + a do block to catch both? Right now do can catch both which is neat.

@SUPERCILEX - ahh sounds like you're describing something a bit different than || as well. If you use it, you'll still continue if the command isn't found:

> asdfasfd || echo "hello"
bash: asdfasfd: command not found
hello

With that in mind, since what you're proposing is a bit different than the precedence, this might be a good point to move this thread to an issue where you can write out the full feature proposal. That'll let other people read the whole thing and give feedback on it.

I'm a little confused. We already have -i to ignore shell errors like command not found. This PR separately adds support for ignoring programs that exit with a non-zero code. I'm saying that separation is good because those are different kinds of errors, hence adding an option to do.

Please come chat with us on discord about the larger design issue you're trying to tackle. It has some knock-on impacts in other parts of the design, so we should be more thorough over discord (or in a feature request issue).

@jntrnr Sure: https://github.com/nushell/nushell/issues/7076