Skip to content

Turning on warnings in "rake test" for railties #3782

Merged
merged 1 commit into from Apr 2, 2013
@arunagw
Ruby on Rails member
arunagw commented Nov 28, 2011

Showing warnings in log

@josevalim
Ruby on Rails member

Sweet, how verbose is it? A bunch of warnings or just a few?

@arunagw
Ruby on Rails member
arunagw commented Nov 28, 2011

There are a lot from thor. From Rails there are less.

@josevalim
Ruby on Rails member
@arunagw
Ruby on Rails member
arunagw commented Nov 28, 2011

Yeah. That can be done. Let me give a try for that. :-)

Thanks :-)

@josevalim
Ruby on Rails member
@arunagw
Ruby on Rails member
arunagw commented Nov 28, 2011

My First Attack is Rack-cache : rtomayko/rack-cache#43

Attacking more.

@henrikhodne

Sweet, I usually just do export RUBYOPT="-w", but this is nicer. Also, many of the warnings already have pull requests to fix them.

@arunagw
Ruby on Rails member
arunagw commented Nov 28, 2011

I have one more here to fix warning.
@josevalim i think this can go anytime

#3278

I have created a branch in Mail gem to fix some of the warnings. Hope we will get that merged soon
https://github.com/mikel/mail/tree/warning_fixes

@arunagw
Ruby on Rails member
arunagw commented Nov 28, 2011

@josevalim I think you going to need to rc for thor which I can test out. Most of the warning already fixed in 0.15.0.rc2. :-)

@parndt
parndt commented Mar 13, 2012

+1 I'd love to see this merged in master that we're made very aware of all warnings so that we can fix them before 4.0.0 :-)

@arunagw
Ruby on Rails member
arunagw commented Mar 13, 2012

We have left with some rake and mail gem warnings and one from us only!

But it's still verbose for each test in a file.

@arunagw
Ruby on Rails member
arunagw commented Mar 13, 2012

OMG there are a lot from thor still!!

@sikachu
Ruby on Rails member
sikachu commented Apr 28, 2012

It has been two months. How are we doing on this?

@arunagw
Ruby on Rails member
arunagw commented Apr 29, 2012

I recently see this and we still have thor warnings. If that can be removed then this PR is good to go.

@josevalim
Ruby on Rails member

They just released Thor 0.15.0. Maybe we should run rails tests with it and see if it reduces the amount of warnings?

@arunagw
Ruby on Rails member
arunagw commented Apr 30, 2012

So we have some warnings

rake

/Users/arunagw/checkouts/rails/bundle/ruby/1.9.1/gems/rake-0.9.2.2/lib/rake/file_list.rb:350: warning: method redefined; discarding old exclude?
/Users/arunagw/checkouts/rails/bundle/ruby/1.9.1/gems/rake-0.9.2.2/lib/rake/file_list.rb:77: warning: previous definition of exclude? was here

and from our code

/Users/arunagw/checkouts/rails/railties/lib/rails/deprecation.rb:6

and also 4-5 from our code for method redefined. Can we removed that also first?

@schneems
Ruby on Rails member

@arunagw what is the state of this issue? Making progress, stuck on something? What are the blockers left for merge.

@steveklabnik
Ruby on Rails member

What's up with this?

@arunagw
Ruby on Rails member
arunagw commented Sep 15, 2012

I rebased with latest master. Just looked tests again with warning mode on. There are few from Bundler and Benchmark-ips. And for others itself from Rails I will do a PR for that. I think it's good to go.

@schneems
Ruby on Rails member

This looks good to me, thanks @arunagw, if you still have the warnings could you paste them in here? We can pass that info along to the appropriate libraries so they might be able to work on them.

@arunagw
Ruby on Rails member
arunagw commented Sep 18, 2012

Major is

/Users/arunagw/checkouts/rails/bundle/ruby/1.9.1/gems/benchmark-ips-1.2.0/lib/benchmark/ips.rb:240: warning: assigned but unused variable - seconds


/Users/arunagw/checkouts/rails/railties/lib/rails/deprecation.rb:6: warning: assigned but unused variable - constant

@rafaelfranca
Ruby on Rails member

Can't we fix the railties/lib/rails/deprecation.rb:6 ?

@frodsan
frodsan commented Oct 27, 2012

@rafaelfranca I think Steve fixed that warning. Is it ready?

@arunagw
Ruby on Rails member
arunagw commented Oct 27, 2012

Yes that's fixed.

Here is summary from latest run

  1. I did a PR here which needs to merge evanphx/benchmark-ips#5

  2. Latest changes from in master got this warning warning: previous definition of include_root_in_json was here

  3. And there are few from sprockets-rails.

@guilleiguaran
Ruby on Rails member

feel free to send any PR to sprockets-rails, I will merge immediately :)

@arunagw
Ruby on Rails member
arunagw commented Nov 20, 2012

@guilleiguaran that's nice. Let me give a try!

@senny
Ruby on Rails member
senny commented Apr 2, 2013

ping.

What is the status on this one?

@spastorino spastorino merged commit 62affac into rails:master Apr 2, 2013
@arunagw arunagw deleted the arunagw:warning_mode_on branch Apr 2, 2013
@arunagw arunagw restored the arunagw:warning_mode_on branch Apr 2, 2013
@sgerrand sgerrand pushed a commit to sgerrand/rails that referenced this pull request Nov 2, 2013
@arunagw arunagw Few more warnings removed.
I found them when I was running
warning mode on with railties

See rails#3782
ff04bb8
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.