Refactor Github.Client #10
Conversation
Pull Request Test Coverage Report for Build 22
💛 - Coveralls |
tallysmartins
force-pushed
the
code_refactoring
branch
from
5aceaa2
to
e74d46d
Compare
lib/elixir_bench/github/client.ex
Outdated
| def get_plain(%Client{} = client, url, params \\ []) do | ||
| get(client, url(client, url, params), [{"accept", "text/plain"}], &(&1)) | ||
| end | ||
| @callback get_plain(path :: term) :: {:ok, config :: term} | {:error, reason :: term} |
There was a problem hiding this comment.
I don't think path is term - isn't it a String?
There was a problem hiding this comment.
yes, I will change it.
|
|
||
| @behaviour ElixirBench.Github.Client | ||
|
|
||
| @params [] |
| end | ||
|
|
||
| defp url(path) do | ||
| Path.join(@base_url, path) <> "?" <> URI.encode_query(@params) |
There was a problem hiding this comment.
we seem to have totally given up on the ability to pass parameters along?
It seems like we didn't need it before so we could drop it imo until we need it again, but then we can also drop this whole part and just leave it at Path.Join.
I have a feeling they might crop up/be needed again eventually but we could wait until that happens
| end | ||
| end | ||
|
|
||
| defp get(url, headers, cb) do |
There was a problem hiding this comment.
this is picky me, but I prefer callback over cb :)
|
|
||
| @params [] | ||
| @base_url "https://raw.githubusercontent.com" | ||
| @ssl_options [] |
There was a problem hiding this comment.
same I don't think we need this, could be inlined
| end | ||
|
|
||
| def get_yaml(_) do | ||
| {:ok, %{}} |
There was a problem hiding this comment.
I think we should return at least some sensible data here :)
config/test.exs
Outdated
| @@ -17,3 +17,6 @@ config :elixir_bench, ElixirBench.Repo, | |||
| database: "elixir_bench_test", | |||
| hostname: "localhost", | |||
| pool: Ecto.Adapters.SQL.Sandbox | |||
|
|
|||
| # Github client for getting repository configuration file bench/config.yml | |||
There was a problem hiding this comment.
Not sure we need this comment in every file, I feel like this might be better as module documentation
There was a problem hiding this comment.
A module docs should definitely should be added, but I also like the comments in every settings described in the environment configuration files. Can we do both?
lib/elixir_bench/github/client.ex
Outdated
| defp url(%{base_url: base}, url, params) do | ||
| Path.join(base, url) <> "?" <> URI.encode_query(params) | ||
| end | ||
| @callback get_yaml(path :: term) :: {:ok, config :: term} | {:error, reason :: term} |
There was a problem hiding this comment.
if I see this correctly currently only get_yaml is used so we could even drop the other methods.
Am I missing something? @michalmuskala
Signed-off-by: Tallys Martins <tallysmartins@gmail.com>
tallysmartins
force-pushed
the
code_refactoring
branch
from
e74d46d
to
a7863dc
Compare
lib/elixir_bench/github/client.ex
Outdated
| end | ||
| - a `get_yaml/1` function that takes the url path of the project and returns | ||
| a map with the configuration found in the benchee.yml that is stored | ||
| inside the `benchee directory`. |
There was a problem hiding this comment.
I feel like this just duplicates information from the function definition/spec further down. If you want to add more information to the function itself, I believe you can add @doc aboove the callback definition
lib/elixir_bench/github/client.ex
Outdated
| defp url(%{base_url: base}, url, params) do | ||
| Path.join(base, url) <> "?" <> URI.encode_query(params) | ||
| end | ||
| @callback get_yaml(path :: String) :: {:ok, config :: Map} | {:error, reason :: term} |
There was a problem hiding this comment.
I don't believe these typespecs actually work but we don't really know whithout dialyzer setup :) Needs to be String.t and Map.t imo
There was a problem hiding this comment.
I will read more about how dialyzer and typespecs works. 👨🎓
|
|
||
| This client uses HTTP requests to gather data from yml files in given | ||
| repository hosted on Github. It uses the `:hackney` library to actually send | ||
| the requests and the `yamerl` library to decode the data in the yml format. |
There was a problem hiding this comment.
mentioning libraries used under the hood is too application specific imo - we can do without that. Those might change or not - whoever uses the module doesn't need to know about them though :)
There was a problem hiding this comment.
Clear, just left a brief description about the module.
| This module implements the following functions: | ||
|
|
||
| - a `get_yaml/1` function that takes the url path of the project and return | ||
| the raw parsed data in the yaml format. |
There was a problem hiding this comment.
The information which methods are implemented isn't needed - mix docs generates it and otherwise you can see it looking at the module itself.
There was a problem hiding this comment.
Alright, removed this 👍
|
|
||
| defp url(path) do | ||
| base_url = 'https://raw.githubusercontent.com' | ||
| Path.join(base_url, path) |
There was a problem hiding this comment.
I don't think we need the intermediary variable - we can just embed it in the method call or if we really wanted make it a module attribute
|
|
||
| - a `get_yaml/1` function that takes the url path of the project and return | ||
| a map with mocked data about the raw configuration found in the | ||
| benchee.yml file. |
There was a problem hiding this comment.
same thing as above, we don't need that information mix docs automatically generates it and in the file we see it either way :)
|
|
||
| def get_yaml(_) do | ||
| {:ok, | ||
| %{ |
There was a problem hiding this comment.
I believe the indentation is slightly wrong here, if you wanted you could run mix format (in Elixir >= 1.6) on just this file to fix it :)
There was a problem hiding this comment.
sorry I've missed that
| alias ElixirBench.{Benchmarks, Benchmarks.Job} | ||
| alias ElixirBench.Repos | ||
|
|
||
| describe "when correct params" do |
There was a problem hiding this comment.
usually (well in my usage and iirc elixir core as well) describe isn't used for contexts or cases but rather methods so like `describe ".create_job/2" ... test "returns a new job given correct parametres"
There was a problem hiding this comment.
...I will keep that in mind 👍
| {:ok, repo} = Repos.create_repo(%{owner: "elixir-ecto", name: "ecto"}) | ||
| job_attrs = %{branch_name: "mm/benche", commit_sha: "ABC123"} | ||
|
|
||
| jobs_count = ElixirBench.Repo.all(Job) |> length |
There was a problem hiding this comment.
you can do Repo.aggregate(Job, :count, :id) iirc to just get the count directly from the db :)
There was a problem hiding this comment.
Fixed! Later on I think I will write an assert_difference/3 function to care about the insertion/deletion assertions. Something like assert_difference(queryable, function, value)
| jobs_count = ElixirBench.Repo.all(Job) |> length | ||
| assert {:ok, %Job{}} = Benchmarks.create_job(repo, job_attrs) | ||
|
|
||
| new_count = ElixirBench.Repo.all(Job) |> length |
Signed-off-by: Tallys Martins <tallysmartins@gmail.com>
The GitHub.Client was very coupled, so I made a refactor based on this post from Jose Valim that explains how to properly use mocks and define contracts.