-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
implementation/tests of the template inheritance mechanism #336
base: main
Are you sure you want to change the base?
Changes from 1 commit
3574889
3f0f76a
a2c3766
7d14d7e
a6e6e70
7e4eacd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,78 @@ | ||
module Liquid | ||
|
||
# Extends allows designer to use template inheritance | ||
# | ||
# {% extends home %} | ||
# {% block content }Hello world{% endblock %} | ||
# | ||
class Extends < Block | ||
Syntax = /(#{QuotedFragment}+)/ | ||
|
||
def initialize(tag_name, markup, options) | ||
super | ||
|
||
if markup =~ Syntax | ||
@template_name = $1.gsub(/["']/o, '').strip | ||
else | ||
raise(SyntaxError.new(options[:locale].t("errors.syntax.extends".freeze))) | ||
end | ||
end | ||
|
||
def parse(tokens) | ||
# get the nodes of the template the object inherits from | ||
parent_template = parse_parent_template | ||
|
||
# find the blocks in case there is a call to | ||
# the super method inside the current template | ||
@options.merge!(:blocks => self.find_blocks(parent_template.root.nodelist)) | ||
|
||
# finally, process the rest of the tokens | ||
# the tags/blocks other than the InheritedBlock type will be ignored. | ||
super | ||
|
||
# replace the nodes of the current template by the ones from the parent | ||
@nodelist = parent_template.root.nodelist.clone | ||
end | ||
|
||
def blank? | ||
false | ||
end | ||
|
||
protected | ||
|
||
def find_blocks(nodelist, blocks = {}) | ||
if nodelist && nodelist.any? | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. right. tests pass without the |
||
0.upto(nodelist.size - 1).each do |index| | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why not use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. totally right. No idea why there was this :-) |
||
node = nodelist[index] | ||
|
||
# is the node an inherited block? | ||
if node.respond_to?(:call_super) | ||
new_node = node.class.clone_block(node) | ||
|
||
nodelist.insert(index, new_node) | ||
nodelist.delete_at(index + 1) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. isn't this just There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @phoet, I've modified it and the tests suite pass. To be honest, I remember doing that for a good purpose but not a clue about the real reason. |
||
|
||
blocks[node.name] = new_node | ||
end | ||
if node.respond_to?(:nodelist) | ||
# find nested blocks too | ||
self.find_blocks(node.nodelist, blocks) | ||
end | ||
end | ||
end | ||
blocks | ||
end | ||
|
||
private | ||
|
||
def parse_parent_template | ||
source = Template.file_system.read_template_file(@template_name, {}) | ||
Template.parse(source) | ||
end | ||
|
||
def assert_missing_delimitation! | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This doesn't seem to be used anywhere There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
end | ||
end | ||
|
||
Template.register_tag('extends', Extends) | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,102 @@ | ||
module Liquid | ||
|
||
# Blocks are used with the Extends tag to define | ||
# the content of blocks. Nested blocks are allowed. | ||
# | ||
# {% extends home %} | ||
# {% block content }Hello world{% endblock %} | ||
# | ||
class InheritedBlock < Block | ||
Syntax = /(#{QuotedFragment}+)/ | ||
|
||
attr_accessor :parent | ||
attr_accessor :nodelist | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm I feel like this shouldn't be publicly writeable There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm fixing it |
||
attr_reader :name | ||
|
||
def initialize(tag_name, markup, options) | ||
super | ||
|
||
if markup =~ Syntax | ||
@name = $1.gsub(/["']/o, '').strip | ||
else | ||
raise(SyntaxError.new(options[:locale].t("errors.syntax.block")), options[:line]) | ||
end | ||
|
||
self.set_full_name!(options) | ||
|
||
(options[:block_stack] ||= []).push(self) | ||
options[:current_block] = self | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is there something that we are using the options for at this point? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. or should this be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. both self.options, options or even @options work here. At first, when I developed this feature, I needed to pass a "context" when parsing a liquid document. Unfortunately, that was not possible with the current Liquid implementation. |
||
end | ||
|
||
def render(context) | ||
context.stack do | ||
context['block'] = InheritedBlockDrop.new(self) | ||
render_all(@nodelist, context) | ||
end | ||
end | ||
|
||
def end_tag | ||
self.register_current_block | ||
|
||
options[:block_stack].pop | ||
options[:current_block] = options[:block_stack].last | ||
end | ||
|
||
def call_super(context) | ||
if parent | ||
parent.render(context) | ||
else | ||
'' | ||
end | ||
end | ||
|
||
def self.clone_block(block) | ||
new_block = new(block.send(:instance_variable_get, :"@tag_name"), block.name, {}) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a bit ugly :-( There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. let me refactor that a little |
||
new_block.parent = block.parent | ||
new_block.nodelist = block.nodelist | ||
new_block | ||
end | ||
|
||
protected | ||
|
||
def set_full_name!(options) | ||
if options[:current_block] | ||
@name = options[:current_block].name + '/' + @name | ||
end | ||
end | ||
|
||
def register_current_block | ||
options[:blocks] ||= {} | ||
|
||
block = options[:blocks][@name] | ||
|
||
if block | ||
# copy the existing block in order to make it a parent of the parsed block | ||
new_block = self.class.clone_block(block) | ||
|
||
# replace the up-to-date version of the block in the parent template | ||
block.parent = new_block | ||
block.nodelist = @nodelist | ||
end | ||
end | ||
|
||
end | ||
|
||
class InheritedBlockDrop < Drop | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you put this into it's own file? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree but where? Any thoughts? |
||
|
||
def initialize(block) | ||
@block = block | ||
end | ||
|
||
def name | ||
@block.name | ||
end | ||
|
||
def super | ||
@block.call_super(@context) | ||
end | ||
|
||
end | ||
|
||
Template.register_tag('block', InheritedBlock) | ||
end | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. you have some more missing line-endings There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. fixed |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,82 @@ | ||
require 'test_helper' | ||
|
||
class LayoutFileSystem | ||
def read_template_file(template_path, context) | ||
case template_path | ||
when "base" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is quite readable, but you could also use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I know but, on my screen, for some of them, I'm not able to see the beginning of the strings if I on-line these statements. |
||
"<body>base</body>" | ||
|
||
when "inherited" | ||
"{% extends base %}" | ||
|
||
when "page_with_title" | ||
"<body><h1>{% block title %}Hello{% endblock %}</h1><p>Lorem ipsum</p></body>" | ||
|
||
when "product" | ||
"<body><h1>Our product: {{ name }}</h1>{% block info %}{% endblock %}</body>" | ||
|
||
when "product_with_warranty" | ||
"{% extends product %}{% block info %}<p>mandatory warranty</p>{% endblock %}" | ||
|
||
when "product_with_static_price" | ||
"{% extends product %}{% block info %}<h2>Some info</h2>{% block price %}<p>$42.00</p>{% endblock %}{% endblock %}" | ||
|
||
else | ||
template_path | ||
end | ||
end | ||
end | ||
|
||
class ExtendsTagTest < Test::Unit::TestCase | ||
include Liquid | ||
|
||
def setup | ||
Liquid::Template.file_system = LayoutFileSystem.new | ||
end | ||
|
||
def test_template_extends_another_template | ||
assert_template_result "<body>base</body>", | ||
"{% extends base %}" | ||
end | ||
|
||
def test_template_extends_an_inherited_template | ||
assert_template_result "<body>base</body>", | ||
"{% extends inherited %}" | ||
end | ||
|
||
def test_template_can_pass_variables_to_the_parent_template | ||
assert_template_result "<body><h1>Our product: Macbook</h1></body>", | ||
"{% extends product %}", 'name' => 'Macbook' | ||
end | ||
|
||
def test_template_can_pass_variables_to_the_inherited_parent_template | ||
assert_template_result "<body><h1>Our product: PC</h1><p>mandatory warranty</p></body>", | ||
"{% extends product_with_warranty %}", 'name' => 'PC' | ||
end | ||
|
||
def test_template_does_not_render_statements_outside_blocks | ||
assert_template_result "<body>base</body>", | ||
"{% extends base %} Hello world" | ||
end | ||
|
||
def test_template_extends_another_template_with_a_single_block | ||
assert_template_result "<body><h1>Hello</h1><p>Lorem ipsum</p></body>", | ||
"{% extends page_with_title %}" | ||
end | ||
|
||
def test_template_overrides_a_block | ||
assert_template_result "<body><h1>Sweet</h1><p>Lorem ipsum</p></body>", | ||
"{% extends page_with_title %}{% block title %}Sweet{% endblock %}" | ||
end | ||
|
||
def test_template_has_access_to_the_content_of_the_overriden_block | ||
assert_template_result "<body><h1>Hello world</h1><p>Lorem ipsum</p></body>", | ||
"{% extends page_with_title %}{% block title %}{{ block.super }} world{% endblock %}" | ||
end | ||
|
||
def test_template_accepts_nested_blocks | ||
assert_template_result "<body><h1>Our product: iPhone</h1><h2>Some info</h2><p>$42.00</p><p>(not on sale)</p></body>", | ||
"{% extends product_with_static_price %}{% block info/price %}{{ block.super }}<p>(not on sale)</p>{% endblock %}", 'name' => 'iPhone' | ||
end | ||
|
||
end # ExtendsTagTest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this needs memoization
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hi @tobi! do you mean for the overall implementation or in some specific methods?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just the regexp. It's done by adding /o to it.
http://stackoverflow.com/questions/13334807/what-does-the-o-modifier-for-a-regexp-mean
CEO Shopify
On Wed, Apr 9, 2014 at 10:00 AM, Didier Lafforgue
notifications@github.comwrote: