-
Notifications
You must be signed in to change notification settings - Fork 353
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
Fix upload file api #334
Fix upload file api #334
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.
Looks good to me. I like the separate perform_request
function.
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.
Looks good. Good work on drying ti up as well.
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.
The actual changes look good to merge. What's needed is a correction of a couple of miner formatting issues and then we're good to go.
lib/stripe/util.ex
Outdated
@@ -65,6 +65,11 @@ defmodule Stripe.Util do | |||
module |> Atom.to_string() |> String.trim_leading("Elixir.") | |||
end | |||
|
|||
def multipart_key(:file),do: :file | |||
def multipart_key(key) when is_atom(key),do: Atom.to_string(key) | |||
def multipart_key(key),do: key |
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.
Some formatting issues here
,do
needs to be , do
lib/stripe/api.ex
Outdated
{ Stripe.Util.multipart_key(key), value} | ||
end) | ||
|
||
perform_request(req_url, :post, {:multipart, parts}, headers, opts ) |
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.
You got an extra space at the end here. opts )
-> opts)
lib/stripe/api.ex
Outdated
|> add_auth_header(api_key) | ||
|> add_connect_header(connect_account_id) | ||
|> Map.to_list() | ||
perform_request(req_url, method, req_body, headers, opts ) |
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.
Extra space here. opts )
-> opts)
lib/stripe/util.ex
Outdated
def multipart_key(key) when is_atom(key),do: Atom.to_string(key) | ||
def multipart_key(key),do: key | ||
|
||
|
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.
You can remove one of these blank lines here.
lib/stripe/api.ex
Outdated
parts = | ||
body | ||
|> Enum.map(fn({key, value})-> | ||
{ Stripe.Util.multipart_key(key), value} |
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.
Extra space here: { Stripe.Util.multipart_key(key), value}
-> {Stripe.Util.multipart_key(key), value}
lib/stripe/api.ex
Outdated
|
||
@http_module.request(method, req_url, req_headers, req_body, req_opts) | ||
|> handle_response() | ||
perform_request(req_url, method, req_body, headers, opts ) |
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.
opts )
-> opts)
@begedin Formatting applied 👍 |
.gitignore
Outdated
@@ -6,6 +6,8 @@ config/config.secret.exs | |||
/doc | |||
.DS_Store | |||
|
|||
#elixir LS files | |||
/.elixir_ls | |||
# IntelliJ IDEA files |
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.
Sorry I missed this on the first passthrough, but more formatting issues here
- there should be a space in the comment:
#elixir LS files
-># elixir LS files
- there should be a blank line between lines 10 and 11
@begedin All formatting issues was addressed 👍 |
@chubarovNick Sorry for the delay on this. Merged. Thank you for your work! |
No description provided.