-
Notifications
You must be signed in to change notification settings - Fork 1
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
Initial work #6
base: main
Are you sure you want to change the base?
Initial work #6
Conversation
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.
This is really great to see! I did a first pass focusing on Req usage. Didn't pay too much attention to snowflake bits as I don't have any experience with them anyway yet. Hope it helps!
defp decode_response(%Req.Response{body: %{"message" => message}}), | ||
do: RuntimeError.exception(message) |
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.
consider returning non-successful responses as is. If your users do Req.new(http_errors: :raise)
, Req will raise the exception.
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.
Do you mean without the RuntimeError?
ie:
defp decode_response(%Req.Response{body: %{"message" => message}}),
do: message
or returning the actual response?
defp decode_response(%Req.Response{body: %{"message" => _message}} = response),
do: RuntimeError.exception(response)
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.
oh, actually we could even do something else. Have just this one clause as is:
defp decode_response(%Req.Response{status: 200, body: %{"data" => %{"token" => token}}}),
do: token
but then make this change at the callsite:
- Req.post!(url, json: data)
+ Req.post!(url, json: data, http_errors: :raise)
|> decode_response()
that is, if we got 4xx/5xx, Req will raise an exception with the (decoded) response body included in the message. On successful response, we'd call decode_response/1
.
WDYT?
Of course if you want a custom error message, what you have is the way to go.
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.
I like it, the main reason I did it the other way was because we can expose the error message Snowflake gives back to us, but for a client like this it's fine (because we should be giving back errors IMHO). That should be the responsibility the user or snowflake_elixir (db_connection) or whatever.
Read the README for a rundown. This is a port from
snowflake_elixir
so some of it might be a bit weird to get it into the req approach.You can signup for a 30 day trial for snowflake here, I can also provide a test account if you contact me on slack (@joshx on Elixir Slack) or email me (email in bio).
Right this only supports JSON, I see there is now a Rustler library for handling Arrow, I'll have to try it as the last time I played with it they had issues with some of the non-standard types Snowflake had (I think it was around decimals or timestamps from memory, I'll need to double check).
Please give as much feedback as you need to make this a great project for the community.
For some reason, running
mix test
fails, but running each test file works. I'll look into this tomorrow.Once we get this in, we'll add the following:
true
snowflake_elixir_ecto
using this library, it should be very straight forward as it already works withsnowflake_elixir
.