search

ruby-ldap / ruby-net-ldap

Pure Ruby LDAP library
https://rubygems.org/gems/net-ldap
Other
399 stars 253 forks source link

Remove deprecated ConnectionRefusedError #366

Closed jurre closed 4 years ago

jurre commented 4 years ago

ConnectionError.new was returning a deprecated ConnectionRefusedError, this pattern was introduced for backward compatibility at some point, but there seems to be no other usage of ConnectionRefusedError needed in the codebase.

If we need to keep the class around for external backward compatibility, I would recommend that we introduce a private-ish way to initialize ConnectionRefusedError without raising the deprecation warning.

I'm happy to PR such a change, but it seems that we could bump the version and get rid of this deprecation in the library.

LdapError was no longer used internally so has been removed in this PR.

This PR is a semi-duplicate of https://github.com/ruby-ldap/ruby-net-ldap/pull/322/files, which I found after making the changes locally, but it's quite old and the author may not have the required context anymore and there are some conflicts, so I decided to open this PR anyway.

jwedoff commented 4 years ago

I've submitted a somewhat more complicated PR to handle this (#368), which preserves compatibility with ConnectionRefusedError, and doesn't raise deprecation warning every time a connection is refused.

jurre commented 4 years ago

Nice one @jwedoff, that looks like a good option if we need to keep the class around 👍

jwedoff commented 4 years ago

I think we need to do something like that; anyone who is catching connection failures. Currently they have to catch ConnectionRefusedError, if we immediately remove it, taking that upgrade breaks their existing code, which seems like a pretty bad thing to do...

jurre commented 4 years ago

The depreciation has been in there for a few years already, if we document it in the changelog the upgrade should be straightforward. I think it should be ok

jurre commented 4 years ago

Hi @HarlemSquirrel, any chance you could take a look at this? Thanks <3

HarlemSquirrel commented 4 years ago

This sounds like a good idea for the next minor version. I've been holding out for someone who can release a new patched version since I don't have that access.

jurre commented 4 years ago

This sounds like a good idea for the next minor version. I've been holding out for someone who can release a new patched version since I don't have that access.

Fantastic, can I help reaching out to someone to get such a release out? I think some of my colleagues are co-maintainers of this gem.

HarlemSquirrel commented 4 years ago

I believe the owners listed in https://rubygems.org/gems/net-ldap are capable of publishing.

cjs commented 4 years ago

@mtodd is this something you can 👀 in a spare moment?