Skip to content

Many specific exceptions instead of one LdapError #61

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

Closed
wants to merge 1 commit into from

Conversation

DenisKnauf
Copy link
Contributor

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.

@RoryO
Copy link
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

@mtodd
Copy link
Member

mtodd commented Oct 6, 2014

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

@jch
Copy link
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.

@schaary
Copy link
Member

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

@jch
Copy link
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?

@mtodd
Copy link
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!

@DenisKnauf
Copy link
Contributor Author

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

@mtodd
Copy link
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 mtodd self-assigned this Oct 15, 2014
@mtodd mtodd mentioned this pull request Oct 22, 2014
@mtodd
Copy link
Member

mtodd commented Dec 10, 2014

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

@satoryu
Copy link
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?

@mtodd
Copy link
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! 👍

@jch
Copy link
Member

jch commented Jan 5, 2015

Closing in favor of #183

@jch jch closed this Jan 5, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants