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

Add Req.TransportError and Req.Test.transport_error/2 #323

Merged
merged 5 commits into from
Mar 16, 2024

Conversation

wojtekmach
Copy link
Owner

No description provided.

lib/req/steps.ex Outdated
Comment on lines 1184 to 1186
plug = fn _conn ->
%Req.TransportError{reason: :econnrefused}
end
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hey @whatyouhide @josevalim I have a favour ask, what do you think about this API?

I initially thought about returning just an atom:

plug = fn _conn ->
  :econnrefused
end

or to stand out even more from the Plug contract (which obviously does not supports it) maybe throw(:econnrefused) but figured exception is extensible. I hope not but maybe people want to check for Mint.HTTPError, Finch.HTTPError, etc. Which I'm not standardising on btw, meaning, Finch adapter turns Mint.TransportError to Req.TransportError but other ones it might return are returned as is.

Any feedback appreciated!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this more than the atom.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is better than the atom indeed. You could also have some sort of Req.Plug.halt(conn, reason), if you want to keep the Plug contract, and then you store the reason somewhere in the connection.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh that's a very good idea, because it allows you to keep the public API flexible!

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reason being exception struct or atom and friends?

maybe Req.Test.transport_error(conn, reason)?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I think Req.Test.transport_error(conn, reason) is the same idea, but it reads better. I also like that it keeps it scoped to Req.Test. reason would be an atom, so that

Req.Test.transport_error(conn, :timeout)

essentially stores conn.private.req_transport_error = %Req.TransportError{reason: :timeout}, which then you can use to return the error. Thoughts?

Copy link
Contributor

@whatyouhide whatyouhide left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Beautiful

lib/req/transport_error.ex Outdated Show resolved Hide resolved
@wojtekmach wojtekmach changed the title Add Req.TransportError, support returning exceptions in put_plug Add Req.TransportError and Req.Test.transport_error/2 usable in put_plug Mar 16, 2024
@wojtekmach wojtekmach changed the title Add Req.TransportError and Req.Test.transport_error/2 usable in put_plug Add Req.TransportError and Req.Test.transport_error/2 Mar 16, 2024
@wojtekmach wojtekmach merged commit a4f5396 into main Mar 16, 2024
2 checks passed
@wojtekmach wojtekmach deleted the wm-transport-error branch March 16, 2024 19:33
@wojtekmach
Copy link
Owner Author

@whatyouhide @josevalim Thank you!

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.

3 participants