output: rework LineFilter API as LineMap #24
Conversation
There was a problem hiding this comment.
Definitely starting to get a bit complicated code-wise though.
I think this comes from the fact that we're using Filter as a primitive to create a Map method and we could probably abstract that away to make it simpler for the user.
Love the iterative approach on the API <3
| if err := cmd.Run(). | ||
| Filter(func(ctx context.Context, line []byte, dst io.Writer) (int, error) { | ||
| if bytes.Contains(line, []byte("loop 3")) { | ||
| defer cancel() // Interrupt parent context here |
There was a problem hiding this comment.
I'm unsure to understand what we're trying to demonstrate here. We want to showcase that we're streaming the output, so both canceling and echoing the output in line 23 would achieve that.
The output we're getting makes sense, but the intention around the cancel function call is a bit confusing for the potential reader. I would add a comment on line 23 saying that we're never going to print this because of the cancel just above to clarify for the reader.
~/work/run/cmd/pollexample linefilter-v2 $ go run ./...
go: downloading github.com/djherbis/buffer v1.2.0
go: downloading github.com/djherbis/nio/v3 v3.0.1
This is a test in loop 1 Tue May 17 09:58:48 CEST 2022
This is a test in loop 2 Tue May 17 09:58:49 CEST 2022
2022/05/17 09:58:51 signal: killed
exit status 1
There was a problem hiding this comment.
I think it demonstrates an interrupt right now after a "ready" event, described in https://sourcegraph.slack.com/archives/C07KZF47K/p1652738100240049
output_aggregator.go
Outdated
|
|
||
| // Pipe output via the filtered line pipe | ||
| filterErrC := make(chan error, 1) | ||
| writtenC := make(chan int64, 1) |
There was a problem hiding this comment.
Unless I'm mistaken, we don't need the buffered channels here, if we read the error before reading the written count.
To put it differently, the need for the buffering comes from the fact that we're writing into A then B, while we're reading from B then A. Writing in A then B and reading in the same order removes the need for the buffered channel.
All examples work with unbuffered channels when lines 152 and 153 are swapped.
There was a problem hiding this comment.
I think buffering it means the goroutine pushing to this channel can exit, otherwise it blocks IIRC
There was a problem hiding this comment.
Yes, that's right, though my point was we don't need it here if it works by just swapping around the order we're doing things. In my experience, buffered channels tend to postpone problems and can become harder to debug, so I tend to favor avoiding them unless it's absolutely needed.
You're saying we should with this Filter API right? I wonder if we should call it |
Yeah re-reading myself, I wasn't very clear. My first thought was that if we want to do a
Filter is a specialization of Map, so I think in that case yes, it makes more sense. When I seen "filter" I think about passing a predicate to say if I want or not that line, whereas what we have here is indeed closer to Map. |
Hmm, I think that's a minor convenience (discarding I think you're right, |
|
+1 to using |
@jhchabran I just remembered we do have this, using StreamLines! |
Reworks the LineFilter API to include context and be more flexible to accommodate chaining patterns based on my thoughts in #14 and this thread, for example:
Definitely starting to get a bit complicated code-wise though. Also, to enable chaining patterns on
LineFilterwe can't flush the output until allLineFilters are executed, which can be quite awkward for streaming long-running commands. That said if those subcommand want streaming output, they should stream toos.Stdoutinstead, so maybe this all makes sense 😁