Shopify / measured-rails

Rails adapter for the measured gem. Encapsulate measurements and their units in Ruby and Rails.
MIT License
92 stars 13 forks source link

Fix warnings by using `method_defined?` or `redefine_method` #58

Closed kmcphillips closed 5 years ago

kmcphillips commented 5 years ago

Problem

In trying to remove warnings I hit these:

measured/rails/active_record.rb:78: warning: method redefined; discarding old size_unit=
measured/rails/active_record.rb:78: warning: previous definition of size_unit= was here
measured/rails/active_record.rb:78: warning: method redefined; discarding old size_unit=
measured/rails/active_record.rb:78: warning: previous definition of size_unit= was here
measured/rails/active_record.rb:78: warning: method redefined; discarding old weight_unit=
measured/rails/active_record.rb:78: warning: previous definition of weight_unit= was here

Which is define_method("#{ unit_field_name }=") triggering multiple times on redefinition of a field.

This was added in #27

Solution

There are a few things. First, the defined_unit_accessors never actually worked as described because the variable was updated at the wrong level. It is originally defined as [] at a class macro scope, but the << was only happening inside the instance definition. So it was never saved.

Moving it down to the right level reduced the warnings, but some still existed as for when a method is redefined in two different calls to the class macro.

So I've settled on using method_defined? I think. That checks if it already exists and if it does then do not define it. This approach may be wrong! As if the method already exists it won't overwrite it, which may not have been defined by measured.

A different approach as suggested by @gmcgibbon is to use redefine_method, meaning that it would then keep the last definition of the method, not the first definition of the method (regardless of where that definition is from). https://github.com/rails/rails/blob/master/activesupport/lib/active_support/core_ext/module/redefine_method.rb

kmcphillips commented 5 years ago

Talking with @gmcgibbon I've shifted to redefine_method as that better matches the original behaviour.