-
Notifications
You must be signed in to change notification settings - Fork 252
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
Conversation
Really like this idea but
|
👍 I dig it. Since the exception classes inherit from a common base class While we're here, how do you guys feel about renaming |
It looks like you discover a little treasure under the heap of PRs ;) +1 for renaming Net::LDAP::LdapError to Net::LDAP::Error |
@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? |
@DenisKnauf any chance you'd like to merge in master and fix conflicts for this? Would really like to get your change merged in! |
At the moment, i haven't Internet, so i can't. |
@DenisKnauf alternatively, you can add me to your fork as collaborators and I can do it for you! :) |
@DenisKnauf any chance you'll have a moment to fix this? Would love to merge it! |
I and my colleagues have been looking forward to this changes on LdapError 👍 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. |
@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! 👍 |
Closing in favor of #183 |
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.