-
Notifications
You must be signed in to change notification settings - Fork 91
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
[Go] Refactored Stream()
API to be more ergonomic.
#766
base: main
Are you sure you want to change the base?
Conversation
The general feeling is that designs that only report the error at the end are error-prone (no pun intended). You are in a better place here, because at least FinalOutput is returning something useful as well as the error. If you think everyone will want that final value, then this is fine. If you think some people might not need it, you may have a problem. If you do go down this road (I'm not going to try and stop you beyond this comment), make sure docs and examples emphasize the need to check that error even if the final output isn't wanted. |
One other thing to think about: Go 1.23 is about to drop, and with it support for iterators, or as we call it, range-over-func. See https://eli.thegreenplace.net/2023/preview-ranging-over-functions-in-go and https://tip.golang.org/doc/go1.23. |
Next() (Stream, bool) | ||
// FinalOutput returns the final output of the flow if it has completed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does it do if the flow hasn't completed?
// again. | ||
// | ||
// Otherwise the Stream field of the passed [StreamFlowValue] holds a streamed result. | ||
func (f *Flow[In, Out, Stream]) Stream(ctx context.Context, input In, opts ...FlowRunOption) func(func(*StreamFlowValue[Out, Stream], error) bool) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I actually wrote this with range-over-func in mind. Its return type is exactly an iter.Seq2
(see https://pkg.go.dev/iter#Seq2). So think about keeping it. It seems weird now but won't in a few months.
My first stab at this (f62be7a) included returning an error from |
For us it would look like this, which is pretty clean: iter := DefineStreamingFlow(...)
for s, err := range iter {
if err != nil {
// handle err
}
if s.Done {
output := s.Output
} else {
chunk := s.Stream
}
} |
The previous API is not ideal because you have to pass in a function into an iterator-like interface that then chooses between stream value, final value, or error. This change resembles a classic iterator.
Checklist (if applicable):