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.

Sign up for GitHub

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

Jump to bottom

Fix "method redefined" warnings introduced in Faraday 2.7.5 #1506

Merged
merged 2 commits into from
Jun 7, 2023
Merged

Fix "method redefined" warnings introduced in Faraday 2.7.5 #1506

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2a1aac0
Print Ruby warnings when running specs
mattbrictson Jun 5, 2023
084f485
Fix 'method redefined' warnings
mattbrictson Jun 5, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 3 additions & 1 deletion Rakefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

end

task default: :spec
1 change: 1 addition & 0 deletions lib/faraday/options.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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:

faraday/spec/faraday/options/options_spec.rb

Line 260 in d40f3db

options_class.memoized(:a) { :foo }

def #{key}() self[:#{key}]; end
RUBY
end
Expand Down
2 changes: 2 additions & 0 deletions lib/faraday/request.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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!

# Replace params, preserving the existing hash type.
#
# @param hash [Hash] new params
Expand All @@ -53,6 +54,7 @@ def params=(hash)
end
end

remove_method :headers=
# Replace request headers, preserving the existing hash type.
#
# @param hash [Hash] new headers
Expand Down
2 changes: 2 additions & 0 deletions lib/faraday/request/instrumentation.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,13 @@ class Request
class Instrumentation < Faraday::Middleware
# Options class used in Request::Instrumentation class.
Options = Faraday::Options.new(:name, :instrumenter) do
remove_method :name
# @return [String]
def name
self[:name] ||= 'request.faraday'
end

remove_method :instrumenter
# @return [Class]
def instrumenter
self[:instrumenter] ||= ActiveSupport::Notifications
Expand Down