{"action":"created","comment":{"url":"https://api.github.com/repos/ruby-ldap/ruby-net-ldap/pulls/comments/40969717","id":40969717,"diff_hunk":"@@ -8,56 +8,49 @@ class Net::LDAP::Connection #:nodoc:\n \n   def initialize(server)\n     @instrumentation_service = server[:instrumentation_service]\n-    server[:hosts] = [[server[:host], server[:port]]] if server[:hosts].nil?\n \n     if server[:socket]\n-      prepare_socket(server)\n+      prepare_socket(server[:socket], server[:encryption])\n     else\n-      open_connection(server)\n+      hosts = server[:hosts]\n+      hosts = [[server[:host], server[:port]]] if hosts.nil?\n+      open_connection(hosts, server[:encryption])","path":"lib/net/ldap/connection.rb","position":13,"original_position":13,"commit_id":"085a6220bde9a52c6a9404d98d9ae95bb90ae61c","original_commit_id":"085a6220bde9a52c6a9404d98d9ae95bb90ae61c","user":{"login":"javanthropus","id":226985,"avatar_url":"https://avatars.githubusercontent.com/u/226985?v=3","gravatar_id":"","url":"https://api.github.com/users/javanthropus","html_url":"https://github.com/javanthropus","followers_url":"https://api.github.com/users/javanthropus/followers","following_url":"https://api.github.com/users/javanthropus/following{/other_user}","gists_url":"https://api.github.com/users/javanthropus/gists{/gist_id}","starred_url":"https://api.github.com/users/javanthropus/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/javanthropus/subscriptions","organizations_url":"https://api.github.com/users/javanthropus/orgs","repos_url":"https://api.github.com/users/javanthropus/repos","events_url":"https://api.github.com/users/javanthropus/events{/privacy}","received_events_url":"https://api.github.com/users/javanthropus/received_events","type":"User","site_admin":false},"body":"After looking into this, I think it won't work out.  Most of what `open_connection` does in this patch is manage exception dispatching, and it needs to do this because in the multi-host case it must suppress any exceptions raised if a connection is successfully opened.  Opening the connection is actually handled by `prepare_socket` and `TCPSocket.new`, one line in the `open_connection` method.  The rest of the method is the looping over hosts to try, gathering of exceptions, and finally raising an exception based on the captured exceptions if no connection was opened.\r\n\r\nIn other words, to do what you suggest moves about 19 lines from `open_connection` into `initialize`, leaving `open_connection` as a one-liner and `initialize` rather more bloated.  I don't really see how this would address the exception handling code smell either since we're just moving logic around rather than eliminating or refactoring it significantly.\r\n\r\nHave you looked at the result of the patch rather than the diffs yet?  I think you'll see that the patch is noisy only because it's simplifying the code.","created_at":"2015-10-01T21:38:58Z","updated_at":"2015-10-01T21:38:58Z","html_url":"https://github.com/ruby-ldap/ruby-net-ldap/pull/224#discussion_r40969717","pull_request_url":"https://api.github.com/repos/ruby-ldap/ruby-net-ldap/pulls/224","_links":{"self":{"href":"https://api.github.com/repos/ruby-ldap/ruby-net-ldap/pulls/comments/40969717"},"html":{"href":"https://github.com/ruby-ldap/ruby-net-ldap/pull/224#discussion_r40969717"},"pull_request":{"href":"https://api.github.com/repos/ruby-ldap/ruby-net-ldap/pulls/224"}}},"pull_request":{"url":"https://api.github.com/repos/ruby-ldap/ruby-net-ldap/pulls/224","id":46232021,"html_url":"https://github.com/ruby-ldap/ruby-net-ldap/pull/224","diff_url":"https://github.com/ruby-ldap/ruby-net-ldap/pull/224.diff","patch_url":"https://github.com/ruby-ldap/ruby-net-ldap/pull/224.patch","issue_url":"https://api.github.com/repos/ruby-ldap/ruby-net-ldap/issues/224","number":224,"state":"open","locked":false,"title":"DRY up connection handling logic","user":{"login":"javanthropus","id":226985,"avatar_url":"https://avatars.githubusercontent.com/u/226985?v=3","gravatar_id":"","url":"https://api.github.com/users/javanthropus","html_url":"https://github.com/javanthropus","followers_url":"https://api.github.com/users/javanthropus/followers","following_url":"https://api.github.com/users/javanthropus/following{/other_user}","gists_url":"https://api.github.com/users/javanthropus/gists{/gist_id}","starred_url":"https://api.github.com/users/javanthropus/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/javanthropus/subscriptions","organizations_url":"https://api.github.com/users/javanthropus/orgs","repos_url":"https://api.github.com/users/javanthropus/repos","events_url":"https://api.github.com/users/javanthropus/events{/privacy}","received_events_url":"https://api.github.com/users/javanthropus/received_events","type":"User","site_admin":false},"body":"This patch simplifies configuration of new connections and removes some duplicate code around setting up encryption.  However, it does change the exception messages somewhat.  All the same exception types are raised in all the same places, but mostly the messages of underlying exceptions that `Net::LDAP::Error` and `Net::LDAP::ConnectionRefusedError` effectively wrap are used directly rather than providing new, different messages.  These changes don't break any tests, but it's possible that some user of the library may depend on the specific messages currently produced.\r\n\r\nThe upshot of this change of exception messages will mostly be felt when users provide host lists.  If all given hosts fail, each failure message for each host is included in an exception summarizing all of the failures along with the original exception type and host parameters that triggered the failure message.  This will certainly help when debugging failures in the multiple host case.","created_at":"2015-09-29T03:01:08Z","updated_at":"2015-10-01T21:38:58Z","closed_at":null,"merged_at":null,"merge_commit_sha":"ebd1092d64b83c85e73bd4bee48f823b66a0c521","assignee":null,"milestone":null,"commits_url":"https://api.github.com/repos/ruby-ldap/ruby-net-ldap/pulls/224/commits","review_comments_url":"https://api.github.com/repos/ruby-ldap/ruby-net-ldap/pulls/224/comments","review_comment_url":"https://api.github.com/repos/ruby-ldap/ruby-net-ldap/pulls/comments{/number}","comments_url":"https://api.github.com/repos/ruby-ldap/ruby-net-ldap/issues/224/comments","statuses_url":"https://api.github.com/repos/ruby-ldap/ruby-net-ldap/statuses/085a6220bde9a52c6a9404d98d9ae95bb90ae61c","head":{"label":"javanthropus:dry_up_connection_handling","ref":"dry_up_connection_handling","sha":"085a6220bde9a52c6a9404d98d9ae95bb90ae61c","user":{"login":"javanthropus","id":226985,"avatar_url":"https://avatars.githubusercontent.com/u/226985?v=3","gravatar_id":"","url":"https://api.github.com/users/javanthropus","html_url":"https://github.com/javanthropus","followers_url":"https://api.github.com/users/javanthropus/followers","following_url":"https://api.github.com/users/javanthropus/following{/other_user}","gists_url":"https://api.github.com/users/javanthropus/gists{/gist_id}","starred_url":"https://api.github.com/users/javanthropus/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/javanthropus/subscriptions","organizations_url":"https://api.github.com/users/javanthropus/orgs","repos_url":"https://api.github.com/users/javanthropus/repos","events_url":"https://api.github.com/users/javanthropus/events{/privacy}","received_events_url":"https://api.github.com/users/javanthropus/received_events","type":"User","site_admin":false},"repo":{"id":43271171,"name":"ruby-net-ldap","full_name":"javanthropus/ruby-net-ldap","owner":{"login":"javanthropus","id":226985,"avatar_url":"https://avatars.githubusercontent.com/u/226985?v=3","gravatar_id":"","url":"https://api.github.com/users/javanthropus","html_url":"https://github.com/javanthropus","followers_url":"https://api.github.com/users/javanthropus/followers","following_url":"https://api.github.com/users/javanthropus/following{/other_user}","gists_url":"https://api.github.com/users/javanthropus/gists{/gist_id}","starred_url":"https://api.github.com/users/javanthropus/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/javanthropus/subscriptions","organizations_url":"https://api.github.com/users/javanthropus/orgs","repos_url":"https://api.github.com/users/javanthropus/repos","events_url":"https://api.github.com/users/javanthropus/events{/privacy}","received_events_url":"https://api.github.com/users/javanthropus/received_events","type":"User","site_admin":false},"private":false,"html_url":"https://github.com/javanthropus/ruby-net-ldap","description":"Pure Ruby LDAP library ","fork":true,"url":"https://api.github.com/repos/javanthropus/ruby-net-ldap","forks_url":"https://api.github.com/repos/javanthropus/ruby-net-ldap/forks","keys_url":"https://api.github.com/repos/javanthropus/ruby-net-ldap/keys{/key_id}","collaborators_url":"https://api.github.com/repos/javanthropus/ruby-net-ldap/collaborators{/collaborator}","teams_url":"https://api.github.com/repos/javanthropus/ruby-net-ldap/teams","hooks_url":"https://api.github.com/repos/javanthropus/ruby-net-ldap/hooks","issue_events_url":"https://api.github.com/repos/javanthropus/ruby-net-ldap/issues/events{/number}","events_url":"https://api.github.com/repos/javanthropus/ruby-net-ldap/events","assignees_url":"https://api.github.com/repos/javanthropus/ruby-net-ldap/assignees{/user}","branches_url":"https://api.github.com/repos/javanthropus/ruby-net-ldap/branches{/branch}","tags_url":"https://api.github.com/repos/javanthropus/ruby-net-ldap/tags","blobs_url":"https://api.github.com/repos/javanthropus/ruby-net-ldap/git/blobs{/sha}","git_tags_url":"https://api.github.com/repos/javanthropus/ruby-net-ldap/git/tags{/sha}","git_refs_url":"https://api.github.com/repos/javanthropus/ruby-net-ldap/git/refs{/sha}","trees_url":"https://api.github.com/repos/javanthropus/ruby-net-ldap/git/trees{/sha}","statuses_url":"https://api.github.com/repos/javanthropus/ruby-net-ldap/statuses/{sha}","languages_url":"https://api.github.com/repos/javanthropus/ruby-net-ldap/languages","stargazers_url":"https://api.github.com/repos/javanthropus/ruby-net-ldap/stargazers","contributors_url":"https://api.github.com/repos/javanthropus/ruby-net-ldap/contributors","subscribers_url":"https://api.github.com/repos/javanthropus/ruby-net-ldap/subscribers","subscription_url":"https://api.github.com/repos/javanthropus/ruby-net-ldap/subscription","commits_url":"https://api.github.com/repos/javanthropus/ruby-net-ldap/commits{/sha}","git_commits_url":"https://api.github.com/repos/javanthropus/ruby-net-ldap/git/commits{/sha}","comments_url":"https://api.github.com/repos/javanthropus/ruby-net-ldap/comments{/number}","issue_comment_url":"https://api.github.com/repos/javanthropus/ruby-net-ldap/issues/comments{/number}","contents_url":"https://api.github.com/repos/javanthropus/ruby-net-ldap/contents/{+path}","compare_url":"https://api.github.com/repos/javanthropus/ruby-net-ldap/compare/{base}...{head}","merges_url":"https://api.github.com/repos/javanthropus/ruby-net-ldap/merges","archive_url":"https://api.github.com/repos/javanthropus/ruby-net-ldap/{archive_format}{/ref}","downloads_url":"https://api.github.com/repos/javanthropus/ruby-net-ldap/downloads","issues_url":"https://api.github.com/repos/javanthropus/ruby-net-ldap/issues{/number}","pulls_url":"https://api.github.com/repos/javanthropus/ruby-net-ldap/pulls{/number}","milestones_url":"https://api.github.com/repos/javanthropus/ruby-net-ldap/milestones{/number}","notifications_url":"https://api.github.com/repos/javanthropus/ruby-net-ldap/notifications{?since,all,participating}","labels_url":"https://api.github.com/repos/javanthropus/ruby-net-ldap/labels{/name}","releases_url":"https://api.github.com/repos/javanthropus/ruby-net-ldap/releases{/id}","created_at":"2015-09-28T00:17:40Z","updated_at":"2015-09-28T00:17:41Z","pushed_at":"2015-09-29T02:46:48Z","git_url":"git://github.com/javanthropus/ruby-net-ldap.git","ssh_url":"git@github.com:javanthropus/ruby-net-ldap.git","clone_url":"https://github.com/javanthropus/ruby-net-ldap.git","svn_url":"https://github.com/javanthropus/ruby-net-ldap","homepage":"https://github.com/ruby-ldap/ruby-net-ldap","size":1229,"stargazers_count":0,"watchers_count":0,"language":"Ruby","has_issues":false,"has_downloads":true,"has_wiki":true,"has_pages":false,"forks_count":0,"mirror_url":null,"open_issues_count":0,"forks":0,"open_issues":0,"watchers":0,"default_branch":"master"}},"base":{"label":"ruby-ldap:master","ref":"master","sha":"5673f83cf755a625ee0a7911cf8acc5243918d99","user":{"login":"ruby-ldap","id":676122,"avatar_url":"https://avatars.githubusercontent.com/u/676122?v=3","gravatar_id":"","url":"https://api.github.com/users/ruby-ldap","html_url":"https://github.com/ruby-ldap","followers_url":"https://api.github.com/users/ruby-ldap/followers","following_url":"https://api.github.com/users/ruby-ldap/following{/other_user}","gists_url":"https://api.github.com/users/ruby-ldap/gists{/gist_id}","starred_url":"https://api.github.com/users/ruby-ldap/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/ruby-ldap/subscriptions","organizations_url":"https://api.github.com/users/ruby-ldap/orgs","repos_url":"https://api.github.com/users/ruby-ldap/repos","events_url":"https://api.github.com/users/ruby-ldap/events{/privacy}","received_events_url":"https://api.github.com/users/ruby-ldap/received_events","type":"Organization","site_admin":false},"repo":{"id":1509861,"name":"ruby-net-ldap","full_name":"ruby-ldap/ruby-net-ldap","owner":{"login":"ruby-ldap","id":676122,"avatar_url":"https://avatars.githubusercontent.com/u/676122?v=3","gravatar_id":"","url":"https://api.github.com/users/ruby-ldap","html_url":"https://github.com/ruby-ldap","followers_url":"https://api.github.com/users/ruby-ldap/followers","following_url":"https://api.github.com/users/ruby-ldap/following{/other_user}","gists_url":"https://api.github.com/users/ruby-ldap/gists{/gist_id}","starred_url":"https://api.github.com/users/ruby-ldap/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/ruby-ldap/subscriptions","organizations_url":"https://api.github.com/users/ruby-ldap/orgs","repos_url":"https://api.github.com/users/ruby-ldap/repos","events_url":"https://api.github.com/users/ruby-ldap/events{/privacy}","received_events_url":"https://api.github.com/users/ruby-ldap/received_events","type":"Organization","site_admin":false},"private":false,"html_url":"https://github.com/ruby-ldap/ruby-net-ldap","description":"Pure Ruby LDAP library ","fork":false,"url":"https://api.github.com/repos/ruby-ldap/ruby-net-ldap","forks_url":"https://api.github.com/repos/ruby-ldap/ruby-net-ldap/forks","keys_url":"https://api.github.com/repos/ruby-ldap/ruby-net-ldap/keys{/key_id}","collaborators_url":"https://api.github.com/repos/ruby-ldap/ruby-net-ldap/collaborators{/collaborator}","teams_url":"https://api.github.com/repos/ruby-ldap/ruby-net-ldap/teams","hooks_url":"https://api.github.com/repos/ruby-ldap/ruby-net-ldap/hooks","issue_events_url":"https://api.github.com/repos/ruby-ldap/ruby-net-ldap/issues/events{/number}","events_url":"https://api.github.com/repos/ruby-ldap/ruby-net-ldap/events","assignees_url":"https://api.github.com/repos/ruby-ldap/ruby-net-ldap/assignees{/user}","branches_url":"https://api.github.com/repos/ruby-ldap/ruby-net-ldap/branches{/branch}","tags_url":"https://api.github.com/repos/ruby-ldap/ruby-net-ldap/tags","blobs_url":"https://api.github.com/repos/ruby-ldap/ruby-net-ldap/git/blobs{/sha}","git_tags_url":"https://api.github.com/repos/ruby-ldap/ruby-net-ldap/git/tags{/sha}","git_refs_url":"https://api.github.com/repos/ruby-ldap/ruby-net-ldap/git/refs{/sha}","trees_url":"https://api.github.com/repos/ruby-ldap/ruby-net-ldap/git/trees{/sha}","statuses_url":"https://api.github.com/repos/ruby-ldap/ruby-net-ldap/statuses/{sha}","languages_url":"https://api.github.com/repos/ruby-ldap/ruby-net-ldap/languages","stargazers_url":"https://api.github.com/repos/ruby-ldap/ruby-net-ldap/stargazers","contributors_url":"https://api.github.com/repos/ruby-ldap/ruby-net-ldap/contributors","subscribers_url":"https://api.github.com/repos/ruby-ldap/ruby-net-ldap/subscribers","subscription_url":"https://api.github.com/repos/ruby-ldap/ruby-net-ldap/subscription","commits_url":"https://api.github.com/repos/ruby-ldap/ruby-net-ldap/commits{/sha}","git_commits_url":"https://api.github.com/repos/ruby-ldap/ruby-net-ldap/git/commits{/sha}","comments_url":"https://api.github.com/repos/ruby-ldap/ruby-net-ldap/comments{/number}","issue_comment_url":"https://api.github.com/repos/ruby-ldap/ruby-net-ldap/issues/comments{/number}","contents_url":"https://api.github.com/repos/ruby-ldap/ruby-net-ldap/contents/{+path}","compare_url":"https://api.github.com/repos/ruby-ldap/ruby-net-ldap/compare/{base}...{head}","merges_url":"https://api.github.com/repos/ruby-ldap/ruby-net-ldap/merges","archive_url":"https://api.github.com/repos/ruby-ldap/ruby-net-ldap/{archive_format}{/ref}","downloads_url":"https://api.github.com/repos/ruby-ldap/ruby-net-ldap/downloads","issues_url":"https://api.github.com/repos/ruby-ldap/ruby-net-ldap/issues{/number}","pulls_url":"https://api.github.com/repos/ruby-ldap/ruby-net-ldap/pulls{/number}","milestones_url":"https://api.github.com/repos/ruby-ldap/ruby-net-ldap/milestones{/number}","notifications_url":"https://api.github.com/repos/ruby-ldap/ruby-net-ldap/notifications{?since,all,participating}","labels_url":"https://api.github.com/repos/ruby-ldap/ruby-net-ldap/labels{/name}","releases_url":"https://api.github.com/repos/ruby-ldap/ruby-net-ldap/releases{/id}","created_at":"2011-03-22T03:52:14Z","updated_at":"2015-09-28T17:08:25Z","pushed_at":"2015-09-29T16:59:41Z","git_url":"git://github.com/ruby-ldap/ruby-net-ldap.git","ssh_url":"git@github.com:ruby-ldap/ruby-net-ldap.git","clone_url":"https://github.com/ruby-ldap/ruby-net-ldap.git","svn_url":"https://github.com/ruby-ldap/ruby-net-ldap","homepage":"https://github.com/ruby-ldap/ruby-net-ldap","size":2352,"stargazers_count":235,"watchers_count":235,"language":"Ruby","has_issues":true,"has_downloads":true,"has_wiki":true,"has_pages":false,"forks_count":164,"mirror_url":null,"open_issues_count":33,"forks":164,"open_issues":33,"watchers":235,"default_branch":"master"}},"_links":{"self":{"href":"https://api.github.com/repos/ruby-ldap/ruby-net-ldap/pulls/224"},"html":{"href":"https://github.com/ruby-ldap/ruby-net-ldap/pull/224"},"issue":{"href":"https://api.github.com/repos/ruby-ldap/ruby-net-ldap/issues/224"},"comments":{"href":"https://api.github.com/repos/ruby-ldap/ruby-net-ldap/issues/224/comments"},"review_comments":{"href":"https://api.github.com/repos/ruby-ldap/ruby-net-ldap/pulls/224/comments"},"review_comment":{"href":"https://api.github.com/repos/ruby-ldap/ruby-net-ldap/pulls/comments{/number}"},"commits":{"href":"https://api.github.com/repos/ruby-ldap/ruby-net-ldap/pulls/224/commits"},"statuses":{"href":"https://api.github.com/repos/ruby-ldap/ruby-net-ldap/statuses/085a6220bde9a52c6a9404d98d9ae95bb90ae61c"}}}}