Adapt to new Hammer API #56
Conversation
ruslandoga
changed the title
ruslandoga
force-pushed
the
new-hammer
branch
2 times, most recently
from
8bdc47e to
5e12d49
Compare
ruslandoga
force-pushed
the
new-hammer
branch
from
847180a to
d5422af
Compare
ruslandoga
force-pushed
the
new-hammer
branch
6 times, most recently
from
aeca5b7 to
7151207
Compare
|
fine for an RC. for final I do want typespec. I have spent time adding them in the past to help documenting and dialyzer be more correct |
|
The functions here are |
|
what s the plan here? merge in master or are we gonna first release Hammer then update this to proper version and update? this is still pointing to a branch |
|
The Hammer PR needs to be merged first. Then the backends. I described the plan in the Hammer PR. |
ruslandoga
changed the title
| @@ -26,7 +26,6 @@ defmodule Hammer.Redis do | |||
| prefix = Keyword.get(hammer_opts, :prefix, prefix) | |||
| timeout = Keyword.get(hammer_opts, :timeout, :infinity) | |||
|
|
|||
| # TODO | |||
| name = module | |||
There was a problem hiding this comment.
If it's unclear, maybe you should ask? :)
| @@ -51,6 +53,7 @@ defmodule Hammer.Redis do | |||
| @prefix unquote(prefix) | |||
| @timeout unquote(timeout) | |||
|
|
|||
| @spec child_spec(Keyword.t()) :: Supervisor.child_spec() | |||
There was a problem hiding this comment.
Why is it Keyword.t here but Redis.redis_opts() in start_link? :)
| @@ -59,30 +62,36 @@ defmodule Hammer.Redis do | |||
| } | |||
| end | |||
|
|
|||
| @spec start_link(Hammer.Redis.redis_opts()) :: {:ok, Redix.connection()} | |||
There was a problem hiding this comment.
This typespec is not valid. Does the connection always succeed? What if the name has already been used.
There was a problem hiding this comment.
interesting. AI failed, Dialyzer did not pick it up. And I missed it too
lib/hammer/redis.ex
Outdated
| def start_link(opts) do | ||
| opts = Keyword.put(opts, :name, @name) | ||
| Hammer.Redis.start_link(opts) | ||
| end | ||
|
|
||
| def hit(key, scale, limit, increment \\ 1, timeout \\ @timeout) do | ||
| Hammer.Redis.hit(@name, @prefix, key, scale, limit, increment, timeout) |
There was a problem hiding this comment.
Please return the timeout arg. It was by design :) If you disagree, let's discuss it first.
There was a problem hiding this comment.
that seems very confusing to have a callbacks /3 and /4 and a call here. /5 that don match. It feels as use a little confusing. I had it separate in prior commit but it felt this is something we could do later. I thought we could start matching the contract first and seprate the functional part already and add this later. Should have asked you with retrospect.
| def start_link(opts) do | ||
| opts = Keyword.put(opts, :name, @name) | ||
| Hammer.Redis.start_link(opts) | ||
| end | ||
|
|
||
| def hit(key, scale, limit, increment \\ 1, timeout \\ @timeout) do | ||
| Hammer.Redis.hit(@name, @prefix, key, scale, limit, increment, timeout) | ||
| @impl Hammer |
There was a problem hiding this comment.
interesting , because it s in a macros it s not needed. TIL
|
This PR was also merged prematurely :) Please ask me first before merging my work. And also please stop adding commits to my branches. Especially the ones with dubious value :) If you don't think we can work together like this, then I'll have to stop with my contributions. |
|
As mentioned in other PR, I am happy to chat with you so we can figure a way to collaborate. there was no intention to make you feel this way. As maintainer, i am just trying to move the needle on this package as. I have a little more time lately. |
This PR conforms to the new api introduced in ExHammer/hammer#104 and also improves performance somewhat by removing unnecessary
GenServerprocesses andpoolboy