-
Notifications
You must be signed in to change notification settings - Fork 345
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
Update SendGrid Adapter to return ok/error tuple #572
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
3 tasks
germsvel
added
adapter
Related to supported adapters
breaking
Potentially breaking change
labels
Dec 28, 2020
Circle is failing on
Otherwise changes lgtm |
Thanks @lady3bean. The build is failing because this branch depends on #571 which introduced the |
This was referenced Feb 18, 2021
What changed? ============== We update the `SendGridAdapter` to abide by the new Adapter behaviour. The adapter now returns an `{:ok, email}` or `{:error, error}`. As part of this work, we create the `build_api_error` function to replace the `raise_api_error` function. Note on implementation with throw/catch --------------------------------------- Building SendGrid's body requires a lot of deeply nested data manipulation. Nested deep within some of the branching logic was some code that raised errors. Rather than returning an error tuple at a really low level and have to bubble up the error all the way til it's returned, we opted for a simpler approach: throwing and catching the errors. We throw and catch `{:error, error}` when we used to `raise error`. It's possible we could do something different in the future, like bubbling up the errors and returning them without having to resort to `throw` and `catch`, but right now we want to minimize the number of changes for these `raise`s deep in the call stack. Raising (not returning) configuration errors -------------------------------- While working on this, an important question came up. Should we raise or return an error because of a missing API key? Currently we raise. I think it's okay to raise an error in that case because it's a configuration issue. It's not the same type of error that happens when you can't build an email, deliver it, or even get a response that isn't 200. That error is a configuration issue and so the sooner we notify the user of Bamboo, the better. We can also think about it this way: we want to return an ok/error tuple because we people want to handle email building or delivery issues. Having an API key missing isn't that kind of issue. It means the library is configured incorrectly, so it seems good to raise an error.
germsvel
force-pushed
the
gv-sendgrid-new-deliver
branch
from
February 19, 2021 14:27
7fb5fe2
to
d5acd13
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Requires: #571
What changed?
We update the
SendGridAdapter
to abide by the new Adapter behaviour. Adapters must now returns an{:ok, email}
or{:error, error}
.Note on implementation with throw/catch
Building SendGrid's body requires a lot of deeply nested data manipulation. Some code that is deeply nested within branching logic raises errors.
Rather than returning an error tuple at a really low level and have to bubble up the error all the way until it's returned, we opted for a simpler approach: throwing and catching the errors.
We throw and catch
{:error, error}
when we used toraise error
.It's possible we could do something different in the future, like bubbling up the errors and returning them without having to resort to
throw
andcatch
, but right now we want to minimize the number of changes for theseraise
s deep in the call stack.Raising (not returning) configuration errors
While working on this, an important question came up. Should we raise or return an error because of a missing API key? Currently we raise.
I think it's okay to raise an error in that case because it's a configuration issue. It's not the same type of error that happens when you can't build an email, deliver it, or even get a response that isn't 200. That error is a configuration issue and so the sooner we notify the user of Bamboo, the better.
We can also think about it this way: we want to return an ok/error tuple because we people want to handle email building or delivery issues. Having an API key missing isn't that kind of issue. It means the library is configured incorrectly, so it seems good to raise an error.