-
Notifications
You must be signed in to change notification settings - Fork 73
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
Make responders into GenServers #55
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've dug in, looks good...just bikeshed
end | ||
def dispatch(msg, responders) do | ||
Enum.map(responders, fn {_, pid, _, _} -> | ||
:ok = GenServer.cast(pid, {:dispatch, msg}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any reason not to just drop these pattern matches on cast
? It can't ever return anything else so it doesn't really serve the 'fail fast' purpose at all with cast...
case dispatch_responder(responder, msg, state) do | ||
:ok -> | ||
state | ||
{:ok, state} -> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe new_state
so this is a bit more clear? That's the elm coming out in me...
@doc false | ||
def usage(name) do | ||
@usage | ||
|> Enum.map(&String.strip/1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I want first-class function composition in Elixir for these situations :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about &(String.strip(&1) |> String.replace("hedwig", name))
...or even:
def usage(name) do
import String
@usage
|> Enum.map(&(strip(&1) |> replace("hedwig", name))
Or is this worse? :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(I never knew how much I wanted automatic currying til i had it)
{aka, opts} = Keyword.pop(opts, :aka) | ||
{name, opts} = Keyword.pop(opts, :name) | ||
responders = Keyword.get(opts, :responders, []) | ||
opts = robot.config(opts) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ever considered with
here? I'm obviously bikeshedding
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_ -> | ||
{:noreply, state} | ||
end | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
def handle_in(%Hedwig.Message{}=msg, state), do: {:dispatch, msg, state}
def handle_in(_, state), do: {:noreply, state}
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I do like this a bit better. Also...could add these to the Robot generator.
{:noreply, state} | ||
end | ||
def handle_cast({:handle_in, msg}, %{responder_sup: sup} = state) do | ||
case __MODULE__.handle_in(msg, state) do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any reason you can't omit __MODULE__
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good point.
This change makes responders into isolated
GenServer
s allowing responders to maintain/update state.TODO