Address all rubocop lint warnings

[cshaffer]

• Prepend unused parameter names with an underscore
• Prefer “safe assignment in condition”
• Disambiguate regexp literals with parens
• Prefer sprintf over String#%
• No longer shadow caller_line variable in DeprecationLogger
• Rescue StandardError instead of Exception
• Remove useless private access modifier in TestAbstract
• Disambiguate block operator with parens
• Disambiguate between grouped expression and method params
• Remove assertion in TestHost#test_assert_hosts_compare_equal that compares something with itself

[robd]

@cshaffer I just wanted to understand about the reason for the == test deletion. Since equal?, eql? and and == share the same implementation, I think testing all 3 methods is reasonable.

I noticed in the commit you mentioned this was removed due to a rubocop warning that this 'compares something with itself', but I don't understand why comparing 2 instances of an object is undesirable.

[leehambley]

I missed this in the review, good catch @robd - This SO question has a nice summary, and links the Object docs - http://stackoverflow.com/a/7157051/119669 - if we explicitly overwrite == (I'd have to check) then we need the test, if we don't, I don't think we do.

[robd]

Maybe the warning is occurring because == is overwritten as an alias and rubocop can't detect that. In any case, I think we could do with some more tests, especially testing that different hosts aren't equal. Testing different user, hostname, and port combinations would probably be a good idea - my preference would probably be to add these to test_assert_hosts_hash_equally rather than testing 3 times for all 3 equal methods.

[cshaffer]

The rubocop cop for detecting what they call "useless comparison" is pretty naive: https://github.com/bbatsov/rubocop/blob/master/lib/rubocop/cop/lint/useless_comparison.rb

It is only checking if the receiver of #== is the same as the arguments passed in.

I agree that since Object#== is being overwritten by an alias that there should be a test, however I think this is an example where the test is testing too many things (not just because of the 3 asserts, in my opinion it's fine to have all of the asserts in one test when dealing with aliases).

What this is really testing is that the initializer is deterministic given the same input (i.e. subsequent calls to the same initializer multiple times does not have different side effects) but actually more specifically that the implementation of the comparison depends only on the deterministic aspects of the initializer.

My personal style of testing would lead me to use stubs to test only what I'm trying to test, in this case that two hosts are equal when their hashes are equal. I went ahead and opened #276 that adds the test for == back but does it in a way that appeases rubocop and only tests the equality method(s).