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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow #fetch access in Rash #189

Closed
wants to merge 1 commit into from
Closed

Conversation

teliosdev
Copy link
Contributor

This allows developers to use the #fetch method on a Rash, the idea being that it either raises a KeyError or yields if there is nothing that matched the given key. Also defines respond_to_missing? on the Rash class, since it was missing that.

On a side note, I keep saying find instead of fetch. I don't know why. 馃嵃

end

if block_given?
block.call(key)
Copy link
Member

Choose a reason for hiding this comment

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

Is this expected to yield on key and not on value?

@teliosdev teliosdev changed the title Allow #find access in Rash Allow #fetch access in Rash Jul 8, 2014
@teliosdev
Copy link
Contributor Author

In the fetch method, it calls all with the block; for the first element that all matches, it returns. If the block given to all isn't called at all, it will then try to determine a default value to return. If a block is given, it yields the key, and returns the value of the block. If an argument is given, it returns that argument (I use the constant NO_DEFAULT which is set to an instance of a generic object to make sure that if an argument is given, it'll use its value). Otherwise, it raises an error.

It follows the behavior of Hash#fetch.

@dblock
Copy link
Member

dblock commented Jul 9, 2014

I don't think I like this whole yield business. It's very hard to guess what it does. Generally yields match something and yield, not the other way around. This is the case of ActiveRecord or Mongoid or so many other libraries. IMO this should just be:

def fetch(key, &block)
    all(key) do |value|
        yield value if block_given?
    end
    nil
end

What am I missing?

@teliosdev
Copy link
Contributor Author

Let's say that you had a hash that you were using as a cache; if the cache missed, then you need to generate the data. Unfortunately, nil and false are both possible values; by default, if you try to access a key that doesn't have a pair in the hash table, then it returns nil (this is also why I use the NO_DEFAULT constant to denote the default value instead of nil), so you couldn't do hash[key] ||= generate_value(key). #fetch lets us customize this behaviour. We can pass it a value to return if the value doesn't exist; or, in case we need to generate the data on the fly (or even just set the value in the hash), we can pass it a block to do so. An example of something that might require this would be this.

ActiveSupport actually has something like this; see #find_or_create_by. The new instance is passed to the block, not the retrieved instance.

@dblock
Copy link
Member

dblock commented Jul 10, 2014

I understand now. You want to mimic behavior of a cache. However, I think that's too specific with a name that is too general for Hashie::Rash. I would be open to to a find_or_create_by method that is very explicitly doing what you'd expect it to do, and it would probably need to be built as an extension that can be mixed into a few hashes.

@teliosdev
Copy link
Contributor Author

The reason I chose #fetch as the name is because the built-in Hash class has the #fetch method; since Rash is meant to act like a Hash in terms of accessing, I thought that the #fetch method should follow the semantics of the Hash's #fetch.

This cannot be extracted into an extension well; the behavior of the #fetch method is too dependent on the class it's defined on to be able to do so. Mash even has a #fetch method that does something similar (I'm not sure if it follows the block semantics).

While I agree in that the name fetch isn't that descriptive of what the method does, the core API implements fetch in that manner; therefore, for the sake of understandability, I believe it should be named after the core API.

@teliosdev
Copy link
Contributor Author

I also want to point out that #fetch isn't just used for caching - you can use it in methods that accept a hash argument to determine values for keys, like in this example:

def some_method(options)
  prints = options.fetch(:prints, 1)
  body   = options.fetch(:body) { "Hello, #{options.fetch(:subject)}" }

  prints.times { puts body }
end

some_method(subject: "world")

Or, somewhere in the Rails repository, this example.

@dblock
Copy link
Member

dblock commented Jul 10, 2014

Ok, I am wrapping my head around this, thanks for being patient. I didn't realize Ruby Hash has these fetch semantics, I should have read it. So, it says "Returns a value from the hash for the given key. If the key can鈥檛 be found, there are several options: With no other arguments, it will raise an KeyError exception; if default is given, then that will be returned; if the optional code block is specified, then that will be run and its result returned."

This isn't quite what's implemented here, beginning with this NO_DEFAULT thing.

h = { "a" => 100, "b" => 200 }
h.fetch("a")                            #=> 100
h.fetch("z", "go fish")                 #=> "go fish"
h.fetch("z") { |el| "go fish, #{el}"}   #=> "go fish, z"

Can we get closer to these semantics?

@teliosdev
Copy link
Contributor Author

