-
Notifications
You must be signed in to change notification settings - Fork 21.4k
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 tickled by the test suite #33257
Conversation
(@rails-bot has picked a reviewer for you, use r? to override) |
@@ -64,7 +65,7 @@ def self.#{sym} | |||
|
|||
if instance_reader && instance_accessor | |||
class_eval(<<-EOS, __FILE__, __LINE__ + 1) | |||
def #{sym} | |||
redefine_method("#{sym}") do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why we need to do this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To avoid provoking warning: method redefined
.
Like here: https://travis-ci.org/rails/rails/jobs/398133523#L1281-L1283
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe better to silence this kind of warning the way Sequel does ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But if the method is being redefined by this method maybe the class/module that is calling it that is wrong, not this method that is. And that is exactly the case. We need to fix delayed_job https://github.com/collectiveidea/delayed_job/blob/73bd1b50e719b336b70fcbb8dc4a37ec9b2f6f35/lib/delayed/worker.rb#L292
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've removed both this and the other change to core extension. Looking into the delayed_job side.
@@ -131,7 +132,7 @@ def self.#{sym}=(obj) | |||
|
|||
if instance_writer && instance_accessor | |||
class_eval(<<-EOS, __FILE__, __LINE__ + 1) | |||
def #{sym}=(obj) | |||
redefine_method("#{sym}=") do |obj| |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why we need to do this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for looking. We don't. I've removed it. And I'm looking to fix these in dealyed_job.
See CI:
https://travis-ci.org/rails/rails/jobs/398133523#L1183-L1184
https://travis-ci.org/rails/rails/jobs/398133523#L1281-L1283
https://travis-ci.org/rails/rails/jobs/398133523#L1306
https://travis-ci.org/rails/rails/jobs/398133523#L1308
https://travis-ci.org/rails/rails/jobs/398133523#L1310
https://travis-ci.org/rails/rails/jobs/398133523#L1354