Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add option to show output on error #371

Closed

Conversation

jelle-bigbridge
Copy link

Fixes #55

Changes

  • Add option to show output on error

@westhomas
Copy link

This is really needed for when commands fail. Otherwise there's no good error messages from stderr and your users have to re-run the command by hand trying to debug why it failed.

spin/options.go Outdated Show resolved Hide resolved
spin/options.go Outdated Show resolved Hide resolved
@maaslalani
Copy link
Contributor

Hey, thanks for the contribution. Can we call this ShowError instead of OutputOnError to match a little better with ShowOutput.

jelle-bigbridge and others added 2 commits August 14, 2023 09:20
Co-authored-by: Maas Lalani <[email protected]>
Co-authored-by: Maas Lalani <[email protected]>
@jelle-bigbridge
Copy link
Author

@hopefulTex could you take a look at this PR. You changed some stuff that this PR depends on, and I'm not sure how to do it now. I think this would still be a really useful feature so maybe you could help make this PR work

@hopefulTex
Copy link
Contributor

Pardon the dust!

Adding a distinction is definitely useful, I'd love to get this updated soon but a few questions still remain.

Currently spin is supposed to show both the output and the error in one block, with the ui being displayed in the stderr, with both outputs mirrored to stdout when using the --show-output flag.

  • Should the error be put on the stderr or the stdout?
  • If we use stderr, how do we distinguish between the ui and the actual error?
  • If stdout is used for the error, how can this play nicely with the --show-output flag?
  • Is this feature meant for parsing errors with a script or program, or will a human-readable separator suffice?
  • How can we make a separator that plays nice with screen-readers?

I'll be updating this to match with the current version of gum, but I believe these questions are probably better answered by people who want/need this for their script or workflow (not me).

@jelle-bigbridge
Copy link
Author

My preferred implementation (I want to use it in a script, and I was using my previous implementation in a script already). Is that stdout and stderr are combined by the system (idk how this works in go but the unix system call is dup2), so that it is the same as if running the command on the shell, and messages on stdout and stderr are interleaved.

Only when the subcommand exits with a non-0 exit code, will the output be shown to the user

  • Should the error be put on the stderr or the stdout?

stderr seems like it makes sense

  • If we use stderr, how do we distinguish between the ui and the actual error?

no distinction is needed, as it should read the same as if the command was normally executed in a user shell session

  • If stdout is used for the error, how can this play nicely with the --show-output flag?

I don't think it makes sense to use the --show-output flag in combination with this feature

  • Is this feature meant for parsing errors with a script or program, or will a human-readable separator suffice?

meant for humans

  • How can we make a separator that plays nice with screen-readers?

n/a

@hopefulTex
Copy link
Contributor

So the ideal behavior is (correct me if I'm wrong):
--show-error will output the accumulated buffer if the program doesn't end on 0. (stderr)
--show-output will display the accumulated buffer in the UI as it appears. (stderr)
--show-output will output the accumulated buffer if the command is being piped. (stdout)

Since we won't know the exit code until the program has finished, it would be easiest (and more flexible) to spit the buffer out in one chunk at the end of execution- similar to when the output is being piped. This would make it a simple check for the --show-error flag && the return code to proc the output and switch the buffer from stdout to stderr.

elliotcourant added a commit to elliotcourant/gum that referenced this pull request Oct 14, 2023
This makes it so that if the `--show-error` flag is provided then the
full output of the command will be printed if the command fails. This
kind of works in conjuncture with `--show-output` in that if the command
succeeds only STDOUT is pushed. If the command fails both `STDOUT` and
`STDERR` are pushed.

This builds off of charmbracelet#371

Resolves charmbracelet#55
@maaslalani
Copy link
Contributor

Hey @jelle-bigbridge, thank you so much for the PR. gum spin added some new changes for live streaming the output of gum spin which would require some changes to this PR.

A PR has been opened to fix this --show-error issue which accounts for the new changes: #440

@jelle-bigbridge
Copy link
Author

Let's close this in favour of #440

pingiun pushed a commit to pingiun/gum that referenced this pull request Mar 27, 2024
This makes it so that if the `--show-error` flag is provided then the
full output of the command will be printed if the command fails. This
kind of works in conjuncture with `--show-output` in that if the command
succeeds only STDOUT is pushed. If the command fails both `STDOUT` and
`STDERR` are pushed.

This builds off of charmbracelet#371

Resolves charmbracelet#55
maaslalani pushed a commit that referenced this pull request Mar 28, 2024
… (#518)

* feat(spin): Add support for `--show-error` for the spinner.

This makes it so that if the `--show-error` flag is provided then the
full output of the command will be printed if the command fails. This
kind of works in conjuncture with `--show-output` in that if the command
succeeds only STDOUT is pushed. If the command fails both `STDOUT` and
`STDERR` are pushed.

This builds off of #371

Resolves #55

* chore: Fix formatting

---------

Co-authored-by: Elliot Courant <[email protected]>
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.

[feature request] add option for gum spin to dump output on error
4 participants