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

[Go] Refactored Stream() API to be more ergonomic. #766

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

apascal07
Copy link
Collaborator

@apascal07 apascal07 commented Aug 9, 2024

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):

  • Tested (manually, unit tested, etc.)
  • Changelog updated
  • Docs updated

@apascal07 apascal07 requested a review from jba August 9, 2024 01:36
@apascal07 apascal07 requested a review from pavelgj August 9, 2024 03:01
@apascal07 apascal07 marked this pull request as ready for review August 9, 2024 03:01
@jba
Copy link
Contributor

jba commented Aug 9, 2024

The general feeling is that designs that only report the error at the end are error-prone (no pun intended).
The classic example is https://pkg.go.dev/bufio#Scanner, where to make the iteration convenient, you have to call a function at the end to get the error. We know empirically, from looking at a lot of code, that people often forget to make that call to Err.

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.

@jba
Copy link
Contributor

jba commented Aug 9, 2024

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.
Now, the Go team promises support for the last two versions of Go, so I recommend you do too. That means you shouldn't use this feature until Go 1.24 comes out in about six months. But it's worth thinking about how you'd design your iterators with range-over-func.

Next() (Stream, bool)
// FinalOutput returns the final output of the flow if it has completed.
Copy link
Contributor

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) {
Copy link
Contributor

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.

@apascal07
Copy link
Collaborator Author

The general feeling is that designs that only report the error at the end are error-prone (no pun intended). The classic example is https://pkg.go.dev/bufio#Scanner, where to make the iteration convenient, you have to call a function at the end to get the error. We know empirically, from looking at a lot of code, that people often forget to make that call to Err.

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.

My first stab at this (f62be7a) included returning an error from Next() so that the caller can be notified if it fails mid-stream. In many cases, users will want the final output so I don't find it that risky or strange to have to call it to get the final result, whether it's good output or an error. I agree with you it should be well documented that it needs to be checked.

@apascal07
Copy link
Collaborator Author

apascal07 commented Aug 9, 2024

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. Now, the Go team promises support for the last two versions of Go, so I recommend you do too. That means you shouldn't use this feature until Go 1.24 comes out in about six months. But it's worth thinking about how you'd design your iterators with range-over-func.

@pavelgj

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
    }
}

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.

2 participants