From 93fcd5687c7e349b019c4a076fb88254ba934614 Mon Sep 17 00:00:00 2001 From: Tristan Hume Date: Thu, 22 Aug 2013 12:12:35 -0400 Subject: [PATCH 1/8] Broken warnings implementation. --- lib/liquid/block.rb | 15 +++++++++++++++ lib/liquid/tag.rb | 4 ++++ lib/liquid/template.rb | 8 ++++++++ test/liquid/error_handling_test.rb | 6 +++--- 4 files changed, 30 insertions(+), 3 deletions(-) diff --git a/lib/liquid/block.rb b/lib/liquid/block.rb index 737fadf2..e4b2398b 100644 --- a/lib/liquid/block.rb +++ b/lib/liquid/block.rb @@ -56,6 +56,21 @@ module Liquid assert_missing_delimitation! end + # warnings of this block and all sub-tags + def warnings + all_warnings = [] + all_warnings.concat(@warnings) if @warnings + + return all_warnings unless @nodelist + @nodelist.each do |node| + p node + node_warns = node.respond_to?(:warnings) ? node.warnings : nil + all_warnings.concat(node_warns) if node_warns + end + + all_warnings + end + def end_tag end diff --git a/lib/liquid/tag.rb b/lib/liquid/tag.rb index ce0df63a..da951e02 100644 --- a/lib/liquid/tag.rb +++ b/lib/liquid/tag.rb @@ -33,6 +33,10 @@ module Liquid @blank || true end + def warnings + @warnings + end + def parse_with_selected_parser(markup) case @options[:error_mode] || Template.error_mode when :strict then strict_parse_with_error_context(markup) diff --git a/lib/liquid/template.rb b/lib/liquid/template.rb index b43a2a38..4617e1e8 100644 --- a/lib/liquid/template.rb +++ b/lib/liquid/template.rb @@ -69,9 +69,17 @@ module Liquid # Returns self for easy chaining def parse(source, options = {}) @root = Document.new(tokenize(source), options) + @warnings = nil self end + # memoize because the warnings operation + # could be expensive. + def warnings + return [] unless @root + @warnings ||= @root.warnings + end + def registers @registers ||= {} end diff --git a/test/liquid/error_handling_test.rb b/test/liquid/error_handling_test.rb index 80902a31..6ac2e490 100644 --- a/test/liquid/error_handling_test.rb +++ b/test/liquid/error_handling_test.rb @@ -92,10 +92,10 @@ class ErrorHandlingTest < Test::Unit::TestCase end def test_warnings - template = Liquid::Template.parse('{% if ~~~ %}derp{% else %}wat{% endif %}', :error_mode => :warn) + template = Liquid::Template.parse('{% if ~~~ %}{{%%%}}{% else %}wat{% endif %}', :error_mode => :warn) + assert_equal 2, template.warnings.size + assert_equal 'Unexpected character ~ in "~~~"', template.warnings.first.message assert_equal 'wat', template.render - assert_equal 1, template.errors.size - assert_equal 'Unexpected character ~ in "~~~"', template.errors.first.message end # Liquid should not catch Exceptions that are not subclasses of StandardError, like Interrupt and NoMemoryError From b0cba5298a6c30cb40ccfdd61ebdd60739081642 Mon Sep 17 00:00:00 2001 From: Tristan Hume Date: Thu, 22 Aug 2013 12:44:23 -0400 Subject: [PATCH 2/8] Fix warnings and make tags a proper syntax tree. --- lib/liquid/block.rb | 13 +++++++++---- lib/liquid/variable.rb | 6 +++--- test/liquid/error_handling_test.rb | 1 + 3 files changed, 13 insertions(+), 7 deletions(-) diff --git a/lib/liquid/block.rb b/lib/liquid/block.rb index e4b2398b..f898d6cd 100644 --- a/lib/liquid/block.rb +++ b/lib/liquid/block.rb @@ -14,6 +14,9 @@ module Liquid @nodelist ||= [] @nodelist.clear + # All child tags of the current block. + @children = [] + while token = tokens.shift case token when IsTag @@ -31,6 +34,7 @@ module Liquid new_tag = tag.new_with_options($1, $2, tokens, @options || {}) @blank &&= new_tag.blank? @nodelist << new_tag + @children << new_tag else # this tag is not registered with the system # pass it to the current block for special handling or error reporting @@ -40,7 +44,9 @@ module Liquid raise SyntaxError, "Tag '#{token}' was not properly terminated with regexp: #{TagEnd.inspect} " end when IsVariable - @nodelist << create_variable(token) + new_var = create_variable(token) + @nodelist << new_var + @children << new_var @blank = false when '' # pass @@ -61,9 +67,8 @@ module Liquid all_warnings = [] all_warnings.concat(@warnings) if @warnings - return all_warnings unless @nodelist - @nodelist.each do |node| - p node + return all_warnings unless @children + @children.each do |node| node_warns = node.respond_to?(:warnings) ? node.warnings : nil all_warnings.concat(node_warns) if node_warns end diff --git a/lib/liquid/variable.rb b/lib/liquid/variable.rb index 7aa2999d..d572d662 100644 --- a/lib/liquid/variable.rb +++ b/lib/liquid/variable.rb @@ -13,12 +13,12 @@ module Liquid class Variable FilterParser = /(?:#{FilterSeparator}|(?:\s*(?:#{QuotedFragment}|#{ArgumentSeparator})\s*)+)/o EasyParse = /^ *(\w+(?:\.\w+)*) *$/ - attr_accessor :filters, :name + attr_accessor :filters, :name, :warnings def initialize(markup, options = {}) @markup = markup @name = nil - @warning = nil + @warnings = [] @options = options || {} @@ -29,7 +29,7 @@ module Liquid begin strict_parse(markup) rescue SyntaxError => e - @warning = e + @warnings << e lax_parse(markup) end end diff --git a/test/liquid/error_handling_test.rb b/test/liquid/error_handling_test.rb index 6ac2e490..aa41b588 100644 --- a/test/liquid/error_handling_test.rb +++ b/test/liquid/error_handling_test.rb @@ -95,6 +95,7 @@ class ErrorHandlingTest < Test::Unit::TestCase template = Liquid::Template.parse('{% if ~~~ %}{{%%%}}{% else %}wat{% endif %}', :error_mode => :warn) assert_equal 2, template.warnings.size assert_equal 'Unexpected character ~ in "~~~"', template.warnings.first.message + assert_equal 'Unexpected character % in "{{%%%}}"', template.warnings.last.message assert_equal 'wat', template.render end From 77db92de54c41cf0c636fabfedc6df82b722b400 Mon Sep 17 00:00:00 2001 From: Tristan Hume Date: Thu, 22 Aug 2013 12:55:54 -0400 Subject: [PATCH 3/8] Better testing of warn mode. --- test/liquid/error_handling_test.rb | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/test/liquid/error_handling_test.rb b/test/liquid/error_handling_test.rb index aa41b588..94c321e4 100644 --- a/test/liquid/error_handling_test.rb +++ b/test/liquid/error_handling_test.rb @@ -92,11 +92,12 @@ class ErrorHandlingTest < Test::Unit::TestCase end def test_warnings - template = Liquid::Template.parse('{% if ~~~ %}{{%%%}}{% else %}wat{% endif %}', :error_mode => :warn) - assert_equal 2, template.warnings.size - assert_equal 'Unexpected character ~ in "~~~"', template.warnings.first.message - assert_equal 'Unexpected character % in "{{%%%}}"', template.warnings.last.message - assert_equal 'wat', template.render + template = Liquid::Template.parse('{% if ~~~ %}{{%%%}}{% else %}{{ hello. }}{% endif %}', :error_mode => :warn) + assert_equal 3, template.warnings.size + assert_equal 'Unexpected character ~ in "~~~"', template.warnings[0].message + assert_equal 'Unexpected character % in "{{%%%}}"', template.warnings[1].message + assert_equal 'Expected id but found [:end_of_string] in "{{ hello. }}"', template.warnings[2].message + assert_equal '', template.render end # Liquid should not catch Exceptions that are not subclasses of StandardError, like Interrupt and NoMemoryError From 5bdfb62bf2049674068c9aaebdc228babe8fbf32 Mon Sep 17 00:00:00 2001 From: Tristan Hume Date: Thu, 22 Aug 2013 12:57:16 -0400 Subject: [PATCH 4/8] Remove old warning method --- lib/liquid/tags/for.rb | 1 - lib/liquid/tags/if.rb | 1 - lib/liquid/variable.rb | 1 - 3 files changed, 3 deletions(-) diff --git a/lib/liquid/tags/for.rb b/lib/liquid/tags/for.rb index 7ddcdde8..69fc9d33 100644 --- a/lib/liquid/tags/for.rb +++ b/lib/liquid/tags/for.rb @@ -58,7 +58,6 @@ module Liquid end def render(context) - context.errors.concat(@warnings) if @warnings context.registers[:for] ||= Hash.new(0) collection = context[@collection_name] diff --git a/lib/liquid/tags/if.rb b/lib/liquid/tags/if.rb index 90cc8807..ac1767d3 100644 --- a/lib/liquid/tags/if.rb +++ b/lib/liquid/tags/if.rb @@ -29,7 +29,6 @@ module Liquid end def render(context) - context.errors.concat(@warnings) if @warnings context.stack do @blocks.each do |block| if block.evaluate(context) diff --git a/lib/liquid/variable.rb b/lib/liquid/variable.rb index d572d662..c119c808 100644 --- a/lib/liquid/variable.rb +++ b/lib/liquid/variable.rb @@ -87,7 +87,6 @@ module Liquid def render(context) return '' if @name.nil? - context.errors << @warning if @warning @filters.inject(context[@name]) do |output, filter| filterargs = [] keyword_args = {} From 86ba2f4174c9a9225a2cac45d84d3ae6319e6d5f Mon Sep 17 00:00:00 2001 From: Tristan Hume Date: Thu, 22 Aug 2013 13:23:44 -0400 Subject: [PATCH 5/8] Fix error message 1.8 compatibility --- lib/liquid/parser.rb | 2 +- test/liquid/error_handling_test.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/liquid/parser.rb b/lib/liquid/parser.rb index 0c499d1c..96260018 100644 --- a/lib/liquid/parser.rb +++ b/lib/liquid/parser.rb @@ -13,7 +13,7 @@ module Liquid def consume(type = nil) token = @tokens[@p] if type && token[0] != type - raise SyntaxError, "Expected #{type} but found #{@tokens[@p]}" + raise SyntaxError, "Expected #{type} but found #{@tokens[@p].first}" end @p += 1 token[1] diff --git a/test/liquid/error_handling_test.rb b/test/liquid/error_handling_test.rb index 94c321e4..192285db 100644 --- a/test/liquid/error_handling_test.rb +++ b/test/liquid/error_handling_test.rb @@ -96,7 +96,7 @@ class ErrorHandlingTest < Test::Unit::TestCase assert_equal 3, template.warnings.size assert_equal 'Unexpected character ~ in "~~~"', template.warnings[0].message assert_equal 'Unexpected character % in "{{%%%}}"', template.warnings[1].message - assert_equal 'Expected id but found [:end_of_string] in "{{ hello. }}"', template.warnings[2].message + assert_equal 'Expected id but found end_of_string in "{{ hello. }}"', template.warnings[2].message assert_equal '', template.render end From dd3196b22e9f7b0f53c0796f4165f9d98bf9279d Mon Sep 17 00:00:00 2001 From: Tristan Hume Date: Thu, 22 Aug 2013 16:15:12 -0400 Subject: [PATCH 6/8] Consistency in warnings. --- lib/liquid/block.rb | 4 +--- lib/liquid/variable.rb | 2 +- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/lib/liquid/block.rb b/lib/liquid/block.rb index f898d6cd..a5c3b1e4 100644 --- a/lib/liquid/block.rb +++ b/lib/liquid/block.rb @@ -67,10 +67,8 @@ module Liquid all_warnings = [] all_warnings.concat(@warnings) if @warnings - return all_warnings unless @children @children.each do |node| - node_warns = node.respond_to?(:warnings) ? node.warnings : nil - all_warnings.concat(node_warns) if node_warns + all_warnings.concat(node.warnings || []) end all_warnings diff --git a/lib/liquid/variable.rb b/lib/liquid/variable.rb index c119c808..7ad60feb 100644 --- a/lib/liquid/variable.rb +++ b/lib/liquid/variable.rb @@ -18,7 +18,6 @@ module Liquid def initialize(markup, options = {}) @markup = markup @name = nil - @warnings = [] @options = options || {} @@ -29,6 +28,7 @@ module Liquid begin strict_parse(markup) rescue SyntaxError => e + @warnings ||= [] @warnings << e lax_parse(markup) end From c94b5e87c9491fe6b6324446fe9349f585cfc47a Mon Sep 17 00:00:00 2001 From: Tristan Hume Date: Thu, 22 Aug 2013 16:16:28 -0400 Subject: [PATCH 7/8] Use attr_reader for warnings. --- lib/liquid/tag.rb | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/lib/liquid/tag.rb b/lib/liquid/tag.rb index da951e02..c1195622 100644 --- a/lib/liquid/tag.rb +++ b/lib/liquid/tag.rb @@ -1,6 +1,7 @@ module Liquid class Tag attr_accessor :nodelist, :options + attr_reader :warnings def self.new_with_options(tag_name, markup, tokens, options) # Forgive me Matz for I have sinned. I know this code is weird @@ -33,10 +34,6 @@ module Liquid @blank || true end - def warnings - @warnings - end - def parse_with_selected_parser(markup) case @options[:error_mode] || Template.error_mode when :strict then strict_parse_with_error_context(markup) From e305edc3b8a92df47bae6d87c33812681c9716b0 Mon Sep 17 00:00:00 2001 From: Tristan Hume Date: Tue, 27 Aug 2013 09:53:06 -0400 Subject: [PATCH 8/8] Remove extra comment --- lib/liquid/template.rb | 2 -- 1 file changed, 2 deletions(-) diff --git a/lib/liquid/template.rb b/lib/liquid/template.rb index 4617e1e8..87f1491c 100644 --- a/lib/liquid/template.rb +++ b/lib/liquid/template.rb @@ -73,8 +73,6 @@ module Liquid self end - # memoize because the warnings operation - # could be expensive. def warnings return [] unless @root @warnings ||= @root.warnings