Skip to content

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

Merged
sneako merged 1 commit intosneako:mainfrom
jonatanklosko:jk-stream-acc
Nov 22, 2024
Merged

Return acc when Finch.{stream,stream_while}/5 fails#295
sneako merged 1 commit intosneako:mainfrom
jonatanklosko:jk-stream-acc

Conversation

@jonatanklosko
Copy link
Copy Markdown
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
Copy Markdown
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
@jonatanklosko jonatanklosko deleted the jk-stream-acc branch November 22, 2024 16:24
@jonatanklosko
Copy link
Copy Markdown
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