Add option to deliver_now to return adapter response #466
Conversation
There was a problem hiding this comment.
@maymillerricci This looks great! Seeing this implemented, I think, bolsters the case for an approach like this over adding to the email struct. Thanks for doing this. I just have one question for discussion but other than that 👍
lib/bamboo/mailer.ex
Outdated
| else | ||
| debug_sent(email, adapter) | ||
| response = adapter.deliver(email, config) | ||
| {response, email} |
There was a problem hiding this comment.
How did you decide to order the tuple response first then email? At first glance I think I may have expected the order reversed. That's not to suggest that you should reverse the order; I just thought it would be interesting to talk through.
There was a problem hiding this comment.
Great question! I initially decided to do {response, email} since I was thinking of the typical {:ok, thing} / {:error, thing} pattern and was thinking of :ok and :error as a response, so the response here would match that and come first. But now that I think about it more, it's different since it's more than just an atom with like a response status, and it comes after the sending of the email, so it probably makes more sense to come second. Plus, since this is an addition to the existing function which just returns an email, it also probably makes sense for the extra information to come after the expected information. I think I will probably reverse it. Thanks!
maymillerricci
force-pushed
the
mmr-response-option
branch
3 times, most recently
from
8b963ed
to
3a98e50
Compare
|
I reversed the tuple to be |
|
@maymillerricci LGTM |
…onse and email instead of just email
maymillerricci
force-pushed
the
mmr-response-option
branch
from
3a98e50
to
0422b5f
Compare
As an alternative to #452, to resolve #451 and #161, this adds a
response: trueoption todeliver_nowthat returns a tuple of the response from the adapter'sdelivercall and theEmailstruct. This is a non-breaking change, since without theresponse: trueoption everything will work as usual, and theEmailwill be returned, but this adds the option to pass that additional argument in and also get the response back.