Skip to content

command: lift StdOut, StdErr into command, lift aggregator into output#29

Merged
bobheadxi merged 3 commits into
mainfrom
lift-output-and-aggregator
May 20, 2022
Merged

command: lift StdOut, StdErr into command, lift aggregator into output#29
bobheadxi merged 3 commits into
mainfrom
lift-output-and-aggregator

Conversation

@bobheadxi

@bobheadxi bobheadxi commented May 20, 2022

Copy link
Copy Markdown
Member

There doesn't seem to be a use case for changing StdOut, StdErr on-the-fly with Output, so we lift that configuration into Command, which greatly simplifies the initialisation of Output and reduces the amount of buffers we need to keep around. It also means that Output is almost purely about consuming output, which means aggregator is no longer needed, which also makes the story a lot simpler.

API-wise, the change looks like:

-  err = run.Cmd(ctx, "ls", "foobar").Run().StdOut().Stream(os.Stdout)
+  err = run.Cmd(ctx, "ls", "foobar").StdOut().Run().Stream(os.Stdout)

Closes #27

In the future, this could also improve the possibility of #5 by not having the full command be created immediately - instead we can export specifically the customizable components

@bobheadxi
bobheadxi requested review from a team and jhchabran May 20, 2022 17:29
@bobheadxi
bobheadxi requested a review from michaellzc as a code owner May 20, 2022 17:29
@bobheadxi

bobheadxi commented May 20, 2022

Copy link
Copy Markdown
Member Author

Current dependencies on/for this PR:

This comment was auto-generated by Graphite.

@codecov-commenter

codecov-commenter commented May 20, 2022

Copy link
Copy Markdown

Codecov Report

Merging #29 (b020a1b) into main (e4a6222) will increase coverage by 2.05%.
The diff coverage is 75.70%.

@@            Coverage Diff             @@
##             main      #29      +/-   ##
==========================================
+ Coverage   67.48%   69.54%   +2.05%     
==========================================
  Files           9        8       -1     
  Lines         366      348      -18     
==========================================
- Hits          247      242       -5     
+ Misses         96       85      -11     
+ Partials       23       21       -2     
Impacted Files Coverage Δ
output.go 76.61% <74.86%> (-23.39%) ⬇️
command.go 60.00% <77.27%> (+23.63%) ⬆️
error.go 71.42% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e4a6222...b020a1b. Read the comment docs.

@bobheadxi

Copy link
Copy Markdown
Member Author

I'm goiing to merge this directly because I have some exciting refactors coming

@bobheadxi
bobheadxi merged commit 0308c5d into main May 20, 2022
@bobheadxi
bobheadxi deleted the lift-output-and-aggregator branch May 20, 2022 22:32
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.

idea: remove output.StdOut

2 participants