Skip to content
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

Config connection, socket classes #135

Closed
wants to merge 3 commits into from

Conversation

mtodd
Copy link
Member

@mtodd mtodd commented Oct 16, 2014

This makes the connection class (by default Net::LDAP::Connection) and the socket class (by default TCPSocket) configurable, and extracts the repetitive "new connection" stanza into a new_connection method.

I don't foresee people replacing either object with something entirely different, but this opens up new opportunities for integrators to wrap the Net::LDAP::Connection or the TCPSocket classes with a decorator or anything without having to monkeypatch the class.

Need to document, test the behavior, but wanted to open up the idea for discussion first.

cc @jch @schaary

mtodd added 3 commits October 15, 2014 23:39
Override to customize socket class/object behavior.
Override to customize connection handling.
@jch
Copy link
Member

jch commented Oct 16, 2014

I like the DRY-er new_connection method, but think we should hold off on parameterizing the connection and socket classes until we have a real use case for it. Do you have something in mind for it already? Maybe easier testing?

Parameterizing the socket class is probably the first step for supporting LDAPI (Unix domain sockets) requested in #14, but I'd still like to see that as its own PR/ discussion.

@mtodd
Copy link
Member Author

mtodd commented Oct 16, 2014

@jch primarily I'm thinking about wiring up connection pooling and want to have that be a drop-in replacement for the existing connection class. There's more work to do on that, though, since new_connection still doesn't quite fit the connection pool model.

I think it might make more sense to be able to set the connection class at the instance level, and expose a Connection.open method (which the connection pool would use to return an instance from the pool, with Connection.close maybe to check a connection back into the pool).

Gonna close this for now and take another swing at more focused PRs. I don't think we should try to ship a connection pooling class with the library, though, so there might be some disconnect between the changes and what they can be used for.

@mtodd mtodd closed this Oct 16, 2014
@jch jch deleted the config-connection-socket-classes branch October 16, 2014 17:47
@jch
Copy link
Member

jch commented Oct 16, 2014

🆒 I dig your reasoning. Whatever you end up deciding, I think it'd be good to include a bit of documentation to explain what the interface for the Connection class replacement is, and why that can be useful.

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.

2 participants