-
Notifications
You must be signed in to change notification settings - Fork 341
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
Add option to deliver_now to return adapter response #466
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!
8b963ed
to
3a98e50
Compare
I reversed the tuple to be |
@maymillerricci LGTM |
…onse and email instead of just email
3a98e50
to
0422b5f
Compare
As an alternative to #452, to resolve #451 and #161, this adds a
response: true
option todeliver_now
that returns a tuple of the response from the adapter'sdeliver
call and theEmail
struct. This is a non-breaking change, since without theresponse: true
option everything will work as usual, and theEmail
will be returned, but this adds the option to pass that additional argument in and also get the response back.