-
Notifications
You must be signed in to change notification settings - Fork 333
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
Empty recipients #40
Empty recipients #40
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,8 +1,8 @@ | ||
defmodule Bamboo.Email do | ||
defstruct from: nil, | ||
to: nil, | ||
cc: nil, | ||
bcc: nil, | ||
to: [], | ||
cc: [], | ||
bcc: [], | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually I think this should go back to nil and it should raise it all recipients are nil. That would mean you forgot to set something There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. After thinking about this more, I think this should be how it's handled. If it starts as nil and you set an list then you probably meant to do that. I can see it going either way though Here are a couple scenarios:
So I think I'm going to go ahead with keep the default addresses as nil, and raising only if all of them are nil, and nooping if the the recipients are an empty array |
||
subject: nil, | ||
html_body: nil, | ||
text_body: nil, | ||
|
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.
Maybe this should be
{:ok, email}
so you can pattern match on the result...? Then I'd need to adddeliver!
anddeliver_async!
I think, and not raise here if there is an API error, instead return{:error, mandrill_response}
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.
Being able to pattern match on the result seems useful. As you said you would need to add
deliver!
anddeliver_async!
but that's probably fine.