Skip to content

Switched the GitHub API to use a simple Gateway module#1138

Merged
joshsmith merged 1 commit intodevelopfrom
refactor-github-api-to-use-gateway
Oct 30, 2017
Merged

Switched the GitHub API to use a simple Gateway module#1138
joshsmith merged 1 commit intodevelopfrom
refactor-github-api-to-use-gateway

Conversation

@begedin
Copy link
Copy Markdown
Contributor

@begedin begedin commented Oct 30, 2017

Closes #1131,
Progress on #1132

This PR

  • extracts a simple API.Gateway module from out of GitHub.API
  • rewrites tests and the rest of the code to use/mock this module
  • rewrites API.EagerAPI into API.Pagination, which is a module containing a set of utility functions to be used from API, so Gateway is only ever called from API
  • rewrites API.get_all so it uses a :head request first, to further consolidate and decouple
  • fixes a bunch of dialyzer issues found along the way

@begedin begedin requested a review from joshsmith October 30, 2017 15:17
joshsmith
joshsmith previously approved these changes Oct 30, 2017
|> Enum.map(&marshall_response/1)
|> Enum.map(&Tuple.to_list/1)
|> Enum.map(&List.last/1)
|> List.flatten
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I just read (oddly in benchee's README) that Enum.map |> List.flatten is 1.83x slower than Enum.flat_map. Wonder if we should use that instead here?

end
end
defp marshall_response({:ok, 404, _headers, body}) do
defp marshall_response({:ok, %Response{body: body, status_code: 404}}) do
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Good fix here.

end
defp marshall_response({:error, %Error{reason: reason}}) do
{:error, HTTPClientError.new(reason: reason)}
end
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Much needed.

last = '<two-pages/?page=2>; rel="last"'

headers = [{"Link", [next, last] |> Enum.join(", ")}]
{:ok, %HTTPoison.Response{headers: headers, status_code: 200}}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is all we needed really for the pagination test. IMO this satisfies most of what I wanted in #1132. What do you think @begedin?

Replaced API.EagerAPI with decoupled set of helper functions to be used from within API
@joshsmith joshsmith force-pushed the refactor-github-api-to-use-gateway branch from 205fc69 to 8b6d5a4 Compare October 30, 2017 17:59
@joshsmith joshsmith merged commit 7725cb9 into develop Oct 30, 2017
@joshsmith joshsmith deleted the refactor-github-api-to-use-gateway branch October 30, 2017 18:04
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.

Add GitHub.API.Gateway which calls HTTPoison

2 participants