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

Upgrade to amqp 1.x #71

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

nsweeting
Copy link

This upgrades the amqp package to the latest version - 1.1.

The migration guide is available here: https://github.com/pma/amqp/wiki/Upgrade-from-0.X-to-1.0

On upgrade, some issues related to lager pop up. These were dealt with following the recommendations here: https://github.com/pma/amqp#troubleshooting--faq

So far - the tests pass. But as mentioned in the migration guide - that may not be enough of a bar to pass. Individuals more familiar with the internals of TaskBunny might be able to provide further insight.

Related to #70

@IanLuites
Copy link
Contributor

Thank you for the PR!

We're going to have a look, but it might take a while, since the pattern matching [might] change current behavior and we noticed some of the code/formatting is really outdated, while we were reviewing.

(Not the code you added, but the code in general, since last time we properly touched it was back on Elixir 1.4)

@nsweeting
Copy link
Author

Sounds good @IanLuites. Thanks for the update. Perhaps we can sort out any behaviour conflicts that might occur from the amqp update, then create a new PR to run the formatter on the codebase?

@christianjgreen
Copy link

Should this be adding pattern matching? I feel like it's best to pass the result through rather than raising a MatchError.

This is also the behavior that is currently in 0.3.1 https://github.com/pma/amqp/blob/c1148673a8a909b3b4412c77aba7ff252c7576e8/lib/amqp/basic.ex#L314

@nsweeting
Copy link
Author

From what I understand, this PR just matches existing behaviour. The older version of AMQP would simply raise a MatchError itself. Whereas here - it will be raised within TaskBunny. Obviously - there are more elegant solutions to handling the errors. I just wanted to focus on the upgrade while maintaining existing behaviour first.

indrekj added a commit to salemove/task_bunny that referenced this pull request Nov 10, 2021
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.

3 participants