Support rail roading with broadcast!/2

[davejlong]

It would be useful if broadcast!/2 supported rail roading. In other words, the data to broadcast should be able to be passed as the first argument of the method. Often times, I find myself writing jobs like:

job :forecast, every: {10, :minutes} do
  forecast = APIs.ForecastIO.get location
  |> APIs.ForecastIO.forecast_for_widget

  broadcast! :forecast, forecast
end

It would be nice syntactic sugar to support:

job :forecast, every: {10, :minutes} do
  APIs.ForecastIO.get location
  |> APIs.ForecastIO.forecast_for_widget
  |> broadcast! :forecast
end

[davejlong]

This might be something that would just go away with a fix to #22, since #22 would allow the job to just be the following without any further changes to the job DSL:

job :forecast, every: {10, :minutes} do
  APIs.ForecastIO.get location
  |> APIs.ForecastIO.forecast_for_widget
  |> broadcast!
end

[saulecabrera]

Hey @davejlong @Zorbash I've been using kitto and you're right, I think this feature is helpful, I'm +1 on this.

I think that your approach of adding this as part of #22 might be a good idea, I've made a few tests and I came up with this:

1.- In case that passing a topic name different than the job name is not supported

block = Macro.prewalk (options[:do] || contents[:do]), fn
  {:broadcast!, meta, args = [_]} -> {:broadcast!, meta, [name] ++ args}
  {:|>, _, [left, {:broadcast!, meta, _}]} -> {:broadcast!, meta, [name] ++ [left]}
  ast_node -> ast_node
end

Usage

job :forecast, every: {10, :minutes} do
  APIs.ForecastIO.get location
  |> APIs.ForecastIO.forecast_for_widget
  |> broadcast!
end

2.- In case that passing a topic name different than the job name is supported

block = Macro.prewalk (options[:do] || contents[:do]), fn
  {:broadcast!, meta, args = [_]} -> {:broadcast!, meta, [name] ++ args}
  {:|>, _, [left, {:broadcast!, meta, [topic]}]} when is_atom(topic) -> {:broadcast!, meta, [topic] ++ [left]}
  {:|>, _, [left, {:broadcast!, meta, _}]} -> {:broadcast!, meta, [name] ++ [left]}
  ast_node -> ast_node
end

Usage

job :forecast, every: {10, :minutes} do
  APIs.ForecastIO.get location
  |> APIs.ForecastIO.forecast_for_widget
  |> broadcast! :other #or just broadcast! as in approach 1
end

I provided both solutions because at the moment I was looking at this I was a little unsure, but I think that approach 2 will be the correct one.

Let me know your thoughts on this, if you think it's the way to go I'll open a PR.

[zorbash]

If I read this right with your suggested changes @saulecabrera we'll support all of the below ways to broadcast:

job :forecast, every: {10, :minutes} do
  broadcast! Weather.in(:london)
  broadcast! :london_weather, Weather.in(:london)

  Weather.in(:london) |> broadcast!
  Weather.in(:london) |> broadcast! :uk_weather
end

I appreciate the flexibility of the above, but it kinda breaks the function's contract. I've never seen an Elixir function behave differently when an argument is provided with a pipe. Perhaps we can make a special exception though for the sake of usability. An alternative would be to make a backwards incompatible change and expect broadcast!(atom(), map()) to become broadcast!(map(), atom())

[zorbash]

@saulecabrera I think it's better to change lib/kitto/notifier.ex to support the alternative order of parameters like:

def broadcast!(data, topic) when is_atom(topic), do broadcast!(topic, data)

and then we'll have to Macro.prewalkonly for:

Weather.in(:london) |> broadcast!

and it'll still be possible to call broadcast! Weather.in(:london), :london_weather.

Will you create a PR for this?

[saulecabrera]

Hi @Zorbash

I've never seen an Elixir function behave differently when an argument is provided with a pipe.

Makes perfect sense. From the previous discussion it was not clear for me which approach will be the best one since this the change can be made in both ways. Your arguments make it clear now.

Will you create a PR for this?

Yes I'll submit a pull request for this.

[davejlong]

This is resolved as of #110