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

Return acc when Finch.{stream,stream_while}/5 fails #295

Merged
merged 1 commit into from
Nov 22, 2024

Conversation

jonatanklosko
Copy link
Contributor

Currently Finch.stream/5 and Finch.stream_while/5 do not return accumulator in case of errors. If an error happens midway through streaming, there may be information in the accumulator that the user would like to access when handling the error.

This originally came up in wojtekmach/req#435 (comment), where in the {:status, 200} handler we "open" collectable with Collectable.into(collectable) and put it in the accumulator. If the request fails, we want to call collector.(:halt), however collector is only available in acc, and not returned on errors.

This changes the signatures as follows:

@spec stream(Request.t(), name(), acc, stream(acc), request_opts()) ::
-        {:ok, acc} | {:error, Exception.t()}
+        {:ok, acc} | {:error, Exception.t(), acc}
@spec stream_while(Request.t(), name(), acc, stream_while(acc), request_opts()) ::
-        {:ok, acc} | {:error, Exception.t()}
+        {:ok, acc} | {:error, Exception.t(), acc}

This is a breaking change, but I think it's justified, since it removes a constraint, thus making it more flexible for the user.

@sneako
Copy link
Owner

sneako commented Nov 22, 2024

Great idea, we definitely should have made it like this to start. We are still pre 1.0, so I think this breaking change, just to the stream functions, is acceptable.
Thanks @jonatanklosko!

@sneako sneako merged commit 93dd445 into sneako:main Nov 22, 2024
2 checks passed
@jonatanklosko jonatanklosko deleted the jk-stream-acc branch November 22, 2024 16:24
@jonatanklosko
Copy link
Contributor Author

Fantastic, thanks :)

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