dry-rb / dry-container

A simple, configurable object container implemented in Ruby
https://dry-rb.org/gems/dry-container
MIT License
335 stars 41 forks source link

Alias #key? as #has_key? #57

Closed endash closed 5 years ago

endash commented 5 years ago

It would be nice if #key? were aliased to #has_key? so that the expect(container).to have_key(key) RSpec matcher can be used instead of expect(container.key?(key)).to be(true)

flash-gordon commented 5 years ago

I wouldn't do it, I like Container's interface being close to Hash but the status of Hash#has_key? isn't clear http://blade.nagaokaut.ac.jp/cgi-bin/scat.rb/ruby/ruby-core/43765

endash commented 5 years ago

Interesting. I actually was surprised to find that the have_key matcher didn't just use key?, so it definitely seems to be one of those hidden dusty corners of Ruby. 7 years on, though, it doesn't seem like has_key? is going anywhere, and in the meantime it'd be nice to test using the same matchers.

I suppose I could take it up with rspec-expectations first, but I suspect they'd be disinclined to change something so battle-tested, in order to accommodate classes that partially duck-subtype Hash.

It's a bit of a catch-22... rspec likely don't see any reason to change what works, but on the other hand perpetuating an API that Matz considers a design mistake is less than ideal.

Won't hurt to ask there, though. I can't think of a practical reason why just changing the matcher to #key? wouldn't be a universally acceptable solution, other than caution.

endash commented 5 years ago

Sigh. Never mind. This is a dynamic matcher, playing off of the has prefix. That makes it a "user's choice" API decision, out of their hands.

# You can use this feature to invoke any predicate that begins with "has_", whether it is
# part of the Ruby libraries (like `Hash#has_key?`) or a method you wrote on your own class.

Maybe including a custom matcher a user can include in their spec_helper... but at that point they might as well just implement it themselves, trivially:

RSpec::Matchers.define(:have_key) do |expected|
  match { |actual| actual.key?(expected) }
end

Personally, I don't see a problem with just aping Hash all that more closely... but I see the other argument, too.

flash-gordon commented 5 years ago

I would go with a matcher since the only "real" reason for having it is good looking specs. And anyway, if we add has_key? why not include? etc. We can add the matcher to source code, not a problem

# load it explicitly
require 'dry/container/rspec/matcher'

# and here you go
is_expected.to have_key(key)
endash commented 5 years ago

Discovered you can also do

is_expected.to be_key(key)

which will invoke the ? predicate matcher. If you want to be both terse AND semantically inaccurate.

I think you're right though @flash-gordon, it's mildly satisfying to have the cleaner looking expect, which is probably worth tossing in an optional require, but if the intention is not to replicate the Hash interface, and it probably shouldn't be, then best to let sleeping dogs lie in this case.