-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Add HybridTag base class and BlockBody reparenting #2059
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
base: main
Are you sure you want to change the base?
Changes from all commits
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 |
|---|---|---|
|
|
@@ -37,6 +37,10 @@ def unknown_tag(tag, _markup, _tokenizer) | |
| end | ||
| end | ||
|
|
||
| def blank? | ||
|
Contributor
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 is this needed? |
||
| @body.blank? | ||
| end | ||
|
|
||
| def render_to_output_buffer(context, output) | ||
| @body.render_to_output_buffer(context, output) | ||
| end | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,44 @@ | ||
| # frozen_string_literal: true | ||
|
|
||
| module Liquid | ||
| class HybridTag < Block | ||
| def reparent_as_block(children, parse_context) | ||
|
Contributor
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.
|
||
| @body = new_body | ||
| @body.nodelist.concat(children) | ||
| @body.freeze | ||
| end | ||
|
|
||
| def parse(_tokens) | ||
| end | ||
|
|
||
| def block_form? | ||
| !!@body | ||
| end | ||
|
|
||
| def nodelist | ||
| @body ? @body.nodelist : Const::EMPTY_ARRAY | ||
| end | ||
|
|
||
| def blank? | ||
| raise NotImplementedError, "#{self.class} must implement blank?" | ||
|
Contributor
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. In the description you mention
Should this be done here instead of letting subclasses manage blank? |
||
| end | ||
|
|
||
| def render_to_output_buffer(context, output) | ||
| if block_form? | ||
| render_block_form_to_output_buffer(context, output) | ||
| else | ||
| render_self_closing_to_output_buffer(context, output) | ||
| end | ||
| end | ||
|
|
||
| private | ||
|
|
||
| def render_block_form_to_output_buffer(_context, _output) | ||
| raise NotImplementedError, "#{self.class} must implement render_block_form_to_output_buffer" | ||
| end | ||
|
|
||
| def render_self_closing_to_output_buffer(_context, _output) | ||
| raise NotImplementedError, "#{self.class} must implement render_self_closing_to_output_buffer" | ||
| end | ||
| end | ||
| end | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,155 @@ | ||
| # frozen_string_literal: true | ||
|
|
||
| require 'test_helper' | ||
|
|
||
| class HybridTagUnitTest < Minitest::Test | ||
| include Liquid | ||
|
|
||
| class TestHybridTag < Liquid::HybridTag | ||
| def blank? | ||
| true | ||
| end | ||
|
|
||
| private | ||
|
|
||
| def render_self_closing_to_output_buffer(_context, output) | ||
| output << "self-closing" | ||
| end | ||
|
|
||
| def render_block_form_to_output_buffer(context, output) | ||
| output << "block[" | ||
| @body.render_to_output_buffer(context, output) | ||
| output << "]" | ||
| end | ||
| end | ||
|
|
||
| def setup | ||
| @environment = Liquid::Environment.build do |env| | ||
| env.register_tag("hybrid", TestHybridTag) | ||
| end | ||
| end | ||
|
|
||
| def test_hybrid_tag_is_subclass_of_block | ||
| assert(TestHybridTag < Liquid::Block) | ||
| end | ||
|
|
||
| def test_self_closing_renders_correctly | ||
| template = Liquid::Template.parse("{% hybrid %}", environment: @environment) | ||
| assert_equal("self-closing", template.render) | ||
| end | ||
|
|
||
| def test_self_closing_block_form_predicate_is_false | ||
| tag = parse_hybrid_tag("{% hybrid %}") | ||
| refute(tag.block_form?) | ||
| end | ||
|
|
||
| def test_self_closing_does_not_consume_subsequent_tokens | ||
| template = Liquid::Template.parse("{% hybrid %}after", environment: @environment) | ||
| assert_equal("self-closingafter", template.render) | ||
| end | ||
|
|
||
| def test_block_form_renders_correctly | ||
| template = Liquid::Template.parse("{% hybrid %}body{% endhybrid %}", environment: @environment) | ||
| assert_equal("block[body]", template.render) | ||
| end | ||
|
|
||
| def test_block_form_predicate_is_true | ||
| tag = parse_hybrid_tag("{% hybrid %}body{% endhybrid %}") | ||
| assert(tag.block_form?) | ||
| end | ||
|
|
||
| def test_block_form_body_accessible_via_nodelist | ||
| tag = parse_hybrid_tag("{% hybrid %}hello world{% endhybrid %}") | ||
| assert(tag.block_form?) | ||
| refute_empty(tag.nodelist) | ||
| assert_equal("hello world", tag.nodelist.map(&:to_s).join) | ||
| end | ||
|
|
||
| def test_empty_block_form | ||
| template = Liquid::Template.parse("{% hybrid %}{% endhybrid %}", environment: @environment) | ||
| assert_equal("block[]", template.render) | ||
| end | ||
|
|
||
| def test_block_form_with_liquid_content | ||
| template = Liquid::Template.parse( | ||
| "{% hybrid %}before{{ var }}after{% endhybrid %}", | ||
| environment: @environment, | ||
| ) | ||
| assert_equal("block[beforeVafter]", template.render({ "var" => "V" })) | ||
| end | ||
|
|
||
| def test_sequential_block_forms | ||
| template = Liquid::Template.parse( | ||
| "{% hybrid %}a{% endhybrid %}{% hybrid %}b{% endhybrid %}", | ||
| environment: @environment, | ||
| ) | ||
| assert_equal("block[a]block[b]", template.render) | ||
| end | ||
|
|
||
| def test_self_closing_followed_by_block_form | ||
| template = Liquid::Template.parse( | ||
| "{% hybrid %}{% hybrid %}body{% endhybrid %}", | ||
| environment: @environment, | ||
| ) | ||
| assert_equal("self-closingblock[body]", template.render) | ||
| end | ||
|
|
||
| def test_block_form_followed_by_self_closing | ||
| template = Liquid::Template.parse( | ||
| "{% hybrid %}body{% endhybrid %}{% hybrid %}", | ||
| environment: @environment, | ||
| ) | ||
| assert_equal("block[body]self-closing", template.render) | ||
| end | ||
|
|
||
| def test_mixed_forms | ||
| template = Liquid::Template.parse( | ||
| "{% hybrid %}{% hybrid %}body{% endhybrid %}{% hybrid %}", | ||
| environment: @environment, | ||
| ) | ||
| assert_equal("self-closingblock[body]self-closing", template.render) | ||
| end | ||
|
|
||
| def test_self_closing_inside_block_tag | ||
| template = Liquid::Template.parse( | ||
| "{% if true %}{% hybrid %}{% endif %}", | ||
| environment: @environment, | ||
| ) | ||
| assert_equal("self-closing", template.render) | ||
| end | ||
|
|
||
| def test_block_form_inside_block_tag | ||
| template = Liquid::Template.parse( | ||
| "{% if true %}{% hybrid %}body{% endhybrid %}{% endif %}", | ||
| environment: @environment, | ||
| ) | ||
| assert_equal("block[body]", template.render) | ||
| end | ||
|
|
||
| def test_nested_same_type_raises_syntax_error | ||
| error = assert_raises(Liquid::SyntaxError) do | ||
| Liquid::Template.parse( | ||
| "{% hybrid %}{% hybrid %}inner{% endhybrid %}{% endhybrid %}", | ||
| environment: @environment, | ||
| ) | ||
| end | ||
| assert_match(/cannot be nested/, error.message) | ||
| end | ||
|
|
||
| def test_orphan_end_tag_raises_syntax_error | ||
| error = assert_raises(Liquid::SyntaxError) do | ||
| Liquid::Template.parse( | ||
| "{% endhybrid %}", | ||
| environment: @environment, | ||
| ) | ||
| end | ||
| assert_match(/no matching/, error.message) | ||
| end | ||
|
|
||
| private | ||
|
|
||
| def parse_hybrid_tag(source) | ||
| template = Liquid::Template.parse(source, environment: @environment) | ||
| template.root.nodelist.find { |node| node.is_a?(TestHybridTag) } | ||
| end | ||
| end |
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.
Do we need to update @blank here after reparenting so that whitespace control works correctly for the parent body?
{%- if something -%} {% hybrid %}content{% endhybrid %} {% endif %}Without updating it, whitespace control might not work properly.