-
Notifications
You must be signed in to change notification settings - Fork 10
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
feat(bang-methods): add bang methods to resource base module #18
Conversation
c4412e0
to
de1b628
Compare
lib/lob/resource_base.ex
Outdated
def list!(params \\ %{}, headers \\ %{}) do | ||
case list(params, headers) do | ||
{:ok, body, headers} -> {body, headers} | ||
error -> raise to_string(error) |
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.
Don't you want to raise the error message not the whole tuple? Something like this?
{:error, error} -> raise(error)
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.
Left a comment.
@mgartner ready for another look! |
lib/lob/resource_base.ex
Outdated
@@ -18,27 +18,59 @@ defmodule Lob.ResourceBase do | |||
def list(params \\ %{}, headers \\ %{}) do | |||
Client.get_request("#{base_url()}?#{Util.build_query_string(params)}" , Util.build_headers(headers)) | |||
end | |||
|
|||
@spec list!(map, map) :: {map, list} |
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.
Nit: Do we need to update the typespecs to reflect that it can raise an error?
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.
LGTM
What
Adds
list!/2
,retrieve!/2
,create!/2
anddelete!/2
methods to resource base.Why
To follow the Elixir package conventions of having methods with a trailing bang.
Note
I haven't thought of the best way to add tests for these new methods. Adding tests to every resource might be a bit cumbersome, but I'm open to doing that.