Fixed #303 (deep_merge/stringify_keys/symbolize_keys not working via hash.extend(…))

[regexident]

Fixes issue #303

[michaelherold]

Hmm ... I've thought about it a little bit and I came to a conclusion. I think duck-checking for this would be more appropriate, since ActiveSupport and the deep_merge gem, as well as others provide functionality like deep_merge, stringify_keys, etc. Since ActiveSupport automatically monkey-patches hashes when used in the Rails configuration, normal hashes will have a lot of these methods already.

Duck-checking also makes it a little more readable than checking the singleton class of the object:

copy = dup
copy.extend(Hashie::Extensions::DeepMerge) unless copy.respond_to?(:deep_merge!)
copy.deep_merge!(other_hash, &block)

Does anyone else have thoughts on the matter?

Also, this needs a change log entry and some style cleanup (per Rubocop) before we can merge it.

[mtrense]

@michaelherold But the dup'ed object either already has a deep_merge method (when included in it's class – in which case one would rather use the existing method instead of explicitly extending that one object) or does not get one at all (because it's a newly created object of the original objects class). So both checks are effectively equal to each other.

[michaelherold]

They're not quite equivalent to each other. If our DeepMerge extension is unthinkingly placed on an object that already knows how to deep merge, then copy would respond to #deep_merge! so we actually wouldn't need to re-extend it given the assumption that all #deep_merge! behaviors work the same. (I admit that is a strong assumption, but in this case I believe it's true.)

I think the question becomes: what do we want the behavior of our mixins to be when they are extended onto an object that already has those methods? Option 1: always use our implementation. Option 2: use the object's pre-existing implementation, if available.

This behavior can be show with this little proof-of-concept:

class BaseClass
  def my_method
    puts 'base class bare method'
  end

  def my_method!
    puts 'base class bang method'
  end
end

module DuckCheckingExtension
  def my_method
    copy = dup
    copy.extend(DuckCheckingExtension) unless copy.respond_to?(:my_method!)
    copy.my_method!
  end

  def my_method!
    puts 'extension method'
  end
end

module SingletonCheckingExtension
  def my_method
    copy = dup
    copy.extend(SingletonCheckingExtension) unless (class << copy; self; end).included_modules.include?(SingletonCheckingExtension)
    copy.my_method!
  end

  def my_method!
    puts 'extension method'
  end
end

test = BaseClass.new
test.extend(DuckCheckingExtension)
test.my_method
#=> base class bang method

test2 = BaseClass.new
test2.extend(SingletonCheckingExtension)
test2.my_method
#=> extension method

The duck checking is also significantly faster on the proof-of-concept, since we're not building and checking the singleton class:

require 'benchmark'

GC.disable

N = 1_000_000

Benchmark.bmbm do |x|
  x.report(:duck) { N.times { test.my_method } }
  x.report(:singleton) { N.times { test2.my_method } }
end

Rehearsal ---------------------------------------------
duck        0.320000   0.000000   0.320000 (  0.322056)
singleton   1.320000   0.170000   1.490000 (  1.484601)
------------------------------------ total: 1.810000sec

                user     system      total        real
duck        0.290000   0.010000   0.300000 (  0.307832)
singleton   1.320000   0.140000   1.460000 (  1.445568)

[dblock]

There're some build errors, to start. Also please squash these commits.

[dblock]

@michaelherold Do you see any issues with this? Feel free to merge if you like.

[regexident]

Thanks a bunch! 🙇