Many specific exceptions instead of one LdapError #61

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
6 participants
Contributor

DenisKnauf commented Mar 16, 2013

We need to rescue some exceptions, but because there is only one LdapError-class, we cannot rescue one specific behavior.
So i wrote this patch for using many LdapError-classes but specific exceptions.
All of these exceptions has LdapError as base-class, so it do not break any exception handling.

Contributor

RoryO commented Jul 23, 2013

Really like this idea but

  1. This makes it unable to merge cleanly
  2. Specs should be updated to use new exception classes instead of always looking for LdapError classes
Member

mtodd commented Oct 6, 2014

@jch @schaary what are your thoughts on this PR?

Member

jch commented Oct 6, 2014

👍 I dig it. Since the exception classes inherit from a common base class Net::LDAP::LdapError, it gives the caller the option to either rescue for a specific error, or to rescue the general one.

While we're here, how do you guys feel about renaming Net::LDAP::LdapError to Net::LDAP::Error? That capitalization bugs me. We can easily add a deprecation notice and alias the old constant for backwards compatibility until the next major version release.

Owner

schaary commented Oct 6, 2014

It looks like you discover a little treasure under the heap of PRs ;)

+1 for renaming Net::LDAP::LdapError to Net::LDAP::Error

Member

jch commented Oct 7, 2014

@DenisKnauf 👍 for proposing this PR. Sorry it took us so long to get to it, but would you be interested in updating it to merge cleanly with master?

Member

mtodd commented Oct 12, 2014

@DenisKnauf any chance you'd like to merge in master and fix conflicts for this? Would really like to get your change merged in!

Contributor

DenisKnauf commented Oct 13, 2014

At the moment, i haven't Internet, so i can't.
Next month it should be possible.

Member

mtodd commented Oct 15, 2014

@DenisKnauf alternatively, you can add me to your fork as collaborators and I can do it for you! :)

mtodd self-assigned this Oct 15, 2014

mtodd referenced this pull request Oct 22, 2014

Closed

Release 0.9.0 #125

Member

mtodd commented Dec 10, 2014

@DenisKnauf any chance you'll have a moment to fix this? Would love to merge it!

Collaborator

satoryu commented Jan 3, 2015

I and my colleagues have been looking forward to this changes on LdapError 👍
Unfortunately @DenisKnauf seems be not able to have a time to fix the conflict.

I have an idea to take over his work, creating new pull request base on a branch into which @DenisKnauf 's master are merged with the current HEAD of this repo's master, of course resolving the conflict.

@mtodd @jch how do you think the idea?

Member

mtodd commented Jan 3, 2015

@satoryu like @DenisKnauf, I too have not had time to fix this. I'd love for you to take this over and get it to a point where we can merge it! 👍

Member

jch commented Jan 5, 2015

Closing in favor of #183

jch closed this Jan 5, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment