Skip to content

Commit

Permalink
Don't log when overwriting Mash keys
Browse files Browse the repository at this point in the history
When we switched to using `#respond_to?` to detecting whether to log
a Mash collision, we started reporting when we were overwriting keys
that already exist in the Mash. This is a poor experience because it
causes extra warnings (as in #414) or, in the worst case, causes an
"undefined method" error (as in #413).

This change fixes that problem and benchmarks to ensure we're not
appreciably regressing performance. The results of two benchmarks are
below:

```
bundle exec ruby benchmark/mash_method_access.rb:

Warming up --------------------------------------
  before    92.456k i/100ms
Calculating -------------------------------------
  before      1.290M (± 4.4%) i/s -      6.472M in   5.028183s

Pausing here -- run Ruby again to measure the next benchmark...

Warming up --------------------------------------
    after    92.941k i/100ms
Calculating -------------------------------------
    after      1.326M (± 5.4%) i/s -      6.692M in   5.060756s

Comparison:
   after:  1326239.2 i/s
  before:  1289624.0 i/s - same-ish: difference falls within error
```

and

```
within spec/integrations/omniauth,
bundle exec rake perf:ips

Warming up --------------------------------------
  before     1.260k i/100ms
Calculating -------------------------------------
  before     13.114k (± 4.2%) i/s -     66.780k in   5.101689s

Pausing here -- run Ruby again to measure the next benchmark...

Warming up --------------------------------------
    after     1.299k i/100ms
Calculating -------------------------------------
    after     13.149k (± 4.0%) i/s -     66.249k in   5.046630s

Comparison:
   after:    13148.9 i/s
  before:    13113.8 i/s - same-ish: difference falls within error
```

Closes #413
Closes #414
  • Loading branch information
michaelherold committed Feb 24, 2017
1 parent 9f77380 commit 2088079
Show file tree
Hide file tree
Showing 3 changed files with 16 additions and 3 deletions.
4 changes: 2 additions & 2 deletions .rubocop_todo.yml
@@ -1,6 +1,6 @@
# This configuration was generated by
# `rubocop --auto-gen-config`
# on 2017-02-22 17:31:41 -0600 using RuboCop version 0.34.2.
# on 2017-02-24 07:11:40 -0600 using RuboCop version 0.34.2.
# The point is for the user to remove these configuration records
# one by one as the offenses are removed from the code base.
# Note that changes in the inspected code, or installation of new
Expand All @@ -18,7 +18,7 @@ Metrics/AbcSize:
# Offense count: 2
# Configuration parameters: CountComments.
Metrics/ClassLength:
Max: 205
Max: 209

# Offense count: 8
Metrics/CyclomaticComplexity:
Expand Down
7 changes: 6 additions & 1 deletion lib/hashie/mash.rb
Expand Up @@ -145,7 +145,7 @@ def custom_reader(key)
def custom_writer(key, value, convert = true) #:nodoc:
key_as_symbol = (key = convert_key(key)).to_sym

log_built_in_message(key_as_symbol) if respond_to?(key_as_symbol)
log_built_in_message(key_as_symbol) if log_collision?(key_as_symbol)
regular_writer(key, convert ? convert_value(value) : value)
end

Expand Down Expand Up @@ -348,5 +348,10 @@ def log_built_in_message(method_key)
'property. You can still access the key via the #[] method.'
)
end

def log_collision?(method_key)
respond_to?(method_key) && !self.class.disable_warnings? &&
!(regular_key?(method_key) || regular_key?(method_key.to_s))
end
end
end
8 changes: 8 additions & 0 deletions spec/hashie/mash_spec.rb
Expand Up @@ -142,6 +142,14 @@
expect(logger_output).to match('Hashie::Mash#trust')
end

it 'can set keys more than once and does not warn when doing so' do
mash = Hashie::Mash.new
mash[:test_key] = 'Test value'

expect { mash[:test_key] = 'A new value' }.not_to raise_error
expect(logger_output).to be_blank
end

it 'does not write to the logger when warnings are disabled' do
mash_class = Class.new(Hashie::Mash) do
disable_warnings
Expand Down

0 comments on commit 2088079

Please sign in to comment.