Adds Repo.slug/1 helper function #24
Conversation
tallysmartins
commented
May 31, 2018
tallysmartins
commented
- closes Create a Repo.slug helper function #13
lib/elixir_bench/repos/repo.ex
Outdated
| @@ -16,4 +16,8 @@ defmodule ElixirBench.Repos.Repo do | |||
| |> cast(attrs, [:owner, :name]) | |||
| |> validate_required([:owner, :name]) | |||
| end | |||
|
|
|||
| def slug(%Repo{} = repo) do | |||
There was a problem hiding this comment.
I wanted to ask if we need to match on repo here, but it's done in other parts of the code base as well 🤔
Upside I see is that it fails early when we don't pass a repo, downside is we can't pass a normal map.
Intuitively I wouldn't match on it but it also makes sense so 🤷♂️
Any input @michalmuskala ? ;P
There was a problem hiding this comment.
Yes, I also wanted to ask you when to do or not this kind of match.. in the ElixirBench.Benchmarks module, for example, some methods does this kind of match like in submit_job(%Job{} = job, results) but others don't def create_job(repo, attrs)
I think we can remove the match indeed. This matches should be used in cases where different types make calls to the same method but with different behaviors, right?
There was a problem hiding this comment.
yeah I think we can remove it until michal tells us otherwise :)
| end | ||
|
|
||
| test "delete_repo/1 deletes the repo" do | ||
| repo = repo_fixture() | ||
| assert {:ok, %Repo{}} = Repos.delete_repo(repo) | ||
| assert Repos.fetch_repo_by_slug(repo.owner <> "/" <> repo.name) == {:error, :not_found} | ||
| assert Repos.fetch_repo_by_slug(Repos.Repo.slug(repo)) == {:error, :not_found} |
There was a problem hiding this comment.
in general one should be careful with using own application code in assertions. Because that's easily how bugs bubble up and aren't detected.
For the assertions, as I assume the name is constant we could just put the verbatim repo slug in a module attribute `@repo_slug "my-cool/repo-slug"
There was a problem hiding this comment.
Didn't get the point. You mean not to use in tests or just inside an assertion? I make use of this method in schema_test as well, should I also remove from it?
There was a problem hiding this comment.
Hey, yeah don't use it in assertion or well... mostly don't. You want your "test code" and production code not to be interdependent so much (other than the thing under test of course)
Imagine you break Repo.slug - all the tests using it would be broken as well!
