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 "method redefined" warnings introduced in Faraday 2.7.5 #1506

Merged
merged 2 commits into from Jun 7, 2023

Conversation

mattbrictson
Copy link
Contributor

@mattbrictson mattbrictson commented Jun 5, 2023

Description

Faraday 2.7.5 prints warnings about redefined methods. This was reported in #1505.

faraday-2.7.5/lib/faraday/options.rb:177: warning: method redefined; discarding old user
faraday-2.7.5/lib/faraday/options.rb:177: warning: method redefined; discarding old password
faraday-2.7.5/lib/faraday/options.rb:177: warning: method redefined; discarding old request
faraday-2.7.5/lib/faraday/options.rb:177: warning: method redefined; discarding old ssl
faraday-2.7.5/lib/faraday/options.rb:177: warning: method redefined; discarding old builder_class
faraday-2.7.5/lib/faraday/request.rb:48: warning: method redefined; discarding old params=
faraday-2.7.5/lib/faraday/request.rb:59: warning: method redefined; discarding old headers=
faraday-2.7.5/lib/faraday/request/instrumentation.rb:10: warning: method redefined; discarding old name
faraday-2.7.5/lib/faraday/request/instrumentation.rb:15: warning: method redefined; discarding old instrumenter

The underlying problem is that these methods are implicitly defined by Struct. When Faraday attempts to redefine them to alter the default Struct behavior, the warnings are printed.

Fix by using remove_method to remove the default Struct implementation before Faraday's are declared.

I tested that the warnings are no longer printed on Ruby 2.6 and 2.7 (via Docker) and 3.0, 3.1, and 3.2 (on macOS via rbenv).

Fixes #1505

@@ -2,6 +2,8 @@

require 'rspec/core/rake_task'

RSpec::Core::RakeTask.new(:spec)
RSpec::Core::RakeTask.new(:spec) do |task|
task.ruby_opts = %w[-W]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

🗒️ This ensures that Ruby warnings are printed when running the specs.

@@ -174,6 +174,7 @@ def self.memoized(key, &block)

memoized_attributes[key.to_sym] = block
class_eval <<-RUBY, __FILE__, __LINE__ + 1
remove_method(key) if method_defined?(key, false)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

🗒️ The memoized helper can be used to memoize existing methods, or it can be used to create entirely new methods. So it is necessary to check whether the method exists before attempting to remove it.

The specs have an example of memoized being used to synthesize a previously non-existent method a:

options_class.memoized(:a) { :foo }

@@ -42,6 +42,7 @@ def self.create(request_method)
end
end

remove_method :params=
Copy link
Contributor Author

Choose a reason for hiding this comment

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

🗒️ Since Request is defined via Struct.new, it already has getter and setter methods for its struct attributes, which include params and params=. The code that follows this statement redefines the params= method, which would normally trigger a "method redefined" warning. To avoid that warning, I am explicitly removing the default Struct method first.

I've applied this pattern to the other method definitions that were triggering warnings.

Copy link
Member

Choose a reason for hiding this comment

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

Right, that makes sense now!

Before the switch from inheritance (Request < Struct(...)) to assignment (Request = Struct(...)), these methods were defined in the superclass, so these definitions were simply overriding the parent implementation.
Now they're technically redefinition, which triggers the Ruby warning.

Great catch!

Copy link
Member

@iMacTia iMacTia left a comment

Choose a reason for hiding this comment

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

Thank you for providing the fix AND the insightful comments 🙏
I was a bit at a loss as to how this was introduced, but now I can clearly see why 🎉

@@ -42,6 +42,7 @@ def self.create(request_method)
end
end

remove_method :params=
Copy link
Member

Choose a reason for hiding this comment

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

Right, that makes sense now!

Before the switch from inheritance (Request < Struct(...)) to assignment (Request = Struct(...)), these methods were defined in the superclass, so these definitions were simply overriding the parent implementation.
Now they're technically redefinition, which triggers the Ruby warning.

Great catch!

@iMacTia iMacTia merged commit 86298d0 into lostisland:main Jun 7, 2023
8 checks passed
@mattbrictson mattbrictson deleted the bugs/fix-method-defn-warnings branch June 7, 2023 14:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Lots of "warning: method redefined; discarding old" printed during testing
2 participants