The purpose of the NO_DEFAULT was so that I could guarantee that the call was passed no other arguments. If, for example, I had used nil instead of NO_DEFAULT, and the call was passed nil to be the default value, then the call would act as if it wasn't passed an argument and would error. Hash#fetch is probably written in C, which would make it easier to determine if the second argument was passed (the function prototype would be something like VALUE Hash_fetch(int argc, VALUE* argv, VALUE self), and argc would be 2 if it was passed 2 arguments).

Looking at it now, I could probably use the spat operator and check the length of the array instead.

@dblock
Copy link
Member

dblock commented Jul 10, 2014

Is there any precedent for the way you did NO_DEFAULT? I would certainly prefer another solution, but I understand why you're doing this, thanks.

@@ -4,6 +4,7 @@
* [#179](https://github.com/intridea/hashie/pull/179): Mash#values_at will convert each key before doing the lookup - [@nahiluhmot](https://github.com/nahiluhmot).
* [#184](https://github.com/intridea/hashie/pull/184): Allow ranges on Rash to match all Numeric types - [@medcat](https://github.com/medcat).
* [#187](https://github.com/intridea/hashie/pull/187): Automatically require version - [@medcat](https://github.com/medcat).
* [#189](https://github.com/intridea/hashie/pull/189): Allow #fetch access in Rash - [@medcat](https://github.com/medcat).
Copy link
Member

Choose a reason for hiding this comment

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

Should probably just say "Added Rash#fetch".

@dblock
Copy link
Member

dblock commented Aug 17, 2014

Merged via ae4cab2, thanks.

@dblock dblock closed this Aug 17, 2014
jsonn pushed a commit to jsonn/pkgsrc that referenced this pull request Feb 6, 2015
## 3.4.0 (02/02/2014)

* [#271](hashie/hashie#271): Added ability to define defaults based on current hash - [@gregory](https://github.com/gregory).
* [#247](hashie/hashie#247): Fixed #stringify_keys and #symbolize_keys collision with ActiveSupport - [@bartoszkopinski](https://github.com/bartoszkopinski).
* [#249](hashie/hashie#249): SafeAssignment will now also protect hash-style assignments - [@jrochkind](https://github.com/jrochkind).
* [#251](hashie/hashie#251): Added block support to indifferent access #fetch - [@jgraichen](https://github.com/jgraichen).
* [#252](https://github.com/intridia/hashie/pull/252): Added support for conditionally required Hashie::Dash attributes - [@ccashwell](https://github.com/ccashwell).
* [#256](https://github.com/intridia/hashie/pull/256): Inherit key coercions - [@Erol](https://github.com/Erol).
* [#259](https://github.com/intridia/hashie/pull/259): Fixed handling of default proc values in Mash - [@Erol](https://github.com/Erol).
* [#260](https://github.com/intridia/hashie/pull/260): Added block support to Extensions::DeepMerge - [@Galathius](https://github.com/galathius).
* [#254](hashie/hashie#254): Added public utility methods for stringify and symbolize keys - [@maxlinc](https://github.com/maxlinc).
* [#261](hashie/hashie#261): Fixed bug where Dash.property modifies argument object - [@d_tw](https://github.com/d_tw).
* [#264](hashie/hashie#264): Methods such as abc? return true/false with Hashie::Extensions::MethodReader - [@Zloy](https://github.com/Zloy).
* [#269](hashie/hashie#269): Add #extractable_options? so ActiveSupport Array#extract_options! can extract it - [@ridiculous](https://github.com/ridiculous).
* Your contribution here.

## 3.3.2 (11/26/2014)

* [#233](hashie/hashie#233): Custom error messages for required properties in Hashie::Dash subclasses - [@Joss](https://github.com/joss).
* [#231](hashie/hashie#231): Added support for coercion on class type that inherit from Hash - [@gregory](https://github.com/gregory).
* [#228](hashie/hashie#228): Made Hashie::Extensions::Parsers::YamlErbParser pass template filename to ERB - [@jperville](https://github.com/jperville).
* [#224](hashie/hashie#224): Merging Hashie::Mash now correctly only calls the block on duplicate values - [@amysutedja](https://github.com/amysutedja).
* [#221](hashie/hashie#221): Reduce amount of allocated objects on calls with suffixes in Hashie::Mash - [@kubum](https://github.com/kubum).
* [#245](hashie/hashie#245): Added Hashie::Extensions::MethodAccessWithOverride to autoloads - [@Fritzinger](https://github.com/Fritzinger).

## 3.3.1 (8/26/2014)

* [#183](hashie/hashie#183): Added Mash#load with YAML file support - [@gregory](https://github.com/gregory).
* [#195](hashie/hashie#195): Ensure that the same object is returned after injecting IndifferentAccess - [@michaelherold](https://github.com/michaelherold).
* [#201](hashie/hashie#201): Hashie::Trash transforms can be inherited - [@FoboCasteR](https://github.com/fobocaster).
* [#189](hashie/hashie#189): Added Rash#fetch - [@medcat](https://github.com/medcat).
* [#200](hashie/hashie#200): Improved coercion: primitives and error handling - [@maxlinc](https://github.com/maxlinc).
* [#204](hashie/hashie#204): Added Hashie::Extensions::MethodOverridingWriter and Hashie::Extensions::MethodAccessWithOverride - [@michaelherold](https://github.com/michaelherold).
* [#205](http://github.com/intridea/hashie/pull/205): Added Hashie::Extensions::Mash::SafeAssignment - [@michaelherold](https://github.com/michaelherold).
* [#206](http://github.com/intridea/hashie/pull/206): Fixed stack overflow from repetitively including coercion in subclasses - [@michaelherold](https://github.com/michaelherold).
* [#207](http://github.com/intridea/hashie/pull/207): Fixed inheritance of transformations in Trash - [@FoboCasteR](https://github.com/fobocaster).
* [#209](http://github.com/intridea/hashie/pull/209): Added Hashie::Extensions::DeepFind - [@michaelherold](https://github.com/michaelherold).
* [#69](hashie/hashie#69): Fixed regression in assigning multiple properties in Hashie::Trash - [@michaelherold](https://github.com/michaelherold), [@einzige](https://github.com/einzige), [@dblock](https://github.com/dblock).

## 3.2.0 (7/10/2014)

* [#164](hashie/hashie#164), [#165](hashie/hashie#165), [#166](hashie/hashie#166): Fixed stack overflow when coercing mashes that contain ActiveSupport::HashWithIndifferentAccess values - [@numinit](https://github.com/numinit), [@kgrz](https://github.com/kgrz).
* [#177](hashie/hashie#177): Added support for coercing enumerables and collections - [@gregory](https://github.com/gregory).
* [#179](hashie/hashie#179): Mash#values_at will convert each key before doing the lookup - [@nahiluhmot](https://github.com/nahiluhmot).
* [#184](hashie/hashie#184): Allow ranges on Rash to match all Numeric types - [@medcat](https://github.com/medcat).
* [#187](hashie/hashie#187): Automatically require version - [@medcat](https://github.com/medcat).
* [#190](hashie/hashie#190): Fixed `coerce_key` with `from` Trash feature and Coercion extension - [@gregory](https://github.com/gregory).
* [#192](hashie/hashie#192): Fixed StringifyKeys#stringify_keys! to recursively stringify keys of embedded ::Hash types - [@dblock](https://github.com/dblock).

## 3.1.0 (6/25/2014)

* [#169](hashie/hashie#169): Hash#to_hash will also convert nested objects that implement to_hash - [@gregory](https://github.com/gregory).
* [#171](hashie/hashie#171): Include Trash and Dash class name when raising `NoMethodError` - [@gregory](https://github.com/gregory).
* [#172](hashie/hashie#172): Added Dash and Trash#update_attributes! - [@gregory](https://github.com/gregory).
* [#173](hashie/hashie#173): Auto include Dash::IndifferentAccess when IndiferentAccess is included in Dash - [@gregory](https://github.com/gregory).
* [#174](hashie/hashie#174): Fixed `from` and `transform_with` Trash features when IndifferentAccess is included - [@gregory](https://github.com/gregory).

## 3.0.0 (6/3/2014)

**Note:** This version introduces several backward incompatible API changes. See [UPGRADING](UPGRADING.md) for details.

* [#150](hashie/hashie#159): Handle nil intermediate object on deep fetch - [@stephenaument](https://github.com/stephenaument).
* [#146](hashie/hashie#146): Mash#respond_to? inconsistent with #method_missing and does not respond to #permitted? - [@dblock](https://github.com/dblock).
* [#152](hashie/hashie#152): Do not convert keys to String in Hashie::Dash and Hashie::Trash, use Hashie::Extensions::Dash::IndifferentAccess to achieve backward compatible behavior - [@dblock](https://github.com/dblock).
* [#152](hashie/hashie#152): Do not automatically stringify keys in Hashie::Hash#to_hash, pass `:stringify_keys` to achieve backward compatible behavior - [@dblock](https://github.com/dblock).
* [#148](hashie/hashie#148): Consolidated Hashie::Hash#stringify_keys implementation - [@dblock](https://github.com/dblock).
* [#149](hashie/hashie#149): Allow IgnoreUndeclared and DeepMerge to be used with undeclared properties - [@jhaesus](https://github.com/jhaesus).
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

2 participants