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 buffered combinedoutput #76

Merged

Conversation

jwomackgsa
Copy link
Contributor

Additional feature that allows the buffered output to have stdout and stderr combined into the same buffer which provides a capability to easily dump the buffered output to a log file.

@jwomackgsa
Copy link
Contributor Author

I am still interested in this being added into the project. I brought my fork into sync with the recent updates and should merge cleanly if accepted. I welcome any comments.

@ip-rw
Copy link

ip-rw commented Dec 3, 2022

I would like this to be merged too... If that helps :)

@MXWXZ
Copy link

MXWXZ commented Jul 6, 2023

+1 respect, useful feature

@daniel-nichter
Copy link
Member

Sorry for long delay. Free open source software moves very slowly at times.

The feature idea is good, but let's fix two things:

  1. s/BufferedCombined/CombinedOutput/ for two reasons. It mirrors Go os.Exec, and later we should extend the functionality to streaming output, too. For now, we can document that Option.CombinedOutput only works for buffered.
  2. Test coverage is missing on,
        case c.stdoutBuf != nil && c.stderrBuf == nil: // buffer combining stderr into stdout
                cmd.Stdout = c.stdoutBuf
                cmd.Stderr = c.stdoutBuf

I'm not sure why because you've got a test case, but it seems the test case doesn't actually hit that code.

@jwomackgsa
Copy link
Contributor Author

Great to see some movement on this PR. I will work on making some updates on the naming and the test cases and report back for another review.

@jwomackgsa
Copy link
Contributor Author

I have updated my branch with the option name update and the additional coverage test. Coverage is now reporting 100%. Please let me know if anything further is needed to merge this feature.

@daniel-nichter
Copy link
Member

Ack, thanks @jwomackgsa. I'll take a look this weekend.

@daniel-nichter daniel-nichter merged commit 4087270 into go-cmd:master Jul 8, 2023
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.

4 participants