Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

StrictKeyAccess Extension #314

Merged
merged 1 commit into from Oct 23, 2015
Merged

StrictKeyAccess Extension #314

merged 1 commit into from Oct 23, 2015

Conversation

pboling
Copy link
Contributor

@pboling pboling commented Oct 18, 2015

  • This extension will raise an error whenever a key is accessed that does not exist in the hash.

In Python a "Hash" is called a "Dictionary", and ...

"It is an error to extract a value using a non-existent key."

See: https://docs.python.org/2/tutorial/datastructures.html#dictionaries

EXAMPLE:

class StrictHash < Hash
  include Hashie::Extensions::StrictKeyAccess
end
>> hash = StrictHash[foo: "bar"]
=> {:foo=>"bar"}
>> hash[:foo]
=> "bar"
>> hash[:cow]
KeyError: key not found: :cow

Why? I will be releasing a gem soon which needs this Python dictionary-like requirement. If not accepted into Hashie, I'll release the dictionary as a separate gem.

@pboling
Copy link
Contributor Author

pboling commented Oct 18, 2015

I forgot to add example to Readme. Will do.

@pboling pboling force-pushed the dictionary branch 2 times, most recently from 615ef6e to 96f4a19 Compare October 18, 2015 21:19
@dblock
Copy link
Member

dblock commented Oct 19, 2015

I like the code very much, but I want to discuss naming. First, Ruby > Python. Ok, just kidding. But I get how coming from Python one would want a Dictionary to be like a Python dictionary, but for a Rubyist that's a bit lost in translation.

I feel like this extension should be KeySomething or AccessSomething to be consistent with the rest of the names.

@pboling
Copy link
Contributor Author

pboling commented Oct 19, 2015

@dblock My first iteration's name was StrictHash, when it was just a modified Hash sub-class. Then I switched it to an extension for Hashie, and thought about StrictKeyAccess, but I changed it because I liked the idea of Dictionary, semantically*. It seems natural to expect an error if looking something up in a (Real Life) Dictionary and it is "not found". Similarly, words can be, and are, added to Dictionaries. A Dictionary in the real world is a very good analogy for this behavior.

But I'm happy to go back to StrictKeyAccess or similar.

On the specs, I am having trouble finding a Rubinius that will pass the build, so will be pending some specs there for the new Dictionary.

* I am by no means a fan of Python. I am a Rubyist. But I don't want to be a wordist, or an NIHist. ;)

@pboling pboling force-pushed the dictionary branch 4 times, most recently from de3440e to 754c16f Compare October 19, 2015 08:29
@pboling
Copy link
Contributor Author

pboling commented Oct 19, 2015

@dblock rebased on top of #313 because I needed the more advanced pending_for method to pend some specs for rbx.
Renamed Dictionary extension to StrictKeyAccess.
This PR is done, but #313 should be merged first.

@dblock
Copy link
Member

dblock commented Oct 19, 2015

Lets see if other people have good naming suggestions?

@michaelherold
Copy link
Member

StrictKeyAccess 👍 -- It matches the naming convention for MethodAccess.

@pboling
Copy link
Contributor Author

pboling commented Oct 23, 2015

@dblock @michaelherold Rebased on intridea/master, Squashed to 1 commit, and Rubocop'd. This is ready.

@dblock
Copy link
Member

dblock commented Oct 23, 2015

I still feel awkward reading that it's a Python like extension. To a Rubyist this makes no sense!

What do you think of this:

In CHANGELOG:

Added a `StringKeyAccess` extension that will raise an error whenever a key is accessed that does not exist in the hash.

In README same thing, something about what it does, as opposed to that it's "Python-like".

I can merge this myself and update if you @pboling wants to flip tables on me being such a pest :) Thanks for hanging in there!

@pboling
Copy link
Contributor Author

pboling commented Oct 23, 2015

