Skip to content

Conversation

jch
Copy link
Member

@jch jch commented Oct 5, 2014

This PR moves Net::LDAP::Connection from lib/net/ldap.rb to lib/net/ldap/connection.rb. The benefits are that it follows ruby conventions for class and file names, and makes the file easier to scan. For example, it wasn't immediately clear that the two #search methods in the file were for different objects.

@michaeltwofish
Copy link
Contributor

👍 I've been tripped by #search here too.

@schaary
Copy link
Member

schaary commented Oct 5, 2014

@jch - well done, thx! This starts to make the code clearer.

I've discussed with @mtodd in a private chat whether we should unify the test frameworks and finally go for rspec ... do you think you could help us again and switch the test-unit file to an rspec file?

@mtodd
Copy link
Member

mtodd commented Oct 5, 2014

@schaary I don't think changes in this PR really map well to any specific tests, so those changes should be handled in a separate PR for better review.

I'm 👍 to going ahead with this and following up with separate PRs for tests. @schaary 👌?

@jch
Copy link
Member Author

jch commented Oct 5, 2014

👍 for unifying to a single test framework. Personally I prefer test/unit
(minitest) to reduce dependencies and as a preference of style, but I'll
save those comments for @mtodd's PR or issue.

On Sunday, October 5, 2014, Matt Todd notifications@github.com wrote:

@schaary https://github.com/schaary I don't think changes in this PR
really map well to any specific tests, so those changes should be handled
in a separate PR for better review.

I'm [image: 👍] to going ahead with this and following up with separate
PRs for tests. @schaary https://github.com/schaary [image: 👌]?


Reply to this email directly or view it on GitHub
#114 (comment)
.

Jerry Cheung
GitHub https://github.com/jch

@schaary
Copy link
Member

schaary commented Oct 5, 2014

Oh - I am looking forward to @mtodd's PR and the discussion there. It's going to be a agile discussion ;)

@schaary schaary closed this Oct 5, 2014
@mtodd
Copy link
Member

mtodd commented Oct 5, 2014

Whoops, think this was closed on accident? @schaary

@mtodd mtodd reopened this Oct 5, 2014
@schaary
Copy link
Member

schaary commented Oct 6, 2014

No. The extraction of the class is a good move, the discussion about the test framework is not part of this PR. I am fine with it. You? @mtodd

mtodd added a commit that referenced this pull request Oct 6, 2014
Move Net::LDAP::Connection to it's own file
@mtodd mtodd merged commit 7055879 into ruby-ldap:master Oct 6, 2014
astratto pushed a commit to astratto/ruby-net-ldap that referenced this pull request Dec 18, 2015
Move Net::LDAP::Connection to it's own file
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.

4 participants