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

Already on GitHub? Sign in to your account

Add support for read_multi (closes #379) #417

Closed
wants to merge 30 commits into
from

Conversation

Projects
None yet
6 participants
Contributor

ahlatimer commented Mar 7, 2013

With the current caching approach in rabl, each item in the cached is read separately. This can have serious performance implications when the cache is accessed over a network. Fortunately, many caching libraries implement a read_multi method that can fetch multiple keys simultaneously. This pull requests adds read_multi support to rabl.

The way rabl was previously architected, there was no way to determine what all of the cache directives were in a child element (i.e., a call to glue, child, extends, or node) without also rendering that element at the same time. That is, applying the child element to the parent view and rendering it were coupled. The child element could retrieve itself from the cache, but there was no way to apply the cache directives to all children elements and then use read_multi to fetch all of the keys. The majority of the changes outside the MultiBuilder class relate to this.

Now, instead of calling render on an engine, apply must be called first. apply does everything render did previously, short of actually rendering the template. This allows the parent engine, via the builder it constructs, to generate all the cache keys for all of the child elements. With those cache keys, we can call read_multi.

This was previously discussed in #379

I realize this is a somewhat wide-reaching change, so I'm more than happy to change things as necessary to get this merged. I don't see a way of getting around the changes to render, however.

ahlatimer added some commits Mar 4, 2013

@ahlatimer ahlatimer When rendering a collection, attempt to use read_multi to fetch the c…
…ached items in parallel, potentially decreasing time to response.
95875ee
@ahlatimer ahlatimer Refactor read_multi support to be more general.
Most changes are consolidated to the engine and builder classes.
High-level, builder now defers the actual rendering (to hash) until
later on, allowing the engine a chance to evaluate all of the various
extends and so forth that may contain a cache directive. Once all of the
cache directives are found, it can generate all of the cache keys and fetch
them simultaneously using read_multi.
f8ebbb6
@ahlatimer ahlatimer Engine#read_multi appropriately builds the resulting hash from the ca…
…ched elements. Add configuration option to disable read_multi.
0af74b7
@ahlatimer ahlatimer Comment Builder engine compilation loop. 249bf0d
@ahlatimer ahlatimer Clean up some left-over logic after moving to the delayed rendering m…
…ethod for support read_multi.
4f33bcd
@ahlatimer ahlatimer Fix broken builder test after delayed render changes. f2fa869
@ahlatimer ahlatimer Only block converting object_to_engine in partials.rb if the object i…
…s nil. The engine knows how to handle objects that aren't really objects or collections.
b20425f
@ahlatimer ahlatimer Refactor read_multi logic into its own class. 58b5d5e
@ahlatimer ahlatimer Add tests for MultiBuilder. 8024520
@ahlatimer ahlatimer Fix a missed local -> instance variable conversion when refactoring M…
…ultiBuilder.
a415b82
@ahlatimer ahlatimer If an attribute is a collection and responds to as_json, go ahead and…
… call as_json on it. Otherwise, the JSON dump can have issues processing AR models and such.
b056060
@ahlatimer ahlatimer Reset the engines array after turning them into a hash, otherwise the…
… same object is used if a builder is reused to build a collection.
c72c0f6
@ahlatimer ahlatimer Don't attempt to call render on what's returned from partial_as_engin…
…e since it may be nil or an empty array.
12949d5
@ahlatimer ahlatimer Since Builder#node doesn't generate an engine, move it to the compile…
…_hash method rather than compile_engines.
13d9d74
@ahlatimer ahlatimer In MultiBuilder, handle the case when a builder does not have any eng…
…ines.
937817d

micho commented Mar 27, 2013

We have the same issue! In our install, we have a latency of 4 ms to memcached, meaning rendering 500 cached objects can actually be slower than rendering new ones.

Your fork looks great (open source rocks), we are now testing this with our codebase and might run it in production if that works well. We'll do a code review as well.

Contributor

ahlatimer commented Mar 27, 2013

We've been using this in production for a week or so now, and we haven't had any issues with it. If you run across any, let me know, and I'll be sure to take a look!

micho commented Mar 27, 2013

Awesome! We were ready to start coding the exact same thing, so this is great timing.

Owner

nesquena commented Mar 27, 2013

@micho If you can try this fork and confirm it works in your case that might be helpful. I will review the changes soon and do testing on my end as well.

@micho micho commented on the diff Mar 27, 2013

lib/rabl/configuration.rb
@@ -71,6 +72,7 @@ def initialize
@view_paths = []
@cache_engine = Rabl::CacheEngine.new
@perform_caching = false
+ @use_read_multi = true
@micho

micho Mar 27, 2013

So it defaults to true. Should this branch update the README for the configurable options?

Owner

nesquena commented Mar 27, 2013

Unrelated, what are you using to get that really nicely formatted stack trace?

micho commented Mar 27, 2013

It's better_errors, enjoy :) https://github.com/charliesome/better_errors

Owner

nesquena commented Mar 27, 2013

Awesome, those really are better errors. Will start using that. In the meantime, thanks for testing this fork. Looks like it already caught an issue. The more people that can try this before it's merged the better. Since it is kind of a large change.

micho commented Mar 28, 2013

We're using rails 3.1.11. I'll put more time into it tomorrow, so if you could give me some answers it'll go a long way into helping me fix things!

micho commented Apr 1, 2013

Great, I got it working on one machine. So far, I had to do the diff below to get one of my views working. Apparently using node + partial is different than using a simple extends.

-node do |task|
-  if params[:no_comments] || params[:simple_index]
-    partial('api_v2/tasks/simple', :object => task)
-  else
-    partial('api_v2/tasks/base', :object => task)
-  end
+if params[:no_comments] || params[:simple_index]
+  extends 'api_v2/tasks/simple'
+else
+  extends 'api_v2/tasks/base'
 end

I'm still working on getting our full spec suite working with this fork.

Owner

nesquena commented Apr 2, 2013

Awesome, thanks for the update. Appreciate your work on this.

micho commented Apr 2, 2013

Update: it's all working well now, pretty much out of the box after a few tweaks to our repo.

There is still a possible optimization, see below:

Cache read_multi: ["rabl/base/projects/4-20130402084231//hash", "rabl/base/projects/3-20130402084231//hash", "rabl/base/projects/2-20130402084231//hash", "rabl/base/projects/1-20130402131549//hash"]
# Notice we already know we don't have this key, so Rails.cache.fetch is making one call too many
Cache read: rabl/base/projects/1-20130402131549//hash
Cache generate: rabl/base/projects/1-20130402131549//hash
Cache write: rabl/base/projects/1-20130402131549//hash
Owner

nesquena commented Apr 2, 2013

Awesome, it's nice to have two of you able to test this in your apps. There's a lot of changes here I want to look through and review but if we can fix those last tests and everything seems to be working then I will merge this for the next bigger version release. Thanks guys

Contributor

ahlatimer commented Apr 2, 2013

How are you getting rake test:full to run? rake test:setup test:fixtures fails for every fixture test due to missing dependencies (Padrino and Rails, though I suspect there are more once I get past that...). When I run rake test, everything is green.

I'll take a look at the additional optimization later today or tomorrow. The problem is, is that if you miss on the cache read_multi, it runs through the entirety of the code that was already present that builds those partials, including the cache fetch. The cache generate/write we obviously still want to happen, but the read can be skipped. You also still want to hit the cache where there is cache directive at the top of the template getting generated (so it's not part of a MultiBuilder that's going to do a read_multi) or you have read_multi support turned off. Since the cache generate/write dominated the amount of time over the read, and we still saw a pretty significant improvement even if it was a little stupid on cache miss, I hadn't bothered to really think through a good way to fix it.

Contributor

ahlatimer commented Apr 3, 2013

I just pushed a commit that should take care of the double cache read on miss problem. Let me know if that works for you, @micho.

just making sure: is @cache_read_on_render a typo, or intended behavior?

Owner

ahlatimer replied Apr 3, 2013

Good catch. Typo.

Owner

ahlatimer replied Apr 3, 2013

Though this commit should still give you the behavior you want since I never do engine.cache_read_on_render = true. It'll either evaluate to true, or, when you set @_cache_read_on_render to false, #cache_read_on_render will return nil, which still evaluates to false.

awesome

micho commented Apr 3, 2013

@ahlatimer I found the reason for my previous errors: I was using the ahlatimer/multi_get branch thinking that was the one, not realizing you were using master. Sorry for the noise I introduced, I updated/deleted the noisy comments for future reviewers.

Your last commit looks great, thanks for the update. All our tests are passing now and we are putting this in production tomorrow to start testing.

About tests:

Contributor

ahlatimer commented Apr 3, 2013

This is what I get when I run rake test:setup test:full https://gist.github.com/ahlatimer/5304997

Owner

nesquena commented Apr 3, 2013

@ahlatimer Looks like the dependencies aren't installing correctly. Does the Rakefile look like this for the setup task? https://github.com/nesquena/rabl/blob/master/Rakefile#L36 I added some changes relatively recently to make the setup more reliable.

Contributor

ahlatimer commented Apr 3, 2013

@nesquena Nope. Rakefiles are different. I'll merge in your master and try again.

Owner

nesquena commented Apr 3, 2013

OK yeah I suspect that will fix it.

Contributor

ahlatimer commented Apr 3, 2013

I've updated my fork to the latest origin/HEAD. The specs run now save for Rails 2 (thanks), but I'm not sure if that one is my fault. Probably, but it just gives me an error about rdoc, so it's hard to tell.

There are three failing specs in Rails 3.2. I haven't dug into them yet, but I suspect it's a problem with the spec itself (at least two are testing caching), not with any of the actual code.

Contributor

ahlatimer commented Apr 3, 2013

For reference, this is what I get when running the rails 2 fixtures: https://gist.github.com/ahlatimer/5305465

Owner

nesquena commented Apr 3, 2013

Just for fun can you merge the last commit from master, I locked rails2 to an older version of rake which should fix that error. I am not able to reproduce that error.

Contributor

ahlatimer commented Apr 4, 2013

Yep, that fixed it. I get a warning from the way rdoc is getting required, but it's not a big deal. All green on Rails 2.

Still haven't figured out what's going on in the Rails 3.2 specs. They do seem to be legitimately broken, but I'm not sure why. I know the use-case it's testing works in 3.2 because I'm using it similarly in a 3.2 app…

Anyhow, I'll keep working on it. Hopefully have a fix in later today.

Contributor

ahlatimer commented Apr 4, 2013

Everything is green.

I broke it when I did the second optimization that stopped rabl from making a second cache read on a read_multi miss. Instead of returning what was just written to the cache in cache_results, it was returning nothing. This is the whole fix. It's always something painfully obvious in hindsight…

I also cleaned some of the logic up while trying to figure out what was going on. There was some stuff that wasn't necessary.

Owner

nesquena commented Apr 4, 2013

Awesome work @ahlatimer thanks so much for putting this together. @micho does this latest version work for you as well. I will try to review and merge this in this weekend.

Contributor

ahlatimer commented Apr 4, 2013

No problem. Let me know if you come across anything that needs some work.

masylum commented Apr 8, 2013

We've been trying this branch and everything seemed to work fine, but we just realized that we are having some issues with our JSON serialized attributes: Its converting the boolean attributes to strings.

attributes :id,
           :name,
           :settings

Expected output:

{id: 1, name: 'paco', settings: {is_pro: false}}

Actual output:

{id: 1, name: 'paco', settings: {is_pro: "false"}}
Contributor

ahlatimer commented Apr 8, 2013

@masylum Do you get the expected output on nesquena/master? I just tried to reproduce what you're seeing, and I get the expected output on my branch and on nesquena/master. It could be that I missed a necessary step in reproducing it in my tests, though, but I don't know of any immediately obvious reason why any of my changes would change the way booleans are rendered; the attributes code is untouched and outside the scope of read_multi.

Does this happen in development with caching disabled or only in environments that having caching enabled?

Edit: clarified my attempts at reproduction.

Owner

nesquena commented Apr 8, 2013

Yes thanks for trying @masylum, hopefully we can track down why it appears your booleans are being incorrectly cast; the more people to give this branch a whirl, the better.

masylum commented Apr 9, 2013

I'm trying to add a failing spec but I'm stucked:

diff --git a/test/engine_test.rb b/test/engine_test.rb
index 795447c..fd1d4df 100644
--- a/test/engine_test.rb
+++ b/test/engine_test.rb
@@ -182,6 +182,16 @@ context "Rabl::Engine" do
         JSON.parse(template.render(scope))
       end.equals JSON.parse("{\"user\":{\"name\":\"irvine\"}}")

+      asserts "that it handles nested attrbutes" do
+        template = rabl %{
+          object @user
+          attribute :nested
+        }
+        scope = Object.new
+        scope.instance_variable_set :@user, User.new()
+        JSON.parse(template.render(scope))
+      end.equals JSON.parse("{\"user\":{\"nested\":{\"bool\":false}}}")
+
       asserts "that it can add attribute under a different key name through :as" do
         template = rabl %{
           object @user
diff --git a/test/models/user.rb b/test/models/user.rb
index 5dfc1a7..e2fab79 100644
--- a/test/models/user.rb
+++ b/test/models/user.rb
@@ -1,6 +1,6 @@
 unless defined?(User)
   class User
-    attr_accessor :age, :city, :name, :first, :float, :hobbies
+    attr_accessor :age, :city, :name, :first, :float, :hobbies, :nested

     DEFAULT_AGE      = 24
     DEFAULT_CITY     = 'irvine'
@@ -8,6 +8,7 @@ unless defined?(User)
     DEFAULT_FIRST    = 'bob'
     DEFAULT_FLOAT    = 1234.56
     DEFAULT_HOBBIES  = ['Photography']
+    DEFAULT_NESTED     = {:bool => false}

     def initialize(attributes={})
       self.age     = attributes[:age]     || DEFAULT_AGE
@@ -15,6 +16,7 @@ unless defined?(User)
       self.name    = attributes[:name]    || DEFAULT_NAME
       self.first   = attributes[:first]   || DEFAULT_FIRST
       self.float   = attributes[:float]   || DEFAULT_FLOAT
+      self.nested  = attributes[:nested]  || DEFAULT_NESTED
       self.hobbies = (attributes[:hobbies] || DEFAULT_HOBBIES).map { |h| Hobby.new(h) }
     end
   end

fails saying:

ERROR
 Rabl::Engine with defaults #attribute asserts that it adds an attribute or method to be included in output => undefined method `bool' for #<User:0x007fd8c9951010> occured
 at /Users/masylum/Sites/rabl/test/engine_test.rb:192:in `block (4 levels) in <top (required)>'
at /Users/masylum/.rbenv/versions/1.9.3-p0/lib/ruby/gems/1.9.1/gems/tilt-1.3.6/lib/tilt/template.rb:77:in `render'
at /Users/masylum/Sites/rabl/lib/rabl/template.rb:15:in `evaluate'
at /Users/masylum/Sites/rabl/lib/rabl/engine.rb:45:in `render'
at /Users/masylum/Sites/rabl/lib/rabl/engine.rb:297:in `cache_results'
at /Users/masylum/Sites/rabl/lib/rabl/engine.rb:45:in `block in render'
at /Users/masylum/Sites/rabl/lib/rabl/engine.rb:80:in `to_json'
at /Users/masylum/Sites/rabl/lib/rabl/engine.rb:64:in `to_hash'
at /Users/masylum/Sites/rabl/lib/rabl/builder.rb:22:in `build'
at /Users/masylum/Sites/rabl/lib/rabl/builder.rb:201:in `cache_results'
at /Users/masylum/Sites/rabl/lib/rabl/builder.rb:22:in `block in build'
at /Users/masylum/Sites/rabl/lib/rabl/builder.rb:69:in `compile_hash'
at /Users/masylum/Sites/rabl/lib/rabl/builder.rb:69:in `each_pair'
at /Users/masylum/Sites/rabl/lib/rabl/builder.rb:70:in `block in compile_hash'
at /Users/masylum/Sites/rabl/lib/rabl/builder.rb:116:in `attribute'
at /Users/masylum/Sites/rabl/lib/rabl/helpers.rb:19:in `data_object_attribute'
at /Users/masylum/Sites/rabl/lib/rabl/helpers.rb:73:in `is_collection?'
at /Users/masylum/Sites/rabl/lib/rabl/helpers.rb:13:in `data_object'

masylum commented Apr 9, 2013

And I can confirm that after reverting to 0.7.10 the issue is solved.

Contributor

ahlatimer commented Apr 9, 2013

@masylum The reason I asked you about using nesquena/master was because, in 0.8.0, rabl switched from using multijson to oj. If you weren't using oj before, there's a chance that whatever version of oj you happened to pull down has a bug that causes booleans to be converted to strings. I don't know that to be the case, but that seemed more likely than something in my branch, since, as I said previously, my branch doesn't operate at the attribute level.

My branch is fairly up-to-date with master, so that would narrow down the problem to just my code if this problem goes away when you switch to nesquena/master. If it persists, this is a bug in rabl (or elsewhere) and should be addressed in another issue.

micho commented Apr 13, 2013

Just a note: @masylum and I work together at Teambox, so whatever issues we report might be specific to our project.

micho commented May 7, 2013

Ok, I managed to reproduce a case where this fails. In the simplest version, my code looks like:

# Model
class User < ActiveRecord::Base
  # I'm not even using a real attribute, but this also happens with a serialized field
  def something
    { a: false, b: "false" }
  end
end

# Controller
class ApiV2::UsersController < ActionController::Base
  def current
    @user = User.new
  end
end

# View
object @user
attributes :something

And this returns { something: { a: "false", b: "false" } } (notice the coercion of bool to string) in this branch, but { something: { a: false, b: "false" } } (correct) in rabl 0.8.0 or 0.8.2.

>> Rabl.configuration
=> #<Rabl::Configuration:0x007fb74010a960 @include_json_root=false, @include_child_root=false, @include_msgpack_root=true, @include_plist_root=true, @include_xml_root=false, @include_bson_root=true, @enable_json_callbacks=true, @bson_check_keys=false, @bson_move_id=false, @json_engine=nil, @msgpack_engine=nil, @bson_engine=nil, @plist_engine=nil, @xml_options={}, @cache_sources=false, @cache_all_output=false, @escape_all_output=false, @view_paths=[], @cache_engine=#<Rabl::CacheEngine:0x007fb74010a8c0>, @perform_caching=false>

This was with ahlatimer/rabl:master, Rails 3.1.11 and caching disabled. Interestingly enough, it doesn't happen with rabl 0.8.2.

Contributor

ahlatimer commented May 7, 2013

If you're getting it with my branch but not 0.8.2, it's definitely something I did. I'll take a look at this in the next few days when I get a chance. Thanks for getting a simple repro case.

Contributor

ahlatimer commented May 14, 2013

I still can't reproduce this. I get the correct output on my branch using Rails 3.2.13 and yajl 1.1.0. Outside of some difference that's manifesting when going back and forth between my branch and 0.8.2 for you guys, I'm at a loss.

This is the only commit that touched the attribute stuff, but it's relying on as_json, which should just convert things to a hash.

Collaborator

DouweM commented Jun 9, 2013

Any update on this? Have you been using this in production without problems? How near is a merge?

I would love this functionality, but I'm hesitant to just deploy it in production at this stage of the pull request.

Contributor

ahlatimer commented Jun 9, 2013

We've been using it in production for about 2 months so far, and I haven't come across any issues with it.

micho commented Jun 21, 2013

I have tested with production code and it worked well, but I haven't deployed it because we haven't had the time to test it thoroughly.

Collaborator

DouweM commented Jun 21, 2013

I'll give it a shot this weekend; thanks guys. @nesquena I think it's time for a merge!

Owner

nesquena commented Jun 21, 2013

Yeah I know will merge soon, @DouweM let me know how it works out for you.

@DouweM DouweM and 1 other commented on an outdated diff Jun 24, 2013

lib/rabl/cache_engine.rb
@@ -21,5 +21,18 @@ def fetch(key, cache_options, &block)
end
end
+ def write(key, value, options={})
+ if defined?(Rails)
+ Rails.cache.write(key, value, options)
+ end
+ end
+
+ def read_multi(keys, cache_options = {})
+ if defined?(Rails)
+ Rails.cache.read_multi(keys, cache_options)
@DouweM

DouweM Jun 24, 2013

Collaborator

#read_multi takes a list of keys, not an array. See the Rails source and Dalli source

@DouweM

DouweM Jun 24, 2013

Collaborator

This has been fixed in my fork.

@ahlatimer

ahlatimer Jun 24, 2013

Contributor

Interesting that it still worked when passed an array instead of a splat. If you open a pull request against my fork, I'll merge it.

@DouweM

DouweM Jun 24, 2013

Collaborator

Yeah, very strange. There was a similar issue with multi_fetch_fragments. I'll send a pull request.

@DouweM

DouweM Jun 24, 2013

Collaborator

It wasn't working with an array for me, btw. I was seeing stuff like:

Cache read_multi: [["rabl/etc1//hash", "rabl/etc2//hash"]]

Dalli would interpret that array-key as "rabl/etc1//hash/rabl/etc2//hash", which wouldn't return any results.

@ahlatimer

ahlatimer Jun 24, 2013

Contributor

I guess it's a Dalli version issue since it has worked for us without a problem for a few months now. Oh well. Thanks for noticing and making the fix!

Collaborator

DouweM commented Jun 24, 2013

This breaks the following:

object @item

node do |item|
  stuff = item.things + items.others

  child stuff => :stuff do
    attributes :id, :name
  end
end

This is of course a contrived example, but we do actually use this in our codebase.

This is useful because it gives you a reference to the item we're rendering for, when @item can't be used because we're, say, rendering through a collection @items; extends "_item".

This isn't intended behavior exactly, as the readme only gives examples like the following, where the block is expected to return a hash:

node do |u|
  { :full_name => u.first_name + " " + u.last_name }
  # => { full_name : "Bob Johnson" }
end

It is a neat side-effect feature though, that we currently depend on in our app.

However if you, @nesquena, think this shouldn't be supported at all, I will happily move our equivalent of stuff = item.things + item.others to a method on item.

Another way to accomplish the same thing and also depends on private API, is to just use item = root_object. This will probably break less quickly, so I opted for this solution.

Collaborator

DouweM commented Jun 24, 2013

EDIT: Disregard most of this.

It seems to break a lot more!

I have the following setup:

# items/index.rabl
collection @items

extends "items/_item"
# items/_item.rabl
object @item
cache [root_object, "v1"] # I can't do `cache [@item, "v1"]`, because @item is nil when rendered through a collection view.

attribute :id

child things: :things do
  extends "things/_thing"
end
# things/_thing.rabl
object @thing
cache [root_object, "v1"]

attribute :id

With caching enabled it doesn't seem to go into items/_item at all, the array in the resulting JSON is empty even though there are items.

With caching disabled, it does go into items/_item, but not things/_thing, even though the item has things.

Names etc are fake, but this is the behavior I'm seeing!

EDIT: Looks like my item didn't actually have any things, so it not going into things/_thing is actually right. Allow me to investigate some more.

Contributor

ahlatimer commented Jun 24, 2013

If you can write up some breaking specs, I'd really, really appreciate it.

Collaborator

DouweM commented Jun 24, 2013

Yeah, I'm looking into that. I commented a bit too soon.

Collaborator

DouweM commented Jun 24, 2013

Looks like the issue is actually just this:

# items/index.rabl
collection @items

extends "items/_item"
# items/_item.rabl
object @item

attribute :id

Note that neither file has a cache directive. In this case, when caching is enabled, rendering items/index.rabl will just return an empty array []: it won't actually render any of the items.

Collaborator

DouweM commented Jun 29, 2013

I just started looking into fixing the bug myself, but I'm having a hard time completely grasping the way Builder, Engine and MultiBuilder work together. The entire thing seems like a bit of a mess to me... I haven't given up yet, but it's probably faster if you would fix it, seeing how you know exactly how this thing works :-)

Collaborator

DouweM commented Jun 29, 2013

Fixed! @ahlatimer, check out ahlatimer/rabl#2.

@ahlatimer ahlatimer Merge pull request #2 from DouweM/master
Fix bug causing collections with non-cached views not being rendered when caching is enabled.
f310bbc
Collaborator

DouweM commented Jul 2, 2013

We're now running this fork in production, and so far we haven't seen any issues and are seeing a small but noticeable jump in performance.

Owner

nesquena commented Jul 2, 2013

Good to know I will review your fork of @ahlatimer fork and since there's been several confirmations hopefully push a pre-release gem with those changes this week

Collaborator

DouweM commented Jul 2, 2013

All of my fixes and changes have been merged into @ahlatimer's fork, so you can just check this pull request.

Owner

nesquena commented Jul 2, 2013

Ah ok great thanks

Collaborator

DouweM commented Jul 12, 2013

Found another bug, fixed in ahlatimer/rabl#3.

Owner

nesquena commented Jul 12, 2013

Thanks!

ahlatimer and others added some commits Jul 13, 2013

@ahlatimer ahlatimer Merge pull request #3 from DouweM/master
Merge #extends into hash before #node etc. to allow the former to be overridden.
a108de0
@DouweM DouweM Don't choke on symbol attribute values. 4ed2ad4
@ahlatimer ahlatimer Merge pull request #4 from DouweM/master
Don't choke on symbol attribute values.
87e68db

any progress towards merging this?
/cc @ahlatimer

Contributor

ahlatimer commented Apr 22, 2014

I get the feeling that this is so far out of date by now that I'm not sure it's even worth attempting it. We've just been relying on my fork, which sucks, since we don't get any updates to the mainline rabl branch.

I've already updated this several times to keep it inline with master in the hopes of getting it merged. It wasn't merged, nor was any path towards getting it merged ever suggested, so that's just the state I've settled on.

If anyone else wants to pick it up to see if we can actually make some progress towards getting it merged, I'd fully support that. I'm not sure I'm going to be the one carrying that, though.

Collaborator

DouweM commented Apr 22, 2014

I don't mind helping getting this ready to be merged, but I can't promise to be able to spend a lot of time on this either. It's a pity this wasn't merged sooner; that would have been a lot easier.

Owner

nesquena commented Apr 22, 2014

This was such a large change to the codebase, i never got around to being able to merge. It's my bad, sorry about that. If you could bring it up to date and get it in a mergeable state again and ping back here I would merge it faster this time and release a pre gem update and test it on a project in production. —
Nathan Esquenazi
CodePath Co-founder
http://thecodepath.com

On Tue, Apr 22, 2014 at 12:21 PM, Douwe Maan notifications@github.com
wrote:

I don't mind helping getting this ready to be merged, but I can't promise to be able to spend a lot of time on this either. It's a pity this wasn't merged sooner; that would have been a lot easier.

Reply to this email directly or view it on GitHub:
#417 (comment)

@DouweM DouweM referenced this pull request Oct 8, 2014

Merged

Add support for read_multi #585

@nesquena nesquena closed this Oct 19, 2014

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