Agreed on the CHANGELOG. I'll switch it. The reason I'd like to mention Python like somewhere is for googlers coming from Python to Ruby and looking for this specific behavior. I could even add:

For googlers coming from Python to Ruby, this extension makes a Hash behave like a "Dictionary".

@dblock what do you think of that, either in code file or read me?

@pboling
Copy link
Contributor Author

pboling commented Oct 23, 2015

This is SEO marketing for your gem, 🙌

@pboling
Copy link
Contributor Author

pboling commented Oct 23, 2015

OK @dblock please re-review. I did a search/replace Dictionary for StrictKeyAccess. There were a lot of dictionary references in there I had missed. Sorry! They are all now gone, with the exception of a single NOTE: in the strict_key_access.rb file, and the commit message itself.

Use Chrome find in page to check! :)

@pboling pboling changed the title Python-like Dictionary Extension StrictKeyAccess Extension Oct 23, 2015
… is accessed that does not exist in the hash.

In Python a "Hash" is called a "Dictionary", and ...

> "It is an error to extract a value using a non-existent key."

See: https://docs.python.org/2/tutorial/datastructures.html#dictionaries

EXAMPLE:

    class StrictHash < Hash
      include Hashie::Extensions::StrictKeyAccess
    end
    >> hash = StrictHash[foo: "bar"]
    => {:foo=>"bar"}
    >> hash[:foo]
    => "bar"
    >> hash[:cow]
      KeyError: key not found: :cow
@pboling
Copy link
Contributor Author

pboling commented Oct 23, 2015

@michaelherold Typo fixed. Please re-review!

dblock added a commit that referenced this pull request Oct 23, 2015
@dblock dblock merged commit c652c5b into hashie:master Oct 23, 2015
@dblock
Copy link
Member

dblock commented Oct 23, 2015

Nice work, merged.

# Also note that defaults still don't make any sense with a StrictKeyAccess.
def self.included(base)
# Can only include into classes with a hash initializer
base.send(:include, Hashie::Extensions::MergeInitializer)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand why this is necessary. Hashes don't, by default, have a merging initializer. Is this just to get around the fact that a default proc doesn't make sense?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@michaelherold Nothing to do with the default. Overriding :[] to fetch breaks normal Hash initialization. See the note at the top of the comment above:

NOTE: This extension would break the default behavior of Hash initialization:
>> a = StrictKeyAccessHash.new(a: :b)
=> {}
>> a[:a]
KeyError: key not found: :a
Includes the Hashie::Extensions::MergeInitializer extension to get around that problem.

Here's a demo:

