Skip to content

Revert "Throw out error if external command in subexpression is failed to run (#8204)"#8475

Merged
sophiajt merged 1 commit intonushell:mainfrom
WindSoilder:revert_eager_eval_in_subexp
Mar 16, 2023
Merged

Revert "Throw out error if external command in subexpression is failed to run (#8204)"#8475
sophiajt merged 1 commit intonushell:mainfrom
WindSoilder:revert_eager_eval_in_subexp

Conversation

@WindSoilder
Copy link
Copy Markdown
Contributor

@WindSoilder WindSoilder commented Mar 16, 2023

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

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 after the PR is merged, if necessary. This will help us keep the docs up to date.

@codecov
Copy link
Copy Markdown

codecov bot commented Mar 16, 2023

Codecov Report

Merging #8475 (821c883) into main (8c487ed) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #8475      +/-   ##
==========================================
- Coverage   68.05%   68.05%   -0.01%     
==========================================
  Files         621      621              
  Lines       99938    99866      -72     
==========================================
- Hits        68015    67960      -55     
+ Misses      31923    31906      -17     
Impacted Files Coverage Δ
crates/nu-engine/src/eval.rs 74.69% <100.00%> (-0.61%) ⬇️
crates/nu-protocol/src/pipeline_data.rs 75.97% <100.00%> (-0.76%) ⬇️

... and 3 files with indirect coverage changes

@Araxeus
Copy link
Copy Markdown

Araxeus commented Mar 16, 2023

Does this fix #8467 ?

Does (cargo build) display properly?

@ghost
Copy link
Copy Markdown

ghost commented Mar 16, 2023

Does (cargo build) display properly?

yes (no delay or graphical issues for me)

@sophiajt sophiajt merged commit 31d9c08 into nushell:main Mar 16, 2023
@Araxeus
Copy link
Copy Markdown

Araxeus commented Mar 16, 2023

@jntrnr could a hotfix be released?
Since the last release introduced breaking bugs?
(and before more people download the current 0.77.0 version)

also, maybe tests should be added so that this situation doesn't happen again?

@WindSoilder WindSoilder deleted the revert_eager_eval_in_subexp branch March 17, 2023 01:12
sophiajt pushed a commit that referenced this pull request Mar 17, 2023
…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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

return statement weird behaviour in version 0.77 make the "External command failed" error more descriptive

3 participants