-
Notifications
You must be signed in to change notification settings - Fork 124
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
Allow for port to be configured as a string #163
Conversation
Any thoughts or input on this PR? Just a friendly bump :) |
Nice work. I think it makes sense but we might want to be consistent on all integer parameters. It is confusing to do the conversion that only for the port parameter. Can you apply the change to other integer parameters for the function too? I like to keep the documentation tidy so let's trim Can you also do README change on a separate PR 🙏? And leave |
@ono Thanks for the feedback! I went ahead and added the integer normalize functionality to the other integer options like you suggested and also removed the README changes as I can make those in a separate PR. Let me know if you have any additional comments. Thanks! |
Sorry for my slow response - I have been bit busy this week. Looking good at glance. I will take a closer look this weekend and merge if there is no issue. Thanks! |
Thanks @ono! Have a nice weekend :) |
@akoutmos Merged and released 1.4.1. Thanks! |
This is a quality of life change for configuring connections to RabbitMQ. If the port is provided as a string (which is often the time if the port is configured via an environment variable), a cryptic Erlang authentication error if returned back and it makes it difficult to root cause that the problem was merely a type issue (string vs integer). Attaching the error below when this occurs.
The proposed change is something that is also done in Phoenix for similar reasons: https://github.com/phoenixframework/phoenix/blob/b53813bf3e612077c887f4241a3ff82b41ae9a3f/lib/phoenix/endpoint/cowboy2_adapter.ex#L124-L125
I have also added some README changes to make contributing to
amqp
a bit easier :).