Skip to content

Allow captured stderr saving to file#6793

Merged
fdncred merged 4 commits intonushell:mainfrom
WindSoilder:save_to_stderr
Oct 20, 2022
Merged

Allow captured stderr saving to file#6793
fdncred merged 4 commits intonushell:mainfrom
WindSoilder:save_to_stderr

Conversation

@WindSoilder
Copy link
Copy Markdown
Contributor

@WindSoilder WindSoilder commented Oct 18, 2022

Description

Closes #6658

With this pr, user can do the following to redirect external commands' stdout/stderr to the same file:

do -i {external command} | save -r out.log --stderr err.log

Or

do -i {external command} | save -r out.log --stderr out.log

And no need to use complete to collect stderr data first, using complete to collect stderr data might results to high memory usage if the external command generates many messages.

Why only works with -r flag

I think we need to consume these two(stdout/stderr) message as soon as possible.

And currently, if we're using save without -r flag, and file have an extension, it'll try to invoke to xxx command, which consumes external commands stdout stream. Finally stderr stream will only be consumed when stdout is handled, which is not good.

The idle way would be using two threads, one for writing stdout, one for writing stderr, then writing these message as soon as possible, like complete, table command.

Why only works with External Stream

Because nu's internal commands just return values, and there is no stdout/stderr property of that value.

Tests

Make sure you've done the following:

  • Add tests that cover your changes, either in the command examples, the crate/tests folder, or in the /tests folder.
  • Try to think about corner cases and various ways how your changes could break. Cover them with tests.
  • If adding tests is not possible, please document in the PR body a minimal example with steps on how to reproduce so one can verify your change works.

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 --features=extra -- -D warnings -D clippy::unwrap_used -A clippy::needless_collect to check that you're using the standard code style
  • cargo test --workspace --features=extra to check that all the tests pass

Documentation

@fdncred
Copy link
Copy Markdown
Contributor

fdncred commented Oct 20, 2022

We talked about this yesterday in a team meeting and while I'm not sure this is the permanent direction we want to take, as far as UI/UX, it is a good stopgap to enable a feature that we currently do not have. Thanks @WindSoilder! Keep up the good work!

@fdncred fdncred merged commit 10aa862 into nushell:main Oct 20, 2022
@WindSoilder WindSoilder deleted the save_to_stderr branch October 20, 2022 14:44
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.

Output stdout and stderr to a file together

2 participants