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

Support different email libraries #74

Merged
merged 5 commits into from
Jan 18, 2022

Conversation

rktjmp
Copy link
Contributor

@rktjmp rktjmp commented Jan 5, 2022

rebase from master, See also: #71 #70 #61.

It's been a moment since I was looking at this, just reopening to confirm it's not dead on my end.

Remaining tasks

  • Test configuration with a "live" system in case the tests are all lying?

@iaguirre88
Copy link
Member

iaguirre88 commented Jan 12, 2022

@rktjmp Thanks for your patience ❤️. I know it's been a lot of back and forth but I think it's finally ready to be merged once the dialyzer issues are fixed and the conflict with master is resolved

Let me know if there's something I can help with 🙌

BTW: you can run mix quality to run mix format && mix credo && mix dialyzer.

@rktjmp rktjmp force-pushed the support-different-email-libraries branch from c7f2f7e to 0fd8df2 Compare January 18, 2022 14:47
@rktjmp
Copy link
Contributor Author

rktjmp commented Jan 18, 2022

Think its all good to go. I had to adjust the some dialyzer settings so it loaded the optional dependencies.

|> text_body(TextContent.build(error_info))

options[:mailer].deliver_later!(email)
if Code.ensure_loaded?(Bamboo) do
Copy link
Contributor Author

@rktjmp rktjmp Jan 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://elixirforum.com/t/is-there-a-guide-for-relying-on-optional-dependencies-in-a-library/37318

Seems to be the prescribed method, or at least, "not illegal". Without these checks downstream users get compiler errors when building boom.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I agree this is the way to do it (or at least I can't come up with a better way to do it)

Copy link
Member

@iaguirre88 iaguirre88 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is awesome! Thanks for your contribution and the hard work you've put on it ❤️

I'll try to make a release with this change this week 🚀

@iaguirre88 iaguirre88 merged commit 8232c8c into wyeworks:master Jan 18, 2022
@iaguirre88
Copy link
Member

Released in v0.7.0
https://hex.pm/packages/boom_notifier/0.7.0

@rktjmp
Copy link
Contributor Author

rktjmp commented Feb 1, 2022

🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants