-
Notifications
You must be signed in to change notification settings - Fork 342
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
Allow passing extra arguments to Interceptor modules #602
Comments
Follwing this example, i think we could have the config :my_app, MyApp.Mailer,
adapter: Bamboo.MandrillAdapter,
interceptors: [{MyApp.Mailer.Interceptor, environment: "staging", foo: :bar}] |
Thanks @gugahoa for opening this issue. I can see how passing an extra set of options could be desired. If we do this, I think @aleDsz's idea of passing a tuple with the interceptor module and then the options could work nicely. So we could make it so that the elements of the config :my_app, MyApp.Mailer,
adapter: Bamboo.MandrillAdapter,
interceptors: [MyApp.Mailer.OneInterceptor, {MyApp.Mailer.AnotherInterceptor, environment: "staging", foo: :bar}] Then, when we're applying the interceptors in the mailer, we can pattern match based on whether it's a tuple or not: defp apply_interceptors(email, config) do
interceptors = config[:interceptors] || []
Enum.reduce(interceptors, email, fn
{interceptor, opts}, email -> apply(interceptor, :call, [email, opts]) end)
interceptor, email -> apply(interceptor, :call, [email]) end)
end
end We'd then have to change the interceptor behaviour to allow for If that fits your use case, I'd be happy to review a pull request with these changes. Otherwise, I'll try to get to it as soon as I can! |
@germsvel we can't define two callbacks with different arguments for the behavior, so the approach would have to be a optional callback—to not break existing code—but since As a user I believe this API is not good. Maybe we could approach it another way? With a new behavior. The new behavior could be the recommended way to define interceptors, with the "old" way being deprecated and removed in the next major version |
Using your commit, we should still use |
@gugahoa @aleDsz what if we were to make both I did a quick spike with a simple test to check that it works: https://github.com/thoughtbot/bamboo/compare/spike-interceptors. What do you think? |
Making both optional sounds good, but i don't think it would be a good interface. Because if someone is implementing the So, if we make it with |
@aleDsz yeah, I think adding the |
Today to configure a interceptors, we add the module to a list in the config file:
So if we want to change the behavior of a Interceptor with a config, we have to fetch the env inside the module and do what we need to do. For example:
What about passing these options via the config file, like this?
We would have to then pass the option to the
call
function in the Interceptor, like it's done with Plugs. What do you think of this? If it's a desirable contribution, could you give some pointers on how to implement?The text was updated successfully, but these errors were encountered: