-
Notifications
You must be signed in to change notification settings - Fork 253
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
Raise Net::LDAP::ConnectionRefusedError when new connection is refused. #213
Conversation
Now Net::LDAP::Connection.new raises Net::LDAP::Error even if the connection refused. It's hard for some application to reconnect it only when refused.
@@ -14,7 +14,7 @@ def initialize(server) | |||
rescue SocketError | |||
raise Net::LDAP::Error, "No such address or other socket error." | |||
rescue Errno::ECONNREFUSED | |||
raise Net::LDAP::Error, "Server #{server[:host]} refused connection on port #{server[:port]}." | |||
raise Net::LDAP::ConnectionRefusedError, "Server #{server[:host]} refused connection on port #{server[:port]}." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally, I prefer not wrapping the error because Errno::ECONNREFUSED
is more descriptive and appropriate. In fact, I'd rather we didn't rescue any wrap any of the errors in this block. However, this would be a breaking change in the API and require changes to the caller. We're not at 1.0, but this does seem like a nasty surprise. How do you feel about making a PR that shows deprecation warnings?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do you feel about making a PR that shows deprecation warnings?
sounds great to me 👍
|
||
private | ||
def warn_deprecation_message | ||
warn "Deprecation warning: Net::LDAP::ConnectionRefused will be deprecated. Use Errno::ECONNREFUSED instead." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When an user use this exception, this will show this exception will be removed. how about this? @jch
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good. Could you add a test that captures stderr and asserts that this message is being printed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @jch ! I did it.
Raise Net::LDAP::ConnectionRefusedError when new connection is refused.
🎉 Awesome, thanks so much! |
=== Net::LDAP 0.12.1 * Whitespace formatting cleanup {#236}[ruby-ldap/ruby-net-ldap#236] * Set operation result if LDAP server is not accessible {#232}[ruby-ldap/ruby-net-ldap#232] === Net::LDAP 0.12.0 * DRY up connection handling logic {#224}[ruby-ldap/ruby-net-ldap#224] * Define auth adapters {#226}[ruby-ldap/ruby-net-ldap#226] * add slash to attribute value filter {#225}[ruby-ldap/ruby-net-ldap#225] * Add the ability to provide a list of hosts for a connection {#223}[ruby-ldap/ruby-net-ldap#223] * Specify the port of LDAP server by giving INTEGRATION_PORT {#221}[ruby-ldap/ruby-net-ldap#221] * Correctly set BerIdentifiedString values to UTF-8 {#212}[ruby-ldap/ruby-net-ldap#212] * Raise Net::LDAP::ConnectionRefusedError when new connection is refused. {#213}[ruby-ldap/ruby-net-ldap#213] * obscure auth password upon #inspect, added test, closes #216 {#217}[ruby-ldap/ruby-net-ldap#217] * Fixing incorrect error class name {#207}[ruby-ldap/ruby-net-ldap#207] * Travis update {#205}[ruby-ldap/ruby-net-ldap#205] * Remove obsolete rbx-19mode from Travis {#204}[ruby-ldap/ruby-net-ldap#204] * mv "sudo" from script/install-openldap to .travis.yml {#199}[ruby-ldap/ruby-net-ldap#199] * Remove meaningless shebang {#200}[ruby-ldap/ruby-net-ldap#200] * Fix Travis CI build {#202}[ruby-ldap/ruby-net-ldap#202] * README.rdoc: fix travis link {#195}[ruby-ldap/ruby-net-ldap#195]
…error Raise Net::LDAP::ConnectionRefusedError when new connection is refused.
Now Net::LDAP::Connection.new raises Net::LDAP::Error even if the
connection refused.
It's hard for some application to reconnect it only when refused.