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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

implementation/tests of the template inheritance mechanism #336

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
78 changes: 78 additions & 0 deletions lib/liquid/tags/extends.rb
@@ -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}+)/
Copy link
Member

Choose a reason for hiding this comment

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

this needs memoization

Copy link
Author

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?

Copy link
Member

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

  • tobi
    CEO Shopify

On Wed, Apr 9, 2014 at 10:00 AM, Didier Lafforgue
notifications@github.comwrote:

In lib/liquid/tags/extends.rb:

@@ -0,0 +1,76 @@
+module Liquid
+

  • Extends allows designer to use template inheritance

  • {% extends home %}

  • {% block content }Hello world{% endblock %}

  • class Extends < Block
  • Syntax = /(#{QuotedFragment}+)/

hi @tobi https://github.com/tobi! do you mean for the overall
implementation or in some specific methods?


Reply to this email directly or view it on GitHubhttps://github.com//pull/336/files#r11436430
.


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?
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 think the nodelist.any? is necessary

Copy link
Author

Choose a reason for hiding this comment

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

right. tests pass without the nodelist.any

0.upto(nodelist.size - 1).each do |index|
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not use nodelist.each_with_index do |node, index| ... end instead?

Copy link
Author

Choose a reason for hiding this comment

The 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)
Copy link
Contributor

Choose a reason for hiding this comment

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

isn't this just nodelist[index] = new_node?

Copy link
Author

Choose a reason for hiding this comment

The 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!
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't seem to be used anywhere

Copy link
Author

Choose a reason for hiding this comment

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

parse_parent_template is used to get the nodes from the "layout" template
assert_missing_delimitation! is super important, otherwise parsing extends tag would fail because the parser would not find the end_extends tag (keep in mind that we want to mimic the Django's inheritance behaviour).

end
end

Template.register_tag('extends', Extends)
end
102 changes: 102 additions & 0 deletions lib/liquid/tags/inherited_block.rb
@@ -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
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm I feel like this shouldn't be publicly writeable

Copy link
Author

Choose a reason for hiding this comment

The 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
Copy link
Contributor

Choose a reason for hiding this comment

The 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?

Copy link
Contributor

Choose a reason for hiding this comment

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

or should this be self.options?

Copy link
Author

Choose a reason for hiding this comment

The 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.
But, for the next version of Liquid (the coming 3.0.0), there is an option parameter when constructing a blog/tag and I'm using it as an alias for my previous context parameter.
The name and the use of options can be misleading here.

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, {})
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a bit ugly :-(

Copy link
Author

Choose a reason for hiding this comment

The 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
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you put this into it's own file?

Copy link
Author

Choose a reason for hiding this comment

The 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
Copy link
Contributor

Choose a reason for hiding this comment

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

you have some more missing line-endings

Copy link
Author

Choose a reason for hiding this comment

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

fixed

82 changes: 82 additions & 0 deletions test/integration/tags/extends_tag_test.rb
@@ -0,0 +1,82 @@
require 'test_helper'

class LayoutFileSystem
def read_template_file(template_path, context)
case template_path
when "base"
Copy link
Contributor

Choose a reason for hiding this comment

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

this is quite readable, but you could also use when xxx then xxx to make a oneliner

Copy link
Author

Choose a reason for hiding this comment

The 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