rockneurotiko/ex_gram

[Tesla-Middlewares] Can not write functions in config

Ironjanowar opened this issue · 4 comments

So... I did not make a release when testing the Tesla middlewares PR.

This error raises when a function is in a config file:
** (Mix) Could not read configuration file. It likely has invalid configuration terms such as functions, references, and pids. Please make sure your configuration is made of numbers, atoms, strings, maps, tuples and lists. Reason: {3, :erl_parse, ['syntax error before: ', 'Fun']}

As you can see the config requires a function in the case of the Tesla.Middleware.Retry

My proposition is to change the config to this:

config :ex_gram, ExGram.Adapter.Tesla,
  middlewares: [
    {TestBot.TeslaMiddlewares, :retry, []}
  ]

Basically ExGram receives in the config a mfa of a module and function defined somewhere in the project. The ExGram.Adapter.Tesla.custom_middleware/0 function should apply all the functions.

The mfa functions should return a middleware as defined by Tesla, this means:

defmodule TestBot.TeslaMiddlewares do
  def retry() do
    {Tesla.Middleware.Retry,
     delay: 500,
     max_retries: 10,
     max_delay: 4_000,
     should_retry: fn
       {:ok, %{status: status}} when status in [400, 500] -> true
       {:ok, _} -> false
       {:error, _} -> true
     end}
  end
end

What do you think @rockneurotiko ?

What I don't really like is that now you would have to create one method for every middleware, just because some middlewares have functions and can't be compiled to a release.

Maybe we can have something like:

middlewares: [{SomeMiddleware, []}, {MyMiddleware, :method, []}]

Middleware configuration is always {module, args}, so, we can accept {m, a} and treat them as middlewares and accept {m, f, a}, and call it to extract the middleware.

Maybe that's too implicit, and we prefer something more explicit:

middlewares: [{SomeMiddleware, []}, {:local, MyModule, :method, []}

WDYT?

I think if we document it properly using {m, f, a} is explicit enough.
I don't know if the :local atom gives enough information to understand what is happening, in that case you will need to check out the docs sooner or later and I think it looks cleaner with {m, a} and {m, f, a}.

Sounds nice 👍