Skip to content
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

Don't raise when calling deliver/2 or deliver_later/2 #393

Closed
wants to merge 1 commit into from

Conversation

poops
Copy link

@poops poops commented May 30, 2018

See #355

@joshchernoff
Copy link
Contributor

wanna try and resolve the merge?

@poops
Copy link
Author

poops commented Sep 5, 2018

Sorry accidentally closed this with a force push, but should be updated!

@germsvel
Copy link
Collaborator

Hi @poops, thanks for opening this PR! I'm starting to maintain Bamboo, so it's taking a little while to catch up with all the issues/PRs.

I know this is a breaking change, so I want to hold off until we're ready for a v2. Or rather, I just want to make sure to bundle all the breaking changes into one release so we don't have to release a few major versions in a sequence.

So hold off on this for me. I'll loop back to this PR when we're ready.

@germsvel germsvel added the breaking Potentially breaking change label Oct 13, 2020
@germsvel germsvel added this to the 2.0 milestone Oct 13, 2020
@germsvel
Copy link
Collaborator

Hi @poops, thanks for the patience with this PR. I think we can start moving forward with these changes.

Ideally, I'd like to stop raising when we call deliver/2 and deliver_later/2 (what you did in this PR), but I also want to keep the existing behavior under ! versions of those functions.

That way people who don't care about the return value have an easy path for fixing breaking changes: they change deliver -> deliver! and deliver_later -> deliver_later!.

So ideally we have something like this (which I mentioned in a related issue #505 (comment)):

# success deliver_now
{:ok, email} = deliver_now(email)
{:ok, email, response} = deliver_now(email, response: true)

# error deliver_now
{:error, email} = deliver_now(email)
{:error, email, response} = deliver_now(email, response: true)

# success deliver_now!
email = deliver_now!(email)
{email, response} = deliver_now!(email, response: true)

# error deliver_now!
deliver_now!(email) # raises
deliver_now!(email, response: true) #raises

I've also been thinking about releasing a version before 2.0 that basically raises deprecation warnings and allows people to opt-in to the new behavior via configuration before 2.0. I'm trying to be extra careful because I wouldn't want someone to update to 2.0, not change deliver -> deliver! and think everything is working as usual, but their emails are now failing silently because we no longer raise on errors.

I realize those changes are outside of the scope of what you were trying to do here. So, I can go ahead and open a separate PR with all those changes. I can use what you have here as a starting point and set you as a co-author. Or if you want to go through all those layers, I'm happy to review PRs as well.

@poops
Copy link
Author

poops commented Oct 26, 2020

Hi @germsvel

The deprecation warnings sound like a great idea. Since it sounds like you're doing that piece separately, I could update this PR once that's merged. Or if you want to start a new PR feel free to close this. No need to set me as co-author, just happy to have a way to call this w/o raising. 😄

@germsvel
Copy link
Collaborator

@poops I couldn't find a good way to add an in-between release that would do deprecations warnings. But.. I managed to make it so that deliver_now! and deliver_later! have the same behavior that the current deliver_now and deliver_later have. So the upgrade path for those who aren't interested in handling errors would be to simply change:

  • deliver_now -> deliver_now! and
  • deliver_later -> deliver_later!.

I have opened a PR against the Bamboo 2 branch with this work. #571 I'd love your thoughts if you're interested in looking at it.

Also, I think #571 supersedes this PR, so we can probably close it, if that sounds good to you.

@poops
Copy link
Author

poops commented Dec 28, 2020

Looks great, thanks for your work on this!

@poops poops closed this Dec 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Potentially breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants