Fix a few warnings while running specs #2115

Merged
merged 1 commit into from Oct 2, 2012

Conversation

Projects
None yet
3 participants
Contributor

rohit commented Oct 2, 2012

So while looking at #2109 I decided to look at the warnings that show up while running specs. These were the warnings that showed up. I just gave a cursory look at why. I've attached notes on if it was fixed. If it wasn't, a note saying why.

/home/rohit/opensource/bundler/spec/quality_spec.rb:4: warning: setting Encoding.default_external

Can't fix, warning because of http://www.ruby-doc.org/core-1.9.3/Encoding.html#method-c-default_external-3D

/home/rohit/opensource/bundler/spec/cache/git_spec.rb:62: warning: possibly useless use of == in void context
/home/rohit/opensource/bundler/spec/cache/git_spec.rb:82: warning: possibly useless use of == in void context

Probably should use err.should be == "" or err.should eql(""). Warning thrown because == is an expression (afaik) and it's return value is not being used/stored. Why only these two warnings? I don't know that but the obj.should == <string_literal> is used throughout bundler, so I didn't feel like changing only these two occurrences.

/home/rohit/opensource/bundler/spec/cache/git_spec.rb:146: warning: assigned but unused variable - ref
/home/rohit/opensource/bundler/spec/cache/git_spec.rb:159: warning: assigned but unused variable - ref

Fixed

/home/rohit/opensource/bundler/lib/bundler/source.rb:516: warning: method redefined; discarding old revision

IDK why the two methods are getting mixed up. Or is the file being run/parsed twice?

/home/rohit/opensource/bundler/lib/bundler/definition.rb:238: warning: assigned but unused variable - e

Fixed

/home/rohit/opensource/bundler/lib/bundler/vendor/thor/base.rb:566: warning: instance variable @no_tasks not initialized
/home/rohit/opensource/bundler/lib/bundler/vendor/thor.rb:294: warning: instance variable @long_desc not initialized

Didn't fix. As I understand it, this is vendored thor and should be as-is from upstream.

/home/rohit/opensource/bundler/spec/runtime/with_clean_env_spec.rb:32: warning: assigned but unused variable - gem_path

Fixed.

After fixing above, two more warnings showed up for git_spec.rb saying git assigned but not used. Fixed that too.

Owner

indirect commented Oct 2, 2012

awesome, thanks! unfortunately running the specs with -W only shows warnings inside the spec files themselves, rather than showing warnings that the Bundler source triggers while the specs run. I'm planning on looking into it in a couple of weeks, but if you want to check it out, you can probably use the sys_exec method to add -w to the ruby calls that invoke Bundler during the tests.

@indirect indirect added a commit that referenced this pull request Oct 2, 2012

@indirect indirect Merge pull request #2115 from rohit/fix_some_warnings
Fix a few warnings while running specs
7fa62c9

@indirect indirect merged commit 7fa62c9 into bundler:master Oct 2, 2012

1 check passed

default The Travis build passed
Details

Can you guys cut a new gem with this?

Owner

indirect commented Oct 3, 2012

@TylerBrock seriously, a new gem so that the tests contain fewer warnings? I mean, I realize that we ship the tests with the gem, but they um... don't actually work. So it doesn't really seem worth it.

Sorry @indirect. I think this fixes issues running bundle exec in conjunction with rake testtasks (superflous warnings not related to what is being tested)

Anyhow, I'll wait patiently. Thanks for taking a look at all of this and thanks to @rohit for the fixes.

Contributor

rohit commented Oct 3, 2012

@TylerBrock Sorry but the particular warning that you wanted fixed, I couldn't fix. I'll take a proper look at it when I have the time. TBH I don't know why it's happening. Because the revision method are in separate classes (albeit one of them is in an inner class) this shouldn't happen. I suspect the file is being run/parsed/something twice hence the warning. No other methods named revision are defined elsewhere so that must be it.

Also it wouldn't be nice to have everybody upgrade Bundler with just some warnings hidden. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment