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

fix ruby warnings and remove obviously unused code #526

Closed
wants to merge 1 commit into from

Conversation

aiomaster
Copy link

  • fixes uninitialized instance variable warnings by initializing them
  • remove methods that can never be called, cause they are defined twice

* fixes uninitialized instance variable warnings by initializing them
* remove methods that can never be called, cause they are defined twice
@@ -303,10 +307,6 @@ def start
@transport.post_initialize_socket
@transport.connect

if @socket_configurator
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why were these lines removed?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I couldn't find any usage of that instance variable. How would you set it? session.set_instance_variable('@socket_configurator', ...) ?
Besides it looks, as if you can just use the configure_socket method of the Session class to reach the same thing. So in my opinion these lines were useless or do you have a usecase?

@@ -31,6 +31,7 @@ class Transport
attr_writer :read_timeout

def initialize(session, host, port, opts)
@socket = nil
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a weird thing to do (in Ruby anyway).

@michaelklishin
Copy link
Member

Thank you for your time but this PR removes some code that is actually used and tries to make perfectly common idioms (such as the use of nil in boolean expressions) work the way they would in a different language.

@aiomaster
Copy link
Author

@michaelklishin Can you please tell me a little bit more what part of that code I removed is actually used? Do you have examples or snippets that don't work anymore after these changes?
What is with these obvious "code smells" I found?
Why has the Channel class for example two to_s methods?
Or why are there two add_default_options class methods in Queue class?
Ruby has no concept of method overloading so it doesn't make any sense to me.

If you think the ruby warnings for uninitialized instance variables are useless consider following example:

@myverycrazyvariableisinitialized = true
unless @myverycrazyvariableisiniailized
  return 'shit misspelled'
end

I think one reason is, to safe you for misspelled class variables, if you take the warnings serious.
I don't know if there is any performance impact negative or positive for this stuff.
But for me it is good to get rid of the warnings.

If you don't like it, ok, but the other warnings that this PR wanted to fix, could be worth to investigate further.
What do you think?

Copy link
Contributor

@carlhoerberg carlhoerberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the changes are reasonable

@@ -47,7 +47,7 @@ def self.open(host, port, options = {})
# @return [String] Data read from the socket
# @api public
def read_fully(count, timeout = nil)
return nil if @__bunny_socket_eof_flag__
return nil if defined?(@__bunny_socket_eof_flag__) && @__bunny_socket_eof_flag__
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think checking if instance variables is defined is needed here

@michaelklishin
Copy link
Member

@aiomaster you are solving a problem that does not exist (in Ruby and dialects of Lisp). Uninitialized ivars are evaluated to nil and nil (and false) evaluate to false.

do_something if value

is a perfectly fine, extremely common idiom in Ruby.

You haven't provided any details on what warnings you are trying to fix which does not help your case.
I'm not interested in this PR as is. Don't waste your time trying to convince me that some hypothetical
misspelling of a class variable might be caught by this. Bunny is an 8 year old library and we've seen plenty of issues but virtually none of them were due to misspellings and none that I can recall were due to misspellings of class variables.

@ruby-amqp ruby-amqp locked and limited conversation to collaborators Oct 30, 2017
@michaelklishin
Copy link
Member

What I am interested in is fixes such as #522 (hard to debug, avoids operational issues, benefits everyone) or even minor improvements such as #524 that solve actual problems. There were code style contributions in the past but fairly objectively Bunny does not have a major code quality problem.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants