hamstergem / hamster

Efficient, Immutable, Thread-Safe Collection classes for Ruby
Other
1.89k stars 94 forks source link

added Hamster.from support (and specs) for Ruby core Structs (one-way only) #200

Closed Bill closed 9 years ago

Bill commented 9 years ago

I'm using Hamster with the AWS SDK for Ruby. That thing produces Structs not Hashes. (nested Struct/Array conglomerations).

This PR adds support for converting .from Structs. There is no conversion back: the conversion back produces Hash since there is no way to now we came from a Struct. Most libs I've seen will accept Hashes in place of their Structs so I think this lack of perfect symmetry will be fine.

Added two expectation pairs to the nested test suite and added an optional third one_way element to an expectation. If present, and truthy, then it elides the .to_ruby specs for that entry. This lets us add more one-way conversions if needed.

Bill commented 9 years ago

Sorry I closed this because I thought I'd put it against the wrong branch. But master is right.

alexdowad commented 9 years ago

ACK

alexdowad commented 9 years ago

Applied with some minor cleanup. Thanks!

Bill commented 9 years ago

Thank you for Hamster!

Now add comprehensions please.

alexdowad commented 9 years ago

@Bill, list comprehensions are definitely a powerful feature. But can you propose a syntax which the Ruby interpreter can parse?

Bill commented 9 years ago

Well @alexdowad used the wrong term there. As you say, comprehensions are nice. But what I had in mind was actually assoc-in. The family assoc-in, update-in and dissoc-in let us modify collections and collections of collections.

The assoc-in family is cool. And in researching it, I've found an interesting set of even more powerful transformation functions (mini-languages really). Instar is a really good example:

https://github.com/boxed/instar/

It not only unifies the assoc-in family under a single transform function, it also provides a "capture groups" feature that lets you tag places in your structure and then get at that tagged content in your transformation function.

This wouldn't need to be part of Hamster of course. But I thought you might find it interesting.

alexdowad commented 9 years ago

instar does look powerful, though I doubt that it is something which we would do in Ruby.

I'm missing what assoc-in gives you which Hamster's Vector#update_in and Hash#update_in don't. Could you explain?

Bill commented 9 years ago

Ahem, well, forgive me for being a Hamster noob. Vector#update_in and Hash#update_in look like they do exactly what I want.

On the other hand, it is kind of anomalous that the code for those two methods is identical. Coming from Clojure-land I didn't think to look in the individual classes for the function since the function operates on both types. Rather than maintaining two copies of the exact same code, I wonder if it would be worthwhile to factor those out. Maybe:

module Hamster
  def update_in(c,*key_path, &block)
    if key_path.empty?
      raise ArgumentError, "must have at least one key in path"
    end
    key = key_path[0]
    if key_path.size == 1
      new_value = block.call(c.get(key))
    else
      value = c.fetch(key, EmptyHash)
      new_value = update_in(value,*key_path[1..-1], &block)
    end
    c.put(key, new_value)
  end
  module_function :update_in
end

It would also allow us to have a single spec instead of the two specs.

I just noticed that there is no Vector#put. Currently Vector#update_in calls set. It looks like Vector#set could be renamed Vector#put since it does the same thing to the Vector that Hash#put does to a Hash.

Did I get that right?

alexdowad commented 9 years ago

@Bill, thanks for this suggestion. Comments from other maintainers?

Bill commented 9 years ago

I refined my proposal from yesterday. See above.

dubek commented 9 years ago

@Bill 's approach looks good to me. It should be in some "generic" file similar to hamster/nested.rb (but not there). At first step the current implementation of Hash#update_in and Vector#update_in can simply call the new suggested method. At a later stage we can deprecate and then erase them in favour of using only Hamster.update_in(myhash, "a", "b", "c") { |val| val + 5 } .

I think in the past we had Vector#put and we removed it as part of the big clean-up prior to releasing 1.0... oops!

dubek commented 9 years ago

Hmmm, I think about it again. In Ruby the methods are "duplicated" for each class. So you have Array#sort and Hash#sort whose implementations are probably very similar - yet Ruby doesn't define a global sort(my_data_structure) function...

So maybe we should improve code reuse but leave the interface as Hash#update_in and Vector#update_in ?

(sorry for contradicting myself)

Bill commented 9 years ago

Yeah @dubek there is a tension here. I now see that Vector and Hash use Enumerable's sort and sort_by. So if we did similarly for update_in we might stick it in a module and then include that module in both Hash and Array. Then a single update_in_spec could test various nested structures.

That would be an improvement. That, plus updating the README to prominently feature update_in because it's a killer feature.

On the other hand, the mutual recursion of such a solution seems kind of hokey to me. Hashes messaging Arrays messaging Hashes… I buy in to Rich Hickey's argument about separation of function from structure and I think this is a prime example. But I'm all for improvement over perfection.

alexdowad commented 9 years ago

For better or worse, Ruby is an class-based OO language which relies heavily on class-based method dispatch. All the core Ruby classes work this way. Therefore, I suggest that we keep #update_in on Vector and Hash. But extracting the code into a module is a good idea.

@Bill, would you like to code up a PR? Adding to the README at the same time would be good.

Bill commented 9 years ago

I'll do it @alexdowad !

Bill commented 9 years ago

BTW update_in() didn't really do exactly what I originally wanted (in my app). I ended up implementing my own beefed-up version of update_in() (called update_having()) and the whole thing is described over here:

http://memerocket.com/2015/11/01/hamstar-transforms-immutable-ruby-collections-better/

Super fun. And thanks especially to @alexdowad for all the help!

dubek commented 9 years ago

Nice post. Your new module name is really confusing...

Besides supporting [:key, :expected_value] and "*" (kleene star) you might want to support procs/lambdas. So the kleene star can be implemented by:

Hamstar.update_having(x, ->{true}, :name)

and the key-expected value can be implemented as:

Hamstar.update_having(x, ->(k,v){ v[:name] == "Pat" }, :name)

and you maybe can extend to stuff like "update having an even-index" etc.

Hamstar.update_having(x, ->(k,v){ k.even? }, :name)

(I guess this discussion should move to the Hamstar repo...)

Bill commented 9 years ago

Nice post.

Thanks @dubek.

Your new module name is really confusing...

Indeed. It is kind of cutesy and perhaps too close to "Hamster". Then again, it is just defining a single function, so naming it in relation to Hamster might not be unwarranted.

…you might want to support procs/lambdas…

Indubitably. The latest version, v0.0.4 adds Proc matcher support. Thanks for the suggestion. The implementation of association matching isn't quite as clean (yet) but it's getting there.

Bill commented 9 years ago

There, now it's cleaned up in v0.0.7. The Proc no longer requires the superfluous third parameter.