Add and fix specs for Hash keys with private hash method #2170

merged 2 commits into from Feb 24, 2013


None yet
2 participants

brainopia commented Feb 23, 2013

MRI and JRuby have support for Hash keys with private hash method. Rubinius currently throws an exception.

I've encountered this error when running specs on travis –

This pull-request fixes it.


dbussink commented Feb 23, 2013

There's one problem with this pull request, that is that send() currently is significantly slower than a regular call, so merging this would affect Hash performance everywhere.

We probably have to use the Rubinius.privately construct we use in other places to allow private sends.


brainopia commented Feb 24, 2013

Thanks, I've changed it to use Rubinius.privately

@dbussink dbussink commented on an outdated diff Feb 24, 2013

@@ -112,4 +112,9 @@
new_hash(x => :x)[x].should == :x
+ it "supports keys with private #hash method" do
+ k = { private :hash }.new
+ lambda { new_hash[k] }.should_not raise_error

dbussink Feb 24, 2013


We should have better specs for this, we should assert on the actual behavior, not on whether it doesn't raise an error. Not raising an error would be a valid spec for any code, so we should verify that the lookup actually works here. Having an implementation for example that ignores values with private hashes here would be valid according to this spec.

The best idea is probably to add a class for testing this in spec/ruby/core/hash/fixtures/classes.rb and use that to verify the behavior. In this case you'd want to specify that it retrieves the correct value from a hash.

Same comment applies to the spec for []= btw.


brainopia commented Feb 24, 2013

Done :)

@dbussink dbussink added a commit that referenced this pull request Feb 24, 2013

@dbussink dbussink Merge pull request #2170 from brainopia/support_private_hash
Add and fix specs for Hash keys with private hash method

@dbussink dbussink merged commit abc1bc5 into rubinius:master Feb 24, 2013

1 check was pending

default The Travis build is in progress

dbussink commented Feb 24, 2013

Thanks for the contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment