Skip to content

Support redirect stderr and stdout+stderr with a pipe#11708

Merged
WindSoilder merged 12 commits intonushell:mainfrom
WindSoilder:err_pipe
Feb 8, 2024
Merged

Support redirect stderr and stdout+stderr with a pipe#11708
WindSoilder merged 12 commits intonushell:mainfrom
WindSoilder:err_pipe

Conversation

@WindSoilder
Copy link
Copy Markdown
Contributor

@WindSoilder WindSoilder commented Feb 2, 2024 •
edited
Loading

Description

Close: #9673
Close: #8277
Close: #10944

This pr introduces the following syntax:

  1. e>|, pipe stderr to next command. Example: $env.FOO=bar nu --testbin echo_env_stderr FOO e>| str length
  2. o+e>| and e+o>|, pipe both stdout and stderr to next command, example: $env.FOO=bar nu --testbin echo_env_mixed out-err FOO FOO e+o>| str length

Note: it only works for external commands. There is no different for internal commands, that is, the following three commands do the same things: Edit: it raises errors if we want to pipes for internal commands

❯ ls e>| str length
Error:   × `e>|` only works with external streams
   ╭─[entry #1:1:1]
 1 │ ls e>| str length
   ·    ─┬─
   ·     ╰── `e>|` only works on external streams
   ╰────

❯ ls e+o>| str length
Error:   × `o+e>|` only works with external streams
   ╭─[entry #2:1:1]
 1 │ ls e+o>| str length
   ·    ──┬──
   ·      ╰── `o+e>|` only works on external streams
   ╰────

This can help us to avoid some strange issues like the following:

$env.FOO=bar (nu --testbin echo_env_stderr FOO) e>| str length

Which is hard to understand and hard to explain to users.

User-Facing Changes

Nan

Tests + Formatting

To be done

After Submitting

Maybe update documentation about these syntax.

@WindSoilder WindSoilder changed the title Err pipe Support redirect stderr and stdout+stderr with a pipe Feb 2, 2024
@kubouch
Copy link
Copy Markdown
Contributor

kubouch commented Feb 3, 2024 •
edited
Loading

I'm OK with it because it fills in missing functionality. The redirection syntax is becoming quite complex at this point. It would be good to have an overview of all possible ways, including do { ... } | complete etc., you can redirect stdout/stderr to file/pipe in some table and put it to our Reference. Seeing them all in one place would also help us tighten the design/syntax if necessary.

But as I said, we should land this PR because it's consistent with our current style.

@WindSoilder WindSoilder mentioned this pull request Feb 4, 2024
Merged
@WindSoilder
Copy link
Copy Markdown
Contributor Author

WindSoilder commented Feb 4, 2024 •
edited
Loading

Thanks for feedback! there is one more thing which is unclear to me.

Currently do -i { ls } | complete raises an error:

Error:   × Complete only works with external streams
   ╭─[entry #35:1:1]
 1 │ do -i { ls } | complete
   ·                ────┬───
   ·                    ╰── complete only works on external streams
   ╰────

Do we want to raise error while using e>|(o+e>|) with internal commands? e.g ls e>| str length name and ls o+e>| str length name
Edit: currently I purpose to raise errors if we use e>|, o+e>| with internal command

@kubouch
Copy link
Copy Markdown
Contributor

kubouch commented Feb 4, 2024

Do we want to raise error while using e>|(o+e>|) with internal commands? e.g ls e>| str length name and ls o+e>| str length name

Maybe we should, to make it consistent with complete.

@IanManske
Copy link
Copy Markdown
Member

IanManske commented Feb 4, 2024

Great, it'll be nice to be able to redirect stderr.

Correct me if I'm wrong, but isn't o>| and o+e>| unnecessary, since stdout is always piped for external commands (if it's followed by a pipe)? Maybe we could add something like e>o which redirects stderr to stdout.

@WindSoilder
Copy link
Copy Markdown
Contributor Author

WindSoilder commented Feb 4, 2024

@IanManske Yup, o>| is unnecessary, this pr doesn't support the syntax :)

Maybe we could add something like e>o which redirects stderr to stdout.

I think it's 2>&1 in bash. I don't think it's necessary either. Currently we use o+e> file to redirect both stdout and stderr to a file, so I think it's good to keep consistency to use the similar syntax like o+e>| nextcmd to pipe both stdout and stderr to next command.

So they would be idential:

bash fish nu
cmd 1>result.log 2>&1 cmd &> result.log cmd o+e> result.log
cmd 2>&1 | next_cmd cmd &| next_cmd cmd o+e>| next_cmd

@IanManske
Copy link
Copy Markdown
Member

IanManske commented Feb 4, 2024

Yup, o>| is unnecessary, this pr doesn't support the syntax :)

Sorry, I misread the PR 😅

I also wrongly thought that command1 e>| command2 had the same result as command1 o+e>| command2. But it looks like stdout is ignored for command1 e>| command2? If so, this makes sense, and the consistent syntax is nice!

@WindSoilder
Copy link
Copy Markdown
Contributor Author

WindSoilder commented Feb 5, 2024

But it looks like stdout is ignored for command1 e>| command2?

Yeah, it's ignored :)

@WindSoilder WindSoilder marked this pull request as ready for review February 5, 2024 08:26
@WindSoilder WindSoilder added the deprecated:pr-language (deprecated: this label is too vague) This PR makes some language changes label Feb 6, 2024
@WindSoilder
Copy link
Copy Markdown
Contributor Author

WindSoilder commented Feb 8, 2024

Let's go and try it

@WindSoilder WindSoilder merged commit 58c6fea into nushell:main Feb 8, 2024
@hustcer hustcer added this to the v0.91.0 milestone Feb 9, 2024
@rgwood
Copy link
Copy Markdown
Contributor

rgwood commented Feb 9, 2024

Thank you for continuing to work on redirection!

dmatos2012 pushed a commit to dmatos2012/nushell that referenced this pull request Feb 20, 2024
@WindSoilder @dmatos2012
Support redirect stderr and stdout+stderr with a pipe (nushell#11708)
b960699 
# Description
Close: nushell#9673
Close: nushell#8277
Close: nushell#10944

This pr introduces the following syntax:
1. `e>|`, pipe stderr to next command. Example: `$env.FOO=bar nu
--testbin echo_env_stderr FOO e>| str length`
2. `o+e>|` and `e+o>|`, pipe both stdout and stderr to next command,
example: `$env.FOO=bar nu --testbin echo_env_mixed out-err FOO FOO e+o>|
str length`

Note: it only works for external commands. ~There is no different for
internal commands, that is, the following three commands do the same
things:~ Edit: it raises errors if we want to pipes for internal
commands
``` 
❯ ls e>| str length
Error:   × `e>|` only works with external streams
   ╭─[entry nushell#1:1:1]
 1 │ ls e>| str length
   ·    ─┬─
   ·     ╰── `e>|` only works on external streams
   ╰────

❯ ls e+o>| str length
Error:   × `o+e>|` only works with external streams
   ╭─[entry nushell#2:1:1]
 1 │ ls e+o>| str length
   ·    ──┬──
   ·      ╰── `o+e>|` only works on external streams
   ╰────
```

This can help us to avoid some strange issues like the following:

`$env.FOO=bar (nu --testbin echo_env_stderr FOO) e>| str length`

Which is hard to understand and hard to explain to users.

# User-Facing Changes
Nan

# Tests + Formatting
To be done

# After Submitting
Maybe update documentation about these syntax.
@WindSoilder WindSoilder deleted the err_pipe branch February 27, 2024 05:49
@fdncred fdncred mentioned this pull request Apr 24, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

deprecated:pr-language (deprecated: this label is too vague) This PR makes some language changes

Projects

None yet

Milestone

v0.91.0

5 participants

@WindSoilder @kubouch @IanManske @rgwood @hustcer