summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorMichael Herold <opensource@michaeljherold.com>2020-01-15 13:17:27 -0600
committerGitHub <noreply@github.com>2020-01-15 13:17:27 -0600
commiteb69c58b627379568d0c7dfc73eead42c5f1f140 (patch)
treea977a64010141cb7641624670246c5a6a2f5e64f
parent25fe2f747ee06e563808e08493ec7a471cc82b26 (diff)
downloadhashie-eb69c58b627379568d0c7dfc73eead42c5f1f140.tar.gz
Don't warn when setting most affixed keys (#500)
Due to how we have implemented the bang/underbang/query behavior within Mash, setting keys that have those affixes in them actually allow overwriting the behavior of those affixes. As such, we shouldn't warn when setting a key that matches those patterns. When it comes to setter-like keys, I believe we still _do_ want to warn for two reasons: 1. Trying to access the key via method access is a syntax error. Ruby expects any method ending in `=` to be a 2+-arity method due to the infix notation of setter methods. This is unexpected behavior unless you're very familiar with Ruby parsing. 2. You can still retrieve the key via the normal `Hash#[]` reader, but it prevents setting a similar key without the equal sign. You can see this in the test about setters. I'd say that is unexpected and surprising behavior. Because of these two gotchas, I think we should still warn in cases where you try to set a key that looks like a setter.
-rw-r--r--CHANGELOG.md1
-rw-r--r--lib/hashie/mash.rb7
-rw-r--r--spec/hashie/mash_spec.rb22
-rw-r--r--spec/spec_helper.rb1
-rw-r--r--spec/support/matchers.rb13
5 files changed, 43 insertions, 1 deletions
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 2f5c487..ec6b19f 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -34,6 +34,7 @@ scheme are considered to be bugs.
* [#507](https://github.com/hashie/hashie/pull/507): Suppress `Psych.safe_load` arg warn when using Psych 3.1.0+ - [@koic](https://github.com/koic).
* [#508](https://github.com/hashie/hashie/pull/508): Fixed `Mash.load` no longer uses Rails-only `#except` - [@bobbymcwho](https://github.com/bobbymcwho).
* [#508](https://github.com/hashie/hashie/pull/508): Fixed `Hashie::Extensions::DeepMerge` `#deep_merge` not correctly dup'ing sub-hashes if active_support hash extensions were not present - [@bobbymcwho](https://github.com/bobbymcwho).
+* [#500](https://github.com/hashie/hashie/pull/500): Do not warn when setting Mash keys that look like underbang, bang, and query methods - [@michaelherold](https://github.com/michaelherold).
* [#510](https://github.com/hashie/hashie/pull/510): Ensure that `Hashie::Mash#compact` is only defined on Ruby version >= 2.4.0 - [@bobbymcwho](https://github.com/bobbymcwho).
* Your contribution here.
diff --git a/lib/hashie/mash.rb b/lib/hashie/mash.rb
index 97f75d5..194a6f8 100644
--- a/lib/hashie/mash.rb
+++ b/lib/hashie/mash.rb
@@ -406,7 +406,12 @@ module Hashie
end
def log_collision?(method_key)
- respond_to?(method_key) && !self.class.disable_warnings?(method_key) &&
+ return unless respond_to?(method_key)
+
+ _, suffix = method_name_and_suffix(method_key)
+
+ (!suffix || suffix == '='.freeze) &&
+ !self.class.disable_warnings?(method_key) &&
!(regular_key?(method_key) || regular_key?(method_key.to_s))
end
end
diff --git a/spec/hashie/mash_spec.rb b/spec/hashie/mash_spec.rb
index 08e6f09..b0aef57 100644
--- a/spec/hashie/mash_spec.rb
+++ b/spec/hashie/mash_spec.rb
@@ -159,6 +159,28 @@ describe Hashie::Mash do
expect(logger_output).to be_empty
end
+ it 'does not write to the logger when setting most affixed keys' do
+ underbang = Hashie::Mash.new('foo_' => 'foo')
+ bang = Hashie::Mash.new('foo!' => 'foo')
+ query = Hashie::Mash.new('foo?' => 'foo')
+
+ expect(logger_output).to be_empty
+ expect(underbang.foo_).to eq 'foo'
+ expect(bang.foo!).to eq 'foo'
+ expect(query.foo?).to eq 'foo'
+ end
+
+ it 'warns when setting a key that looks like a setter' do
+ setter = Hashie::Mash.new('foo=' => 'foo')
+
+ expect(logger_output).to match 'Hashie::Mash#foo='
+ expect('setter.foo=').not_to parse_as_valid_ruby
+
+ setter.foo = 'bar'
+
+ expect(setter.to_h).to eq 'foo=' => 'foo'
+ end
+
it 'cannot disable logging on the base Mash' do
expected_error = Hashie::Extensions::KeyConflictWarning::CannotDisableMashWarnings
diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb
index dc31d61..b0f9ce9 100644
--- a/spec/spec_helper.rb
+++ b/spec/spec_helper.rb
@@ -10,6 +10,7 @@ require 'hashie'
require 'rspec/pending_for'
require './spec/support/ruby_version_check'
require './spec/support/logger'
+require './spec/support/matchers'
RSpec.configure do |config|
config.extend RubyVersionCheck
diff --git a/spec/support/matchers.rb b/spec/support/matchers.rb
new file mode 100644
index 0000000..cb2b3c0
--- /dev/null
+++ b/spec/support/matchers.rb
@@ -0,0 +1,13 @@
+RSpec::Matchers.define :parse_as_valid_ruby do
+ require 'ripper'
+
+ match do |actual|
+ parsed = Ripper.sexp(actual)
+
+ !parsed.nil?
+ end
+
+ failure_message do |actual|
+ "expected that #{actual} would parse as valid Ruby"
+ end
+end