Build mailer config during runtime #170
Conversation
| @@ -64,17 +64,14 @@ defmodule Bamboo.Mailer do | |||
| quote bind_quoted: [opts: opts] do | |||
| config = Bamboo.Mailer.parse_opts(__MODULE__, opts) | |||
There was a problem hiding this comment.
Is this line still needed? Should it always use the function instead?
There was a problem hiding this comment.
parse_opts was doing a Keyword.fetch!(opts, :otp_app) to enforce that it wasn't omitted. Going to include that in the macro so that can get caught during compile time.
smdern
force-pushed
the
mailer-config-during-runtime
branch
from
b7fdcac
to
fbcf969
Compare
| @spec deliver_now(Bamboo.Email.t) :: Bamboo.Email.t | ||
| def deliver_now(email) do | ||
| Bamboo.Mailer.deliver_now(@adapter, email, @config) | ||
| Bamboo.Mailer.deliver_now(adapter, email, config) |
There was a problem hiding this comment.
Random thoughts: This parses the config 2x per delivery, once to get the config, but again to get adapter which just calls config.adapter. Maybe something like:
conf = config
Bamboo.Mailer.deliver_now(conf.adapter, email, conf)
Premature optimization. :)
There was a problem hiding this comment.
👍 Appreciate the feedback.
smdern
force-pushed
the
mailer-config-during-runtime
branch
from
fbcf969
to
b5deca7
Compare
| @@ -184,8 +184,7 @@ defmodule Bamboo.Mailer do | |||
| end | |||
|
|
|||
| @doc false | |||
| def parse_opts(mailer, opts) do | |||
| otp_app = Keyword.fetch!(opts, :otp_app) | |||
| def parse_opts(mailer, otp_app) do | |||
There was a problem hiding this comment.
❓ This makes it so that the omitting :otp_app in a Mailer is caught during compile time, but it changes a public method signature. Can add an additional clause def parse_opts(mailer, opts, otp_app) if concerned
There was a problem hiding this comment.
I think it's ok because it's not documented and probably shouldn't be used by other devs anyway :)
Good question though!
|
👍 |
| @@ -184,8 +184,7 @@ defmodule Bamboo.Mailer do | |||
| end | |||
|
|
|||
| @doc false | |||
| def parse_opts(mailer, opts) do | |||
| otp_app = Keyword.fetch!(opts, :otp_app) | |||
| def parse_opts(mailer, otp_app) do | |||
| config = Application.fetch_env!(otp_app, mailer) |> Enum.into(%{}) | |||
There was a problem hiding this comment.
Since you're working on the config, could you change Enum.into(%{}) into Map.new? That is faster and I think a bit more clear.
|
Left a few small comments. Overall this looks great though :) Can you ping me when you've had a chance to implement those suggestions? Then I'll merge this in. Thanks! |
smdern
force-pushed
the
mailer-config-during-runtime
branch
from
b5deca7
to
2bae9af
Compare
@paulcsmith feedback addressed. |
| end | ||
|
|
||
| otp_app = Keyword.fetch!(opts, :otp_app) | ||
|
|
||
| defp get_config, do: Bamboo.Mailer.parse_opts(__MODULE__, unquote(otp_app)) |
There was a problem hiding this comment.
I'm thinking this should be called build_config so it implies that there is some sort of work that has to be done whenever it's called
There was a problem hiding this comment.
Actually how do you feel about Bamboo.Mailer.build_config(mailer, otp_app) and then we can just inline it in the functions as config = Bamboo.Mailer.build_config(mailer, unquote(otp_app) ? Seems like build_config would be a better public function than parse_opts.
There was a problem hiding this comment.
I think I'd have to see it first, but it sounds pretty good. Definitely worth a try
smdern
force-pushed
the
mailer-config-during-runtime
branch
from
d226f86
to
da038f5
Compare
smdern
force-pushed
the
mailer-config-during-runtime
branch
from
da038f5
to
174f1b1
Compare
|
@paulcsmith Made the changes. |
|
Thank you @smdern! |
Fixes - #166