>> class BasicStrictHash < Hash
>>   def [](key); fetch(key); end
>> end
=> :[]
>> basic_strict_hash = BasicStrictHash[a: :b] # works!
=> {:a=>:b}
>> basic_strict_hash[:a]
=> :b
>> basic_strict_hash_via_new = BasicStrictHash.new(a: :b) # does *not* work!
=> {}
>> basic_strict_hash_via_new[:a] # ERROR!!!
KeyError: key not found: :a
    from (irb):2:in `fetch'
    from (irb):2:in `[]'
    from (irb):7
    from bin/console:14:in `<main>'

When we use the MergeInitializer... magic happens:

>> class StrictHashWithFixedInitializer < Hash
>>   include Hashie::Extensions::MergeInitializer # Insert Magic Beans!
>>   def [](key); fetch(key); end
>> end
=> :[]
>> strict_hash_with_fixed_initializer = StrictHashWithFixedInitializer[a: :b] # still works!
=> {:a=>:b}
>> strict_hash_with_fixed_initializer[:a]
=> :b
>> strict_hash_with_fixed_initializer_via_new = StrictHashWithFixedInitializer.new(a: :b) # now it works!
=> {:a=>:b}
>> strict_hash_with_fixed_initializer_via_new[:a] # PROFIT!!
=> :b

Does that make it clear?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, this still seems completely unrelated to me. It looks like you're expecting a basic Hash.new(a: :b) to initialize the created hash to {:a => :b}, which is not the expected behavior.

When you pass any object to Hash.new (including a preexisting Hash), the result is a Hash that has a default value of the object you passed in. In my experience, this trips up a lot of people, but it doesn't mean that it's not the default behavior.

test = Hash.new(a: :b)  #=> {}

In the case of a Hash with StrictKeyAccess mixed in, passing a default value into Hash.new doesn't semantically make sense, but neither does the result of implicitly mixing in MergeInitializer. When I mix in StrictKeyAccess, I want strict key access ... not strict key access AND merge initialization. Doing things implicitly, in my experience, leads to confusion for users because it adds behavior they weren't expecting.

In an ideal world, I would want to disallow passing in the default value when StrictKeyAccess is mixed in, but I'm not sure how well that would play with the other mixins. I would need to play around with it a bit. As the extension currently stands -- sans the implicit inclusion of MergeInitializer -- this implementation works as expected:

class Dictionary < Hash
  include Hashie::Extensions::StrictKeyAccess
end

# Note: This is just for example. I don't think you should _ever_ do this next line
# without something like MergeInitializer because the default implementation is
# not what you'd think.
test = Dictionary.new(a: :b)  # => {}
test[:a]  #=> KeyError: key not found: :a

If the user wants merge initialization, that's fine -- they can also mix in MergeInitializer. That's why we have it after all. 😃

I would rather this extension not also include MergeInitializer, since MergeInitializer has nothing to do with StrictKeyAccess.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah... very good explanation, I had thought the new thing was a bit off, but didn't quite remember the default behavior. The stack overflow I read about this, and got my original idea from was confusingly written (http://stackoverflow.com/questions/16905191/raise-exception-when-accessing-attributes-that-doesnt-exist-in-openstruct).

I agree completely. I'll submit another PR with a fix.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I get it now too. Sorry for missing this in the CR.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No worries! I think the whole "default value" implementation in Ruby is a little nonsensical. I think the behavior of MergeInitializer would make a lot more sense, but it would get rid of the simple example in the core docs:

(from http://ruby-doc.org/core-2.2.3/Hash.html#method-c-new)

h = Hash.new("Go Fish")
h["a"] = 100
h["b"] = 200
h["a"]           #=> 100
h["c"]           #=> "Go Fish"

This confusing part is that you can then do this:

h = Hash.new("Go Fish")
h["a"] = 100
h["b"] = 200
h["a"]           #=> 100
h["c"]           #=> "Go Fish"
# The following alters the single default object
h["c"].upcase!   #=> "GO FISH"
h["d"]           #=> "GO FISH"

Which is nonsense and confusing when you're not expecting it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@michaelherold your example with upcase is awesome! I would love to read a blog post on abusing Ruby hashes for fun and no profit ;)

jsonn pushed a commit to jsonn/pkgsrc that referenced this pull request Dec 12, 2015
## 3.4.3 (10/25/2015)

* [#314](hashie/hashie#314): Added a
  `StrictKeyAccess` extension that will raise an error whenever a key is
  accessed that does not exist in the hash -
  [@pboling](https://github.com/pboling).

* [#304](hashie/hashie#304): Ensured compatibility
  of `Hash` extensions with singleton objects -
  [@regexident](https://github.com/regexident).

* [#306](hashie/hashie#306): Added
  `Hashie::Extensions::Dash::Coercion` -
  [@marshall-lee](https://github.com/marshall-lee).

* [#310](hashie/hashie#310): Fixed
  `Hashie::Extensions::SafeAssignment` bug with private methods -
  [@marshall-lee](https://github.com/marshall-lee).

* [#313](hashie/hashie#313): Restrict pending spec
  to only Ruby versions 2.2.0-2.2.2 - [@pboling](https://github.com/pboling).

* [#315](hashie/hashie#315): Default `bin/` scripts:
  `console` and `setup` - [@pboling](https://github.com/pboling).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants