Skip to content

Commit

Permalink
Fixed bc5896e, and added test case for the caching bug it originally …
Browse files Browse the repository at this point in the history
…introduced.
  • Loading branch information
josh committed Jul 23, 2008
1 parent e0db925 commit 55adaa2
Show file tree
Hide file tree
Showing 4 changed files with 13 additions and 6 deletions.
3 changes: 3 additions & 0 deletions actionpack/lib/action_view/base.rb
Original file line number Diff line number Diff line change
Expand Up @@ -332,6 +332,9 @@ def pick_template(template_path)
end
end

extend ActiveSupport::Memoizable
memoize :pick_template

private
# Renders the template present at <tt>template_path</tt>. The hash in <tt>local_assigns</tt>
# is made available as local variables.
Expand Down
9 changes: 5 additions & 4 deletions actionpack/lib/action_view/partials.rb
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,8 @@ module ActionView
#
# As you can see, the <tt>:locals</tt> hash is shared between both the partial and its layout.
module Partials
extend ActiveSupport::Memoizable

private
def render_partial(partial_path, object_assigns = nil, local_assigns = {}) #:nodoc:
local_assigns ||= {}
Expand Down Expand Up @@ -129,14 +131,12 @@ def render_partial_collection(partial_path, collection, partial_spacer_template

local_assigns = local_assigns ? local_assigns.clone : {}
spacer = partial_spacer_template ? render(:partial => partial_spacer_template) : ''
_paths = {}
_templates = {}

index = 0
collection.map do |object|
_partial_path ||= partial_path || ActionController::RecordIdentifier.partial_path(object, controller.class.controller_path)
path = _paths[_partial_path] ||= find_partial_path(_partial_path)
template = _templates[path] ||= pick_template(path)
path = find_partial_path(_partial_path)
template = pick_template(path)
local_assigns[template.counter_name] = index
result = template.render_partial(self, object, local_assigns, as)
index += 1
Expand All @@ -153,5 +153,6 @@ def find_partial_path(partial_path)
"_#{partial_path}"
end
end
memoize :find_partial_path
end
end
2 changes: 1 addition & 1 deletion actionpack/lib/action_view/renderable.rb
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ def #{render_symbol}(local_assigns)
# The template will be compiled if the file has not been compiled yet, or
# if local_assigns has a new key, which isn't supported by the compiled code yet.
def recompile?(symbol)
!(frozen? && Base::CompiledTemplates.method_defined?(symbol))
!(ActionView::PathSet::Path.eager_load_templates? && Base::CompiledTemplates.method_defined?(symbol))
end
end
end
5 changes: 4 additions & 1 deletion actionpack/test/template/compiled_templates_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -30,9 +30,12 @@ def test_compiled_template_will_not_be_recompiled_when_rendered_with_identical_l
assert_equal "Hello world!", render("test/hello_world.erb")
end

def test_compiled_template_will_be_recompiled_when_rendered_if_template_is_outside_cache
def test_compiled_template_will_always_be_recompiled_when_eager_loaded_templates_is_off
ActionView::PathSet::Path.expects(:eager_load_templates?).times(4).returns(false)
assert_equal 0, @compiled_templates.instance_methods.size
assert_equal "Hello world!", render("#{FIXTURE_LOAD_PATH}/test/hello_world.erb")
ActionView::Template.any_instance.expects(:compile!).times(3)
3.times { assert_equal "Hello world!", render("#{FIXTURE_LOAD_PATH}/test/hello_world.erb") }
assert_equal 1, @compiled_templates.instance_methods.size
end

Expand Down

12 comments on commit 55adaa2

@michaelklishin
Copy link
Contributor

Choose a reason for hiding this comment

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

so… this pretty much means you never run this extremely useful piece of code you added to Rails core, correct?

@josh
Copy link
Contributor Author

@josh josh commented on 55adaa2 Jul 24, 2008

Choose a reason for hiding this comment

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

What never runs?

@josh
Copy link
Contributor Author

@josh josh commented on 55adaa2 Jul 24, 2008

Choose a reason for hiding this comment

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

If you talking about “testing”, it caused a problem in development mode with cache reloading. We didn’t exactly have test coverage for that at the time.

@michaelklishin
Copy link
Contributor

Choose a reason for hiding this comment

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

looks like at the moment in time bc5896e was committed with this problem no production applications were using this uber cool mezmorize feature. How come? It’s so valuable and beats ||= every possible way. Josh, can you explain?

@nbibler
Copy link
Contributor

Choose a reason for hiding this comment

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

I don’t know if it’s just early or what, but I can’t tell if this is supposed to be sarcasm or not. :-\

@msheakoski
Copy link

Choose a reason for hiding this comment

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

@michaelklishin: But without abstracting ||= users would have to use the actual Ruby language. Yuck! :)

@Roman2K
Copy link

Choose a reason for hiding this comment

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

@GMFlash & michaelklishin:

What do you have against “memoize”? It doesn’t prevent one from still using ||=, it’s just another way of doing the same thing, a “more declarative” way as stated in the documentation of “memoize”.

And it has its pluses: it makes the code of the memoized method clearer (lines of code the result of should be cached are not offset next to or underneath the “@expensive ||=”), and more importantly: it does memoize nil and false results like you would expect.

@lifo
Copy link
Member

@lifo lifo commented on 55adaa2 Jul 26, 2008

Choose a reason for hiding this comment

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

ZOMFG

@lifo
Copy link
Member

@lifo lifo commented on 55adaa2 Jul 26, 2008

Choose a reason for hiding this comment

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

ZOMFG(&:alias_method_chain)

@Roman2K
Copy link

Choose a reason for hiding this comment

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

@lifo: would you care to elaborate?

@NZKoz
Copy link
Member

@NZKoz NZKoz commented on 55adaa2 Jul 26, 2008

Choose a reason for hiding this comment

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

Michael,

If you’re not going to add any value with your comments, can I suggest you just don’t bother making them. If you have something useful to add we’d love to hear from you either here or on the mailing list. If you’re just going to troll spouting underinformed opinions based on FUD just save your time and ours and don’t bother.

@ncr
Copy link
Contributor

@ncr ncr commented on 55adaa2 Jul 26, 2008

Choose a reason for hiding this comment

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

I think lifo had on mind that alias_method_chain is the same as memoize in a way: declarative, easier, dry, etc. than aliasing by hand. It’s a pattern, so fleshing it out as a method is a good idea.

Please sign in to comment.