-
Notifications
You must be signed in to change notification settings - Fork 253
refactor connection establishment #180
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
lib/net/ldap.rb
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs documentation.
What are your thoughts on accepting the full args here instead of just the auth hash? While we're only using the auth portion, it seems overly specific for the scope of the method (connection management) and could evolve to need all of the connection options.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
documented, and switched to passing the full args hash
2b259db to
8cbbfc0
Compare
lib/net/ldap.rb
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to pull @result out of this method. It would be fine to have a local result that gets passed back to the caller.
0 is a strange default, too.
reduces duplicated code, and makes it easier for future changes to connection establishment because it's all in one place now
8cbbfc0 to
443fdb1
Compare
|
new version without assigning to @Result directly in use_connection is pushed |
|
@ccutrer rebasing/force-pushing changes isn't necessary, though it's fine if that's your preference. RE: using |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was thinking it'd be good to document that the result of the block is passed through, since that's relevant to the @result = assignment we do in the caller. Not a blocker but would be good to communicate that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Derp, didn't read well enough, that's exactly what's documented here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method feels very similar to Net::LDAP#open.
|
Think this is good to go. My comments aren't blockers, and this sets us up to iterate more ably than we were before so I think holding off would be pointless. |
refactor connection establishment
refactor connection establishment
reduces duplicated code, and makes it easier for future changes
to connection establishment because it's all in one place now