Remove more warnings #905

Merged
merged 31 commits into from May 28, 2012

Projects

None yet

4 participants

@jherdman

This branch is not done. I'm simply making it available to show some visibility into some work I'm doing on removing warnings in Fog, especially since this work touches so many files.

The biggest remaining problem is a circular dependency issue mentioned in issue #904.

Would someone mind looking over what's done so far to see if anything is out of sorts?

@freeformz freeformz commented on the diff May 14, 2012
lib/fog/libvirt/compute.rb
@@ -58,7 +58,10 @@ def initialize(options={})
end
private
- attr_reader :client
+
+ def client
+ return @client if defined?(@client)
@freeformz
freeformz May 14, 2012

What's wrong with attr_reader here?

or at least just

def client
  @client
end
@jherdman
jherdman May 14, 2012

Ruby will complain about the use of attr_reader. The problem with your solution is that Ruby will complain about @client being uninitialized. Here are some examples from IRB:

irb(main):001:0> class Foo
irb(main):002:1>   private
irb(main):003:1>   attr_reader :client
irb(main):004:1>   end
(irb):3: warning: private attribute?

And...

irb(main):001:0> class Foo
irb(main):002:1>   private
irb(main):003:1>   def client
irb(main):004:2>     @client
irb(main):005:2>     end
irb(main):006:1>   end
=> nil
irb(main):007:0> f = Foo.new
=> #<Foo:0x007fbaa988b970>
irb(main):008:0> f.send :client
(irb):4: warning: instance variable @client not initialized

Proof of my solution...

irb(main):001:0> class Foo
irb(main):002:1>   private
irb(main):003:1>   def client 
irb(main):004:2>     return @client if defined?(@client)
irb(main):005:2>     end
irb(main):006:1>   end
=> nil
irb(main):007:0> f = Foo.new
=> #<Foo:0x007f99fb851000>
irb(main):008:0> f.send :client
=> nil
@freeformz
freeformz May 14, 2012

Ahh yes ... -w ;-)

@geemus
fog member

Looks good so far, let me know when you are ready for a final review/merge.

@jherdman

@geemus as far as I can tell I've removed all warnings except the following:

/Users/james/.rvm/rubies/ruby-1.9.3-p194/lib/ruby/1.9.1/forwardable.rb:199: warning: method redefined; discarding old close
/Users/james/.rvm/rubies/ruby-1.9.3-p194/lib/ruby/1.9.1/forwardable.rb:199: warning: previous definition of close was here
/Users/james/.rvm/rubies/ruby-1.9.3-p194/lib/ruby/1.9.1/forwardable.rb:199: warning: method redefined; discarding old readline
/Users/james/.rvm/rubies/ruby-1.9.3-p194/lib/ruby/1.9.1/forwardable.rb:199: warning: previous definition of readline was here

These appear to be within Ruby itself.

Can you check over the circular require changes? It touches a lot of files.

@geemus
fog member

I think there might be problems with the circular reference removal as it stands. In particular, the reason a lot of that is in there is from how lib/fog/compute and the like do their requires.

ie: https://github.com/fog/fog/blob/master/lib/fog/compute.rb#L12

Due to the nature of the beast I think this is fixable, that line (and those like it) could probably be changed to require the aws file directly or perhaps the service definitions should be changed to not also require. In any event, I'm not certain the current approach will solve it appropriately (without causing issues for some use cases).

@geemus geemus referenced this pull request May 15, 2012
Closed

Interesting Comment in Code #904

@jherdman

Is there a particular reason Ruby's autoload isn't being used?

@geemus
fog member

@jherdman - there were (at least historically) concerns with its compatibility across different interpreters (jruby vs mri vs rubinius) and lack of thread safety in some earlier versions.

@p0deje

@jherdman I can't find the link, but a couple of months ago Matz mentioned that autoload will be deprecated in Ruby 2.0.

@jherdman

@p0deje I don't think we need to worry about that for a while. Matz has said it will be deprecated in 3.0.

@jherdman

@geemus would autoload be desirable if we can show that it's compatible across JRuby, MRI and Rubinius, or would you rather a different strategy for requiring files? I'm indifferent as to the shape of the final solution so long as circular requires are resolved.

@geemus
fog member

@jherdman - I would somewhat prefer to not use autoload given that there have been and will be issues with it (granted we might be in a window of it not having issues, it seems like it may be more trouble than it is worth). Part of the complication is the desire to allow either require "fog/compute" or require "fog/aws/compute" to be valid (and several other similar permutations). As such I'm not exactly sure what the correct approach is. Perhaps the service method should check to see if something is already loaded before doing the require. Overall I hadn't really worried about it too much as require ought to be a no-op in this case, but I don't generally think to run with warnings on. Not entirely certain what the best approach is here. Thanks for bringing attention to it. I may need to think about it a bit, but I'm certainly open to suggestions. If it ends up that autoload is the only real option we can discuss, but it seems troublesome.

@jherdman

@geemus I think what I'm going to do is drop the commit that "fixes" the circular references to get some of these fixes in. Circular references may take a little more work... I'm still not sure of the solution either. I think it'll require some discussion on Fog use cases. Thoughts?

@geemus
fog member

@jherdman - yeah, I think that sounds good. Would be good to get the other fixes in, but yeah I think the circular reference portion needs a bit more discussion. Thanks!

@jherdman

@geemus this branch should be ready now. I'll open an issue for the circular reference problem.

Can someone run the tests on this branch to make sure everything is passing? I saw some fails, but I'm not sure if they're known issues, or if they're because of additional configuration required to run all tests — I've had this problem in the past.

@jherdman jherdman referenced this pull request May 25, 2012
Closed

Circular References Found #929

@geemus
fog member

@jherdman - I can take it from here for now. The tests ought to pass. Missing configuration should result in pending (not failing) tests. So feel free to post new issues for any failures you see and we can get those adressed as well. Thanks!

@geemus geemus merged commit 8992b32 into fog:master May 28, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment