Specific errors subclassing Net::LDAP::Error

[satoryu]

I and my colleagues are looking forward to the release of @DenisKnauf's work #61 but unfortunately there were some conflicts while he was being busy 🏃

so to take over the remaining work, I've recreated new pull request.

[jch]

👍 for switching the name from LdapError to just Error. Does anyone have strong opinions on having a backwards compatibility placeholder?

class LdapError < Error
  def message
    "Deprecation warning: Net::LDAP::LdapError is no longer used. Use Net::LDAP::Error or rescue one of it's subclasses. [link to this file]\n" + super
  end
end

We're pre 1.0, there's no obligation for us to do this. I'm fine with either because it's an easy error to fix if anyone runs into it and we can document it in History.md

[mtodd]

@jch I support the rename and keeping around the backwards compatible constant, though the semantics of the backwards compatible constant is a bit trickier than inheriting from Error. What needs to be backwards compatible is this:

begin
  # something that could fail
rescue Net::LDAP::LdapError => error
  # handle error
end

The problem with the above is that it wouldn't actually catch anything that inherits from Net::LDAP::Error, only the specific Net::LDAP::LdapError subclass. Instead, I think we can just have a constant assignment that would work as expected:

class Net::LDAP
  class Error < StandardError; end
  LdapError = Error
end

begin
rescue Net::LDAP::LdapError
  # should work
end

[jch]

@mtodd good eye. What about having Error inherit from LdapError for a few point releases so that we can add a deprecation warning?

class Net::LDAP
  class LdapError < StandardError
    def message
      "Deprecation warning... " + super
    end
  end

  class Error < LdapError; end
end

This allows us to eventually remove LdapError.

[mtodd]

That would work!

[satoryu]

👍

[jch]

@satoryu nice start on this PR. Could you go over the changes and make sure that you're using the specific error when one is appropriate instead of using the generic Net::LDAP::Error class? Let me know when you have that done, and I can review again.

[mtodd]

👎 we need to continue using SecureRandom for this.

[jch]

Agree with @mtodd. This is also out of scope for this PR.

[mtodd]

It may indicate that your branch was behind upstream master.

[satoryu]

I miss it in merging.

[mtodd]

make sure that you're using the specific error

I think the goal should be that we don't raise Net::LDAP::Error directly, always using the specific error class. Does that sound right, @jch?

[jch]

I think the goal should be that we don't raise Net::LDAP::Error directly, always using the specific error class.

👍 The base error class serves as a convenient catch-all for users to rescue.

[satoryu]

@jch @mtodd I got the goal. I think there are

• use SecureRandom as salt in lib/net/ldap/password.rb
• Define Net::LDAP::LdapError again and it says WARNING message.
• Replace Net::LDAP::Error with appropriate error object

[jch]

@satoryu Remove this dangling srand.

[satoryu]

ops, Rubocop says that Net::LDAP::Connection#search offenses...
I'll try to fix it.

[mtodd]

@satoryu looks like @mynameisrufus is working on a fix here: #185

[satoryu]

👍 good news.

[jch]

I merged this manually after disabling rubocop in CI. We can continue to iterate on #185, but I wanted these changes in the release I'm cutting today (0.10.2).

[satoryu]

@jch thanks!