Throw out error if external command in subexpression is failed to run by WindSoilder · Pull Request #8204 · nushell/nushell

WindSoilder · 2023-02-25T05:07:08Z

Description

User-Facing Changes

The following command:

❯ let x = (^cat asdf; "foo")
Error: nu::shell::external_command

  × External command failed
   ╭─[entry #1:1:1]
 1 │ let x = (^cat asdf; "foo")
   ·           ─┬─
   ·            ╰── External command failed
   ╰────
  help: cat: asdf: No such file or directory

As we can see, it will throws out error rather than return "foo".

I think it's fine, because the semantic of subexpression is eval a subexpression to a value, if the external command is failed to run, it means that we can't convert the result to value.

It's different to ^cat asdf, which runs the command directlry.

Tests + Formatting

Don't forget to add tests that cover your changes.

Make sure you've run and fixed any issues with these commands:

cargo fmt --all -- --check to check standard code formatting (cargo fmt --all applies these changes)
cargo clippy --workspace -- -D warnings -D clippy::unwrap_used -A clippy::needless_collect to check that you're using the standard code style
cargo test --workspace to check that all tests pass

After Submitting

If your PR had any user-facing changes, update the documentation after the PR is merged, if necessary. This will help us keep the docs up to date.

rgwood · 2023-02-25T05:53:58Z

Nice PR, thanks! This is an improvement, but the result of the subexpression is surprising to me:

〉let f = (^false; "foo")
〉$f
〉$f | describe
string
〉(^false;"hello,world!") | describe
string

I'm not 100% sure what the desired behaviour is here, but returning an empty string seems wrong.

WindSoilder · 2023-02-25T09:13:18Z

Hmm...Or the variable shouldn't defined?

And I'm not sure how it breaks test code, need to take a deeper look

WindSoilder · 2023-02-26T12:53:49Z

Hi, refer to let assignment, sorry I don't have a perfect way....

Currently I have two options:

Throws out error if external command runs into failed and then in the case let f = (^false; "foo"), variable f won't be defined. One shortcoming is that for pure expression (^cat asdf; "foo"), it still throws out error, which would make user confused, and the error message is not good.

❯ (^cat asdf; "foo")
cat: asdf: No such file or directory
Error: nu::shell::external_command

  × External command failed
   ╭─[entry #1:1:1]
 1 │ (^cat asdf; "foo")
   ·   ─┬─
   ·    ╰── External command failed
   ╰────
  help:

Keep f to be empty string. And something like (^cat adsfa; "foo") works better

codecov · 2023-02-26T13:12:37Z

Codecov Report

Merging #8204 (19c3c35) into main (592e677) will increase coverage by 0.37%.
The diff coverage is 97.15%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #8204      +/-   ##
==========================================
+ Coverage   67.72%   68.09%   +0.37%     
==========================================
  Files         621      620       -1     
  Lines       99312    99313       +1     
==========================================
+ Hits        67258    67629     +371     
+ Misses      32054    31684     -370

Impacted Files	Coverage Δ
crates/nu-command/src/default_context.rs	`99.78% <ø> (-0.01%)`	⬇️
crates/nu-protocol/src/pipeline_data.rs	`77.45% <89.79%> (+0.46%)`	⬆️
crates/nu-engine/src/eval.rs	`75.59% <93.02%> (+0.59%)`	⬆️
crates/nu-cli/src/print.rs	`100.00% <100.00%> (ø)`
crates/nu-cmd-lang/src/core_commands/alias.rs	`100.00% <100.00%> (ø)`
crates/nu-cmd-lang/src/core_commands/break_.rs	`100.00% <100.00%> (ø)`
...rates/nu-cmd-lang/src/core_commands/commandline.rs	`54.68% <100.00%> (ø)`
crates/nu-cmd-lang/src/core_commands/continue_.rs	`100.00% <100.00%> (ø)`
crates/nu-cmd-lang/src/core_commands/def.rs	`100.00% <100.00%> (ø)`
crates/nu-cmd-lang/src/core_commands/def_env.rs	`100.00% <100.00%> (ø)`
... and 176 more

rgwood · 2023-02-26T17:42:38Z

I prefer 1. I think (^cat asdf; "foo") should fail if the call to cat fails.

WindSoilder · 2023-02-27T01:35:12Z

Yeah, I like 1 too, and I've update title and descripiton to reflect latest changes

rgwood · 2023-02-27T01:47:14Z

Nice. The new behaviour seems good to me 👍

Looks like the Windows virtualenv tests are failing with a stack overflow again 😭. Last time, @kubouch fixed this by refactoring code into a new function: #8198 Not sure if that's a viable solution here.

…a function

kubouch · 2023-03-01T11:24:24Z

Great the tests are working now!

One question: What value will be stored in x?

WindSoilder · 2023-03-01T11:30:01Z

In this case x will remain undefined

kubouch · 2023-03-01T11:50:30Z

Yeah, this is interesting, because parser still has the x definition (check $nu.scope.vars), but it's not in the Stack during evaluation. I think it's OK, but maybe good to keep in mind in case we see some weird behavior.

…d to run (nushell#8204)" This reverts commit dec0a25.

…d to run (#8204)" (#8475) This reverts commit dec0a25. It breaks programs like `fzf` # Description Fixes: #8472 Fixes: #8313 Reopen: #7690 # User-Facing Changes _(List of all changes that impact the user experience here. This helps us keep track of breaking changes.)_ # Tests + Formatting Don't forget to add tests that cover your changes. Make sure you've run and fixed any issues with these commands: - `cargo fmt --all -- --check` to check standard code formatting (`cargo fmt --all` applies these changes) - `cargo clippy --workspace -- -D warnings -D clippy::unwrap_used -A clippy::needless_collect` to check that you're using the standard code style - `cargo test --workspace` to check that all tests pass > **Note** > from `nushell` you can also use the `toolkit` as follows > ```bash > use toolkit.nu # or use an `env_change` hook to activate it automatically > toolkit check pr > ``` # After Submitting If your PR had any user-facing changes, update [the documentation](https://github.com/nushell/nushell.github.io) after the PR is merged, if necessary. This will help us keep the docs up to date.

WindSoilder added 2 commits February 23, 2023 16:14

make semicolon works better in subexpression

4d39be5

Merge branch 'main' into subexpression_with_external_failed

9f86c2e

WindSoilder marked this pull request as draft February 25, 2023 09:13

WindSoilder added 2 commits February 26, 2023 00:40

fix unit tests

2616bf3

try fix?

e33be4d

WindSoilder marked this pull request as ready for review February 26, 2023 12:54

WindSoilder marked this pull request as draft February 26, 2023 13:42

WindSoilder marked this pull request as ready for review February 26, 2023 13:43

make user friendly

6e0b448

WindSoilder changed the title ~~Make semicolon works better in subcommand~~ Throw out error if external command in subexpression is failed in run Feb 27, 2023

WindSoilder added 2 commits February 27, 2023 09:54

Merge branch 'main' into subexpression_with_external_failed

430062d

try avoid stack overflow by abstracting pipeline checking logic into …

494c1a7

…a function

WindSoilder changed the title ~~Throw out error if external command in subexpression is failed in run~~ Throw out error if external command in subexpression is failed to run Feb 27, 2023

WindSoilder added 3 commits February 27, 2023 17:46

Merge branch 'main' into subexpression_with_external_failed

c4957bd

Merge branch 'main' into subexpression_with_external_failed

a4ae067

only want to check subexp substitution if input is externalstream

19c3c35

kubouch merged commit dec0a25 into nushell:main Mar 1, 2023

WindSoilder mentioned this pull request Mar 4, 2023

make the "External command failed" error more descriptive #8313

Closed

sophiajt mentioned this pull request Mar 16, 2023

return statement weird behaviour in version 0.77 #8472

Closed

WindSoilder added a commit to WindSoilder/nushell that referenced this pull request Mar 16, 2023

Revert "Throw out error if external command in subexpression is faile…

821c883

…d to run (nushell#8204)" This reverts commit dec0a25.

sholderbach mentioned this pull request Mar 16, 2023

0.77.0 broke my def-env command #8467

Closed

WindSoilder deleted the subexpression_with_external_failed branch August 2, 2023 22:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Throw out error if external command in subexpression is failed to run#8204

Throw out error if external command in subexpression is failed to run#8204
kubouch merged 10 commits intonushell:mainfrom
WindSoilder:subexpression_with_external_failed

WindSoilder commented Feb 25, 2023 •

edited

Loading

Uh oh!

rgwood commented Feb 25, 2023

Uh oh!

WindSoilder commented Feb 25, 2023

Uh oh!

WindSoilder commented Feb 26, 2023

Uh oh!

codecov bot commented Feb 26, 2023 •

edited

Loading

Uh oh!

rgwood commented Feb 26, 2023

Uh oh!

WindSoilder commented Feb 27, 2023

Uh oh!

rgwood commented Feb 27, 2023

Uh oh!

kubouch commented Mar 1, 2023

Uh oh!

WindSoilder commented Mar 1, 2023 •

edited

Loading

Uh oh!

kubouch commented Mar 1, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

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

Uh oh!

Description

User-Facing Changes

Tests + Formatting

After Submitting

Uh oh!

rgwood commented Feb 25, 2023

Uh oh!

WindSoilder commented Feb 25, 2023

Uh oh!

WindSoilder commented Feb 26, 2023

Uh oh!

codecov bot commented Feb 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

rgwood commented Feb 26, 2023

Uh oh!

WindSoilder commented Feb 27, 2023

Uh oh!

rgwood commented Feb 27, 2023

Uh oh!

kubouch commented Mar 1, 2023

Uh oh!

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

Uh oh!

Uh oh!

kubouch commented Mar 1, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

WindSoilder commented Feb 25, 2023 •

edited

Loading

codecov bot commented Feb 26, 2023 •

edited

Loading

WindSoilder commented Mar 1, 2023 •

edited

Loading