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

Spring cleaning #138

Merged
merged 23 commits into from Jul 11, 2019
Merged

Spring cleaning #138

merged 23 commits into from Jul 11, 2019

Conversation

woylie
Copy link
Contributor

@woylie woylie commented Jun 21, 2019

Just a bit of (admittedly oddly timed) spring cleaning.

  • add formatter config and format code
  • dependency updates
  • add, fix and improve type specs
  • improve error handling in content-type extraction
  • some typo fixes
  • add credo
  • update ci config

I'm a bit confused about the case statement in OAuth2.Request.request/5:

case :hackney.request(method, url, headers, body, req_opts) do
  {:ok, ref} when is_reference(ref) ->
    {:ok, ref}

  {:ok, status, headers, ref} when is_reference(ref) ->
    process_body(client, status, headers, ref)

  {:ok, status, headers, body} when is_binary(body) ->
    process_body(client, status, headers, body)

  {:error, reason} ->
    {:error, %Error{reason: reason}}
end

According to the documentation, the return types of :hackney.request/5 are:

{ok, integer(), list(), client_ref()}
| {ok, integer(), list()}
| {ok, client_ref()}
| {error, term()}

So the branch {:ok, status, headers, body} when is_binary(body) doesn't seem to make sense. But then I'm wondering why dialyzer doesn't put out a warning? Is that branch safe to remove?

@scrogson
Copy link
Member

scrogson commented Jun 21, 2019

So the branch {:ok, status, headers, body} when is_binary(body) doesn't seem to make sense.

I can't remember exactly, but I think this is to support to with_body: true option.

https://github.com/benoitc/hackney/blob/master/src/hackney.erl#L210-L213

@woylie
Copy link
Contributor Author

woylie commented Jun 21, 2019

Indeed. I added a test case for that.

@woylie
Copy link
Contributor Author

woylie commented Jul 10, 2019

I'm done here, by the way, so you can merge this, if you want. Otherwise I'll close it.

@scrogson
Copy link
Member

I'm done here, by the way, so you can merge this, if you want. Otherwise I'll close it.

Ok, great. I'll review.

@scrogson scrogson merged commit f13aaf9 into ueberauth:master Jul 11, 2019
@scrogson
Copy link
Member

@woylie 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.

None yet

2 participants