From cebf75b8d700593af2d40021c825e1176b51ab33 Mon Sep 17 00:00:00 2001 From: Dylan Thacker-Smith Date: Sat, 4 Jul 2015 20:10:15 -0400 Subject: [PATCH 1/4] Implement line numbers without the Liquid::Token class. --- lib/liquid.rb | 2 +- lib/liquid/block.rb | 12 ------ lib/liquid/block_body.rb | 36 ++++++---------- lib/liquid/context.rb | 14 ++++--- lib/liquid/document.rb | 6 +-- lib/liquid/errors.rb | 6 --- lib/liquid/parse_context.rb | 39 ++++++++++++++++++ lib/liquid/parser_switching.rb | 7 ++-- lib/liquid/tag.rb | 13 +++--- lib/liquid/tags/assign.rb | 1 - lib/liquid/tags/include.rb | 23 ++++++----- lib/liquid/template.rb | 18 ++++---- lib/liquid/token.rb | 18 -------- lib/liquid/tokenizer.rb | 18 +++----- lib/liquid/variable.rb | 8 ++-- test/integration/filter_test.rb | 57 +++++++++++--------------- test/unit/tag_unit_test.rb | 6 +-- test/unit/tokenizer_unit_test.rb | 26 +++++++++--- test/unit/variable_unit_test.rb | 70 +++++++++++++++++--------------- 19 files changed, 186 insertions(+), 194 deletions(-) create mode 100644 lib/liquid/parse_context.rb delete mode 100644 lib/liquid/token.rb diff --git a/lib/liquid.rb b/lib/liquid.rb index 14afa757..f4c6fea2 100644 --- a/lib/liquid.rb +++ b/lib/liquid.rb @@ -69,7 +69,7 @@ require 'liquid/standardfilters' require 'liquid/condition' require 'liquid/utils' require 'liquid/tokenizer' -require 'liquid/token' +require 'liquid/parse_context' # Load all the tags of the standard library # diff --git a/lib/liquid/block.rb b/lib/liquid/block.rb index fdc7567d..71506423 100644 --- a/lib/liquid/block.rb +++ b/lib/liquid/block.rb @@ -23,18 +23,6 @@ module Liquid @body.nodelist end - # warnings of this block and all sub-tags - def warnings - all_warnings = [] - all_warnings.concat(@warnings) if @warnings - - (nodelist || []).each do |node| - all_warnings.concat(node.warnings || []) if node.respond_to?(:warnings) - end - - all_warnings - end - def unknown_tag(tag, _params, _tokens) case tag when 'else'.freeze diff --git a/lib/liquid/block_body.rb b/lib/liquid/block_body.rb index 247b91e0..b93d4a6d 100644 --- a/lib/liquid/block_body.rb +++ b/lib/liquid/block_body.rb @@ -12,8 +12,9 @@ module Liquid @blank = true end - def parse(tokens, options) - while token = tokens.shift + def parse(tokenizer, parse_context) + parse_context.line_number = tokenizer.line_number + while token = tokenizer.shift begin unless token.empty? case @@ -23,9 +24,7 @@ module Liquid markup = $2 # fetch the tag from registered blocks if tag = registered_tags[tag_name] - markup = token.child(markup) if token.is_a?(Token) - new_tag = tag.parse(tag_name, markup, tokens, options) - new_tag.line_number = token.line_number if token.is_a?(Token) + new_tag = tag.parse(tag_name, markup, tokenizer, parse_context) @blank &&= new_tag.blank? @nodelist << new_tag else @@ -34,12 +33,10 @@ module Liquid return yield tag_name, markup end else - raise_missing_tag_terminator(token, options) + raise_missing_tag_terminator(token, parse_context) end when token.start_with?(VARSTART) - new_var = create_variable(token, options) - new_var.line_number = token.line_number if token.is_a?(Token) - @nodelist << new_var + @nodelist << create_variable(token, parse_context) @blank = false else @nodelist << token @@ -47,9 +44,10 @@ module Liquid end end rescue SyntaxError => e - e.set_line_number_from_token(token) + e.line_number ||= parse_context.line_number raise end + parse_context.line_number = tokenizer.line_number end yield nil, nil @@ -59,14 +57,6 @@ module Liquid @blank end - def warnings - all_warnings = [] - nodelist.each do |node| - all_warnings.concat(node.warnings || []) if node.respond_to?(:warnings) - end - all_warnings - end - def render(context) output = [] context.resource_limits.render_score += @nodelist.length @@ -92,7 +82,7 @@ module Liquid rescue MemoryError => e raise e rescue ::StandardError => e - output << context.handle_error(e, token) + output << context.handle_error(e, token.line_number) end end @@ -112,12 +102,12 @@ module Liquid node_output end - def create_variable(token, options) + def create_variable(token, parse_context) token.scan(ContentOfVariable) do |content| - markup = token.is_a?(Token) ? token.child(content.first) : content.first - return Variable.new(markup, options) + markup = content.first + return Variable.new(markup, parse_context) end - raise_missing_variable_terminator(token, options) + raise_missing_variable_terminator(token, parse_context) end def raise_missing_tag_terminator(token, options) diff --git a/lib/liquid/context.rb b/lib/liquid/context.rb index 042a5432..b15f16b2 100644 --- a/lib/liquid/context.rb +++ b/lib/liquid/context.rb @@ -13,13 +13,14 @@ module Liquid # context['bob'] #=> nil class Context class Context attr_reader :scopes, :errors, :registers, :environments, :resource_limits - attr_accessor :exception_handler, :template_name + attr_accessor :exception_handler, :template_name, :partial def initialize(environments = {}, outer_scope = {}, registers = {}, rethrow_errors = false, resource_limits = nil) @environments = [environments].flatten @scopes = [(outer_scope || {})] @registers = registers @errors = [] + @partial = false @resource_limits = resource_limits || ResourceLimits.new(Template.default_resource_limits) squash_instance_assigns_with_environments @@ -66,10 +67,10 @@ module Liquid @interrupts.pop end - def handle_error(e, token = nil) + def handle_error(e, line_number = nil) if e.is_a?(Liquid::Error) - e.template_name = template_name - e.set_line_number_from_token(token) + e.template_name ||= template_name + e.line_number ||= line_number end output = nil @@ -79,7 +80,10 @@ module Liquid case result when Exception e = result - e.set_line_number_from_token(token) if e.is_a?(Liquid::Error) + if e.is_a?(Liquid::Error) + e.template_name ||= template_name + e.line_number ||= line_number + end when String output = result else diff --git a/lib/liquid/document.rb b/lib/liquid/document.rb index c964ae84..7ecced6e 100644 --- a/lib/liquid/document.rb +++ b/lib/liquid/document.rb @@ -1,12 +1,8 @@ module Liquid class Document < BlockBody - DEFAULT_OPTIONS = { - locale: I18n.new - } - def self.parse(tokens, options) doc = new - doc.parse(tokens, DEFAULT_OPTIONS.merge(options)) + doc.parse(tokens, options) doc end diff --git a/lib/liquid/errors.rb b/lib/liquid/errors.rb index f1d4a2d4..9ee3a662 100644 --- a/lib/liquid/errors.rb +++ b/lib/liquid/errors.rb @@ -17,12 +17,6 @@ module Liquid str end - def set_line_number_from_token(token) - return unless token.respond_to?(:line_number) - return if line_number - self.line_number = token.line_number - end - def self.render(e) if e.is_a?(Liquid::Error) e.to_s diff --git a/lib/liquid/parse_context.rb b/lib/liquid/parse_context.rb new file mode 100644 index 00000000..99274816 --- /dev/null +++ b/lib/liquid/parse_context.rb @@ -0,0 +1,39 @@ +module Liquid + class ParseContext + attr_accessor :partial, :locale, :line_number + attr_reader :warnings, :error_mode + + def initialize(options = {}) + @template_options = options ? options.dup : {} + @locale = @template_options[:locale] ||= I18n.new + @warnings = [] + self.partial = false + end + + def [](option_key) + @options[option_key] + end + + def partial=(value) + @partial = value + @options = value ? partial_options : @template_options + @error_mode = @options[:error_mode] || Template.error_mode + value + end + + def partial_options + @partial_options ||= begin + dont_pass = @template_options[:include_options_blacklist] + if dont_pass == true + { locale: locale } + elsif dont_pass.is_a?(Array) + opts = @template_options.dup + dont_pass.each { |o| opts.delete(o) } + opts + else + @template_options + end + end + end + end +end diff --git a/lib/liquid/parser_switching.rb b/lib/liquid/parser_switching.rb index fe033bf7..631a9a55 100644 --- a/lib/liquid/parser_switching.rb +++ b/lib/liquid/parser_switching.rb @@ -1,16 +1,15 @@ module Liquid module ParserSwitching def parse_with_selected_parser(markup) - case @options[:error_mode] || Template.error_mode + case @options.error_mode when :strict then strict_parse_with_error_context(markup) when :lax then lax_parse(markup) when :warn begin return strict_parse_with_error_context(markup) rescue SyntaxError => e - e.set_line_number_from_token(markup) - @warnings ||= [] - @warnings << e + e.line_number = line_number + @options.warnings << e return lax_parse(markup) end end diff --git a/lib/liquid/tag.rb b/lib/liquid/tag.rb index eb9e6a05..36993c07 100644 --- a/lib/liquid/tag.rb +++ b/lib/liquid/tag.rb @@ -1,23 +1,24 @@ module Liquid class Tag - attr_accessor :options, :line_number - attr_reader :nodelist, :warnings, :tag_name + attr_accessor :options + attr_reader :nodelist, :tag_name, :line_number include ParserSwitching class << self - def parse(tag_name, markup, tokens, options) + def parse(tag_name, markup, tokenizer, options) tag = new(tag_name, markup, options) - tag.parse(tokens) + tag.parse(tokenizer) tag end private :new end - def initialize(tag_name, markup, options) + def initialize(tag_name, markup, parse_context) @tag_name = tag_name @markup = markup - @options = options + @options = parse_context + @line_number = parse_context.line_number end def parse(_tokens) diff --git a/lib/liquid/tags/assign.rb b/lib/liquid/tags/assign.rb index b0e14587..9cfb478a 100644 --- a/lib/liquid/tags/assign.rb +++ b/lib/liquid/tags/assign.rb @@ -15,7 +15,6 @@ module Liquid if markup =~ Syntax @to = $1 @from = Variable.new($2, options) - @from.line_number = line_number else raise SyntaxError.new options[:locale].t("errors.syntax.assign".freeze) end diff --git a/lib/liquid/tags/include.rb b/lib/liquid/tags/include.rb index 3b1fc23d..ba970cc7 100644 --- a/lib/liquid/tags/include.rb +++ b/lib/liquid/tags/include.rb @@ -53,8 +53,10 @@ module Liquid end old_template_name = context.template_name + old_partial = context.partial begin context.template_name = template_name + context.partial = true context.stack do @attributes.each do |key, value| context[key] = context.evaluate(value) @@ -72,11 +74,15 @@ module Liquid end ensure context.template_name = old_template_name + context.partial = old_partial end end private + alias_method :parse_context, :options + private :parse_context + def load_cached_partial(template_name, context) cached_partials = context.registers[:cached_partials] || {} @@ -84,7 +90,12 @@ module Liquid return cached end source = read_template_from_file_system(context) - partial = Liquid::Template.parse(source, pass_options) + begin + parse_context.partial = true + partial = Liquid::Template.parse(source, parse_context) + ensure + parse_context.partial = false + end cached_partials[template_name] = partial context.registers[:cached_partials] = cached_partials partial @@ -95,16 +106,6 @@ module Liquid file_system.read_template_file(context.evaluate(@template_name_expr)) end - - def pass_options - dont_pass = @options[:include_options_blacklist] - return { locale: @options[:locale] } if dont_pass == true - opts = @options.merge(included: true, include_options_blacklist: false) - if dont_pass.is_a?(Array) - dont_pass.each { |o| opts.delete(o) } - end - opts - end end Template.register_tag('include'.freeze, Include) diff --git a/lib/liquid/template.rb b/lib/liquid/template.rb index 9395795f..07d56b29 100644 --- a/lib/liquid/template.rb +++ b/lib/liquid/template.rb @@ -14,7 +14,7 @@ module Liquid # class Template attr_accessor :root - attr_reader :resource_limits + attr_reader :resource_limits, :warnings @@file_system = BlankFileSystem.new @@ -116,16 +116,12 @@ module Liquid @options = options @profiling = options[:profile] @line_numbers = options[:line_numbers] || @profiling - @root = Document.parse(tokenize(source), options) - @warnings = nil + parse_context = options.is_a?(ParseContext) ? options : ParseContext.new(options) + @root = Document.parse(tokenize(source), parse_context) + @warnings = parse_context.warnings self end - def warnings - return [] unless @root - @warnings ||= @root.warnings - end - def registers @registers ||= {} end @@ -206,7 +202,7 @@ module Liquid begin # render the nodelist. # for performance reasons we get an array back here. join will make a string out of it. - result = with_profiling do + result = with_profiling(context) do @root.render(context) end result.respond_to?(:join) ? result.join : result @@ -228,8 +224,8 @@ module Liquid Tokenizer.new(source, @line_numbers) end - def with_profiling - if @profiling && !@options[:included] + def with_profiling(context) + if @profiling && !context.partial raise "Profiler not loaded, require 'liquid/profiler' first" unless defined?(Liquid::Profiler) @profiler = Profiler.new diff --git a/lib/liquid/token.rb b/lib/liquid/token.rb deleted file mode 100644 index acf8ef9e..00000000 --- a/lib/liquid/token.rb +++ /dev/null @@ -1,18 +0,0 @@ -module Liquid - class Token < String - attr_reader :line_number - - def initialize(content, line_number) - super(content) - @line_number = line_number - end - - def raw - "" - end - - def child(string) - Token.new(string, @line_number) - end - end -end diff --git a/lib/liquid/tokenizer.rb b/lib/liquid/tokenizer.rb index a7d67ad2..7ea04075 100644 --- a/lib/liquid/tokenizer.rb +++ b/lib/liquid/tokenizer.rb @@ -1,13 +1,17 @@ module Liquid class Tokenizer + attr_reader :line_number + def initialize(source, line_numbers = false) @source = source - @line_numbers = line_numbers + @line_number = 1 if line_numbers @tokens = tokenize end def shift - @tokens.shift + token = @tokens.shift + @line_number += token.count("\n") if @line_number && token + token end private @@ -17,21 +21,11 @@ module Liquid return [] if @source.to_s.empty? tokens = @source.split(TemplateParser) - tokens = @line_numbers ? calculate_line_numbers(tokens) : tokens # removes the rogue empty element at the beginning of the array tokens.shift if tokens[0] && tokens[0].empty? tokens end - - def calculate_line_numbers(tokens) - current_line = 1 - tokens.map do |token| - Token.new(token, current_line).tap do - current_line += token.count("\n") - end - end - end end end diff --git a/lib/liquid/variable.rb b/lib/liquid/variable.rb index d9fdf1c5..344bed05 100644 --- a/lib/liquid/variable.rb +++ b/lib/liquid/variable.rb @@ -11,14 +11,14 @@ module Liquid # class Variable FilterParser = /(?:\s+|#{QuotedFragment}|#{ArgumentSeparator})+/o - attr_accessor :filters, :name, :warnings - attr_accessor :line_number + attr_accessor :filters, :name, :line_number include ParserSwitching - def initialize(markup, options = {}) + def initialize(markup, parse_context) @markup = markup @name = nil - @options = options || {} + @options = parse_context + @line_number = parse_context.line_number parse_with_selected_parser(markup) end diff --git a/test/integration/filter_test.rb b/test/integration/filter_test.rb index 1e904ab1..d3c880e8 100644 --- a/test/integration/filter_test.rb +++ b/test/integration/filter_test.rb @@ -39,13 +39,13 @@ class FiltersTest < Minitest::Test @context['var'] = 1000 @context.add_filters(MoneyFilter) - assert_equal ' 1000$ ', Variable.new("var | money").render(@context) + assert_equal ' 1000$ ', Template.parse("{{var | money}}").render(@context) end def test_underscore_in_filter_name @context['var'] = 1000 @context.add_filters(MoneyFilter) - assert_equal ' 1000$ ', Variable.new("var | money_with_underscore").render(@context) + assert_equal ' 1000$ ', Template.parse("{{var | money_with_underscore}}").render(@context) end def test_second_filter_overwrites_first @@ -53,20 +53,20 @@ class FiltersTest < Minitest::Test @context.add_filters(MoneyFilter) @context.add_filters(CanadianMoneyFilter) - assert_equal ' 1000$ CAD ', Variable.new("var | money").render(@context) + assert_equal ' 1000$ CAD ', Template.parse("{{var | money}}").render(@context) end def test_size @context['var'] = 'abcd' @context.add_filters(MoneyFilter) - assert_equal 4, Variable.new("var | size").render(@context) + assert_equal '4', Template.parse("{{var | size}}").render(@context) end def test_join @context['var'] = [1, 2, 3, 4] - assert_equal "1 2 3 4", Variable.new("var | join").render(@context) + assert_equal "1 2 3 4", Template.parse("{{var | join}}").render(@context) end def test_sort @@ -76,11 +76,11 @@ class FiltersTest < Minitest::Test @context['arrays'] = ['flower', 'are'] @context['case_sensitive'] = ['sensitive', 'Expected', 'case'] - assert_equal [1, 2, 3, 4], Variable.new("numbers | sort").render(@context) - assert_equal ['alphabetic', 'as', 'expected'], Variable.new("words | sort").render(@context) - assert_equal [3], Variable.new("value | sort").render(@context) - assert_equal ['are', 'flower'], Variable.new("arrays | sort").render(@context) - assert_equal ['Expected', 'case', 'sensitive'], Variable.new("case_sensitive | sort").render(@context) + assert_equal '1 2 3 4', Template.parse("{{numbers | sort | join}}").render(@context) + assert_equal 'alphabetic as expected', Template.parse("{{words | sort | join}}").render(@context) + assert_equal '3', Template.parse("{{value | sort}}").render(@context) + assert_equal 'are flower', Template.parse("{{arrays | sort | join}}").render(@context) + assert_equal 'Expected case sensitive', Template.parse("{{case_sensitive | sort | join}}").render(@context) end def test_sort_natural @@ -89,19 +89,13 @@ class FiltersTest < Minitest::Test @context['objects'] = [TestObject.new('A'), TestObject.new('b'), TestObject.new('C')] # Test strings - assert_equal ['Assert', 'case', 'Insensitive'], Variable.new("words | sort_natural").render(@context) + assert_equal 'Assert case Insensitive', Template.parse("{{words | sort_natural | join}}").render(@context) # Test hashes - sorted = Variable.new("hashes | sort_natural: 'a'").render(@context) - assert_equal sorted[0]['a'], 'A' - assert_equal sorted[1]['a'], 'b' - assert_equal sorted[2]['a'], 'C' + assert_equal 'A b C', Template.parse("{{hashes | sort_natural: 'a' | map: 'a' | join}}").render(@context) # Test objects - sorted = Variable.new("objects | sort_natural: 'a'").render(@context) - assert_equal sorted[0].a, 'A' - assert_equal sorted[1].a, 'b' - assert_equal sorted[2].a, 'C' + assert_equal 'A b C', Template.parse("{{objects | sort_natural: 'a' | map: 'a' | join}}").render(@context) end def test_compact @@ -110,49 +104,44 @@ class FiltersTest < Minitest::Test @context['objects'] = [TestObject.new('A'), TestObject.new(nil), TestObject.new('C')] # Test strings - assert_equal ['a', 'b', 'c'], Variable.new("words | compact").render(@context) + assert_equal 'a b c', Template.parse("{{words | compact | join}}").render(@context) # Test hashes - sorted = Variable.new("hashes | compact: 'a'").render(@context) - assert_equal sorted[0]['a'], 'A' - assert_equal sorted[1]['a'], 'C' - assert_nil sorted[2] + assert_equal 'A C', Template.parse("{{hashes | compact: 'a' | map: 'a' | join}}").render(@context) # Test objects - sorted = Variable.new("objects | compact: 'a'").render(@context) - assert_equal sorted[0].a, 'A' - assert_equal sorted[1].a, 'C' - assert_nil sorted[2] + assert_equal 'A C', Template.parse("{{objects | compact: 'a' | map: 'a' | join}}").render(@context) end def test_strip_html @context['var'] = "bla blub" - assert_equal "bla blub", Variable.new("var | strip_html").render(@context) + assert_equal "bla blub", Template.parse("{{ var | strip_html }}").render(@context) end def test_strip_html_ignore_comments_with_html @context['var'] = "bla blub" - assert_equal "bla blub", Variable.new("var | strip_html").render(@context) + assert_equal "bla blub", Template.parse("{{ var | strip_html }}").render(@context) end def test_capitalize @context['var'] = "blub" - assert_equal "Blub", Variable.new("var | capitalize").render(@context) + assert_equal "Blub", Template.parse("{{ var | capitalize }}").render(@context) end def test_nonexistent_filter_is_ignored @context['var'] = 1000 - assert_equal 1000, Variable.new("var | xyzzy").render(@context) + assert_equal '1000', Template.parse("{{ var | xyzzy }}").render(@context) end def test_filter_with_keyword_arguments @context['surname'] = 'john' + @context['input'] = 'hello %{first_name}, %{last_name}' @context.add_filters(SubstituteFilter) - output = Variable.new(%( 'hello %{first_name}, %{last_name}' | substitute: first_name: surname, last_name: 'doe' )).render(@context) + output = Template.parse(%({{ input | substitute: first_name: surname, last_name: 'doe' }})).render(@context) assert_equal 'hello john, doe', output end @@ -181,7 +170,7 @@ class FiltersInTemplate < Minitest::Test end end # FiltersTest -class TestObject +class TestObject < Liquid::Drop attr_accessor :a def initialize(a) @a = a diff --git a/test/unit/tag_unit_test.rb b/test/unit/tag_unit_test.rb index c95cfca0..c4b901b1 100644 --- a/test/unit/tag_unit_test.rb +++ b/test/unit/tag_unit_test.rb @@ -4,18 +4,18 @@ class TagUnitTest < Minitest::Test include Liquid def test_tag - tag = Tag.parse('tag', [], [], {}) + tag = Tag.parse('tag', "", Tokenizer.new(""), ParseContext.new) assert_equal 'liquid::tag', tag.name assert_equal '', tag.render(Context.new) end def test_return_raw_text_of_tag - tag = Tag.parse("long_tag", "param1, param2, param3", [], {}) + tag = Tag.parse("long_tag", "param1, param2, param3", Tokenizer.new(""), ParseContext.new) assert_equal("long_tag param1, param2, param3", tag.raw) end def test_tag_name_should_return_name_of_the_tag - tag = Tag.parse("some_tag", [], [], {}) + tag = Tag.parse("some_tag", "", Tokenizer.new(""), ParseContext.new) assert_equal 'some_tag', tag.tag_name end end diff --git a/test/unit/tokenizer_unit_test.rb b/test/unit/tokenizer_unit_test.rb index e9cabfa0..de84c1fb 100644 --- a/test/unit/tokenizer_unit_test.rb +++ b/test/unit/tokenizer_unit_test.rb @@ -22,20 +22,34 @@ class TokenizerTest < Minitest::Test end def test_calculate_line_numbers_per_token_with_profiling - assert_equal [1], tokenize("{{funk}}", true).map(&:line_number) - assert_equal [1, 1, 1], tokenize(" {{funk}} ", true).map(&:line_number) - assert_equal [1, 2, 2], tokenize("\n{{funk}}\n", true).map(&:line_number) - assert_equal [1, 1, 3], tokenize(" {{\n funk \n}} ", true).map(&:line_number) + assert_equal [1], tokenize_line_numbers("{{funk}}") + assert_equal [1, 1, 1], tokenize_line_numbers(" {{funk}} ") + assert_equal [1, 2, 2], tokenize_line_numbers("\n{{funk}}\n") + assert_equal [1, 1, 3], tokenize_line_numbers(" {{\n funk \n}} ") end private - def tokenize(source, line_numbers = false) - tokenizer = Liquid::Tokenizer.new(source, line_numbers) + def tokenize(source) + tokenizer = Liquid::Tokenizer.new(source) tokens = [] while t = tokenizer.shift tokens << t end tokens end + + def tokenize_line_numbers(source) + tokenizer = Liquid::Tokenizer.new(source, true) + line_numbers = [] + loop do + line_number = tokenizer.line_number + if tokenizer.shift + line_numbers << line_number + else + break + end + end + line_numbers + end end diff --git a/test/unit/variable_unit_test.rb b/test/unit/variable_unit_test.rb index 5119b2d7..5a21ace8 100644 --- a/test/unit/variable_unit_test.rb +++ b/test/unit/variable_unit_test.rb @@ -4,133 +4,133 @@ class VariableUnitTest < Minitest::Test include Liquid def test_variable - var = Variable.new('hello') + var = create_variable('hello') assert_equal VariableLookup.new('hello'), var.name end def test_filters - var = Variable.new('hello | textileze') + var = create_variable('hello | textileze') assert_equal VariableLookup.new('hello'), var.name assert_equal [['textileze', []]], var.filters - var = Variable.new('hello | textileze | paragraph') + var = create_variable('hello | textileze | paragraph') assert_equal VariableLookup.new('hello'), var.name assert_equal [['textileze', []], ['paragraph', []]], var.filters - var = Variable.new(%( hello | strftime: '%Y')) + var = create_variable(%( hello | strftime: '%Y')) assert_equal VariableLookup.new('hello'), var.name assert_equal [['strftime', ['%Y']]], var.filters - var = Variable.new(%( 'typo' | link_to: 'Typo', true )) + var = create_variable(%( 'typo' | link_to: 'Typo', true )) assert_equal 'typo', var.name assert_equal [['link_to', ['Typo', true]]], var.filters - var = Variable.new(%( 'typo' | link_to: 'Typo', false )) + var = create_variable(%( 'typo' | link_to: 'Typo', false )) assert_equal 'typo', var.name assert_equal [['link_to', ['Typo', false]]], var.filters - var = Variable.new(%( 'foo' | repeat: 3 )) + var = create_variable(%( 'foo' | repeat: 3 )) assert_equal 'foo', var.name assert_equal [['repeat', [3]]], var.filters - var = Variable.new(%( 'foo' | repeat: 3, 3 )) + var = create_variable(%( 'foo' | repeat: 3, 3 )) assert_equal 'foo', var.name assert_equal [['repeat', [3, 3]]], var.filters - var = Variable.new(%( 'foo' | repeat: 3, 3, 3 )) + var = create_variable(%( 'foo' | repeat: 3, 3, 3 )) assert_equal 'foo', var.name assert_equal [['repeat', [3, 3, 3]]], var.filters - var = Variable.new(%( hello | strftime: '%Y, okay?')) + var = create_variable(%( hello | strftime: '%Y, okay?')) assert_equal VariableLookup.new('hello'), var.name assert_equal [['strftime', ['%Y, okay?']]], var.filters - var = Variable.new(%( hello | things: "%Y, okay?", 'the other one')) + var = create_variable(%( hello | things: "%Y, okay?", 'the other one')) assert_equal VariableLookup.new('hello'), var.name assert_equal [['things', ['%Y, okay?', 'the other one']]], var.filters end def test_filter_with_date_parameter - var = Variable.new(%( '2006-06-06' | date: "%m/%d/%Y")) + var = create_variable(%( '2006-06-06' | date: "%m/%d/%Y")) assert_equal '2006-06-06', var.name assert_equal [['date', ['%m/%d/%Y']]], var.filters end def test_filters_without_whitespace - var = Variable.new('hello | textileze | paragraph') + var = create_variable('hello | textileze | paragraph') assert_equal VariableLookup.new('hello'), var.name assert_equal [['textileze', []], ['paragraph', []]], var.filters - var = Variable.new('hello|textileze|paragraph') + var = create_variable('hello|textileze|paragraph') assert_equal VariableLookup.new('hello'), var.name assert_equal [['textileze', []], ['paragraph', []]], var.filters - var = Variable.new("hello|replace:'foo','bar'|textileze") + var = create_variable("hello|replace:'foo','bar'|textileze") assert_equal VariableLookup.new('hello'), var.name assert_equal [['replace', ['foo', 'bar']], ['textileze', []]], var.filters end def test_symbol - var = Variable.new("http://disney.com/logo.gif | image: 'med' ", error_mode: :lax) + var = create_variable("http://disney.com/logo.gif | image: 'med' ", error_mode: :lax) assert_equal VariableLookup.new('http://disney.com/logo.gif'), var.name assert_equal [['image', ['med']]], var.filters end def test_string_to_filter - var = Variable.new("'http://disney.com/logo.gif' | image: 'med' ") + var = create_variable("'http://disney.com/logo.gif' | image: 'med' ") assert_equal 'http://disney.com/logo.gif', var.name assert_equal [['image', ['med']]], var.filters end def test_string_single_quoted - var = Variable.new(%( "hello" )) + var = create_variable(%( "hello" )) assert_equal 'hello', var.name end def test_string_double_quoted - var = Variable.new(%( 'hello' )) + var = create_variable(%( 'hello' )) assert_equal 'hello', var.name end def test_integer - var = Variable.new(%( 1000 )) + var = create_variable(%( 1000 )) assert_equal 1000, var.name end def test_float - var = Variable.new(%( 1000.01 )) + var = create_variable(%( 1000.01 )) assert_equal 1000.01, var.name end def test_dashes - assert_equal VariableLookup.new('foo-bar'), Variable.new('foo-bar').name - assert_equal VariableLookup.new('foo-bar-2'), Variable.new('foo-bar-2').name + assert_equal VariableLookup.new('foo-bar'), create_variable('foo-bar').name + assert_equal VariableLookup.new('foo-bar-2'), create_variable('foo-bar-2').name with_error_mode :strict do - assert_raises(Liquid::SyntaxError) { Variable.new('foo - bar') } - assert_raises(Liquid::SyntaxError) { Variable.new('-foo') } - assert_raises(Liquid::SyntaxError) { Variable.new('2foo') } + assert_raises(Liquid::SyntaxError) { create_variable('foo - bar') } + assert_raises(Liquid::SyntaxError) { create_variable('-foo') } + assert_raises(Liquid::SyntaxError) { create_variable('2foo') } end end def test_string_with_special_chars - var = Variable.new(%( 'hello! $!@.;"ddasd" ' )) + var = create_variable(%( 'hello! $!@.;"ddasd" ' )) assert_equal 'hello! $!@.;"ddasd" ', var.name end def test_string_dot - var = Variable.new(%( test.test )) + var = create_variable(%( test.test )) assert_equal VariableLookup.new('test.test'), var.name end def test_filter_with_keyword_arguments - var = Variable.new(%( hello | things: greeting: "world", farewell: 'goodbye')) + var = create_variable(%( hello | things: greeting: "world", farewell: 'goodbye')) assert_equal VariableLookup.new('hello'), var.name assert_equal [['things', [], { 'greeting' => 'world', 'farewell' => 'goodbye' }]], var.filters end def test_lax_filter_argument_parsing - var = Variable.new(%( number_of_comments | pluralize: 'comment': 'comments' ), error_mode: :lax) + var = create_variable(%( number_of_comments | pluralize: 'comment': 'comments' ), error_mode: :lax) assert_equal VariableLookup.new('number_of_comments'), var.name assert_equal [['pluralize', ['comment', 'comments']]], var.filters end @@ -138,13 +138,13 @@ class VariableUnitTest < Minitest::Test def test_strict_filter_argument_parsing with_error_mode(:strict) do assert_raises(SyntaxError) do - Variable.new(%( number_of_comments | pluralize: 'comment': 'comments' )) + create_variable(%( number_of_comments | pluralize: 'comment': 'comments' )) end end end def test_output_raw_source_of_variable - var = Variable.new(%( name_of_variable | upcase )) + var = create_variable(%( name_of_variable | upcase )) assert_equal " name_of_variable | upcase ", var.raw end @@ -153,4 +153,10 @@ class VariableUnitTest < Minitest::Test assert_equal 'a', lookup.name assert_equal ['b', 'c'], lookup.lookups end + + private + + def create_variable(markup, options = {}) + Variable.new(markup, ParseContext.new(options)) + end end From 920e1df643541bb461fdbd77f513f8636b9de11a Mon Sep 17 00:00:00 2001 From: Dylan Thacker-Smith Date: Sun, 5 Jul 2015 16:07:47 -0400 Subject: [PATCH 2/4] Rescue and re-raise syntax errors in Template#parse to add line numbers. This can be done now that the parse context has the line number information, so it doesn't need to be added on closer to the original exception. This has the advantage of not having to rescue and re-raise the exception multiple times, and simplifies liquid-c which would otherwise have to rescue the exception in BlockBody#parse. --- lib/liquid/block_body.rb | 47 +++++++++++++++------------------- lib/liquid/parser_switching.rb | 2 +- lib/liquid/template.rb | 3 +++ 3 files changed, 25 insertions(+), 27 deletions(-) diff --git a/lib/liquid/block_body.rb b/lib/liquid/block_body.rb index b93d4a6d..9efa2141 100644 --- a/lib/liquid/block_body.rb +++ b/lib/liquid/block_body.rb @@ -15,37 +15,32 @@ module Liquid def parse(tokenizer, parse_context) parse_context.line_number = tokenizer.line_number while token = tokenizer.shift - begin - unless token.empty? - case - when token.start_with?(TAGSTART) - if token =~ FullToken - tag_name = $1 - markup = $2 - # fetch the tag from registered blocks - if tag = registered_tags[tag_name] - new_tag = tag.parse(tag_name, markup, tokenizer, parse_context) - @blank &&= new_tag.blank? - @nodelist << new_tag - else - # end parsing if we reach an unknown tag and let the caller decide - # determine how to proceed - return yield tag_name, markup - end + unless token.empty? + case + when token.start_with?(TAGSTART) + if token =~ FullToken + tag_name = $1 + markup = $2 + # fetch the tag from registered blocks + if tag = registered_tags[tag_name] + new_tag = tag.parse(tag_name, markup, tokenizer, parse_context) + @blank &&= new_tag.blank? + @nodelist << new_tag else - raise_missing_tag_terminator(token, parse_context) + # end parsing if we reach an unknown tag and let the caller decide + # determine how to proceed + return yield tag_name, markup end - when token.start_with?(VARSTART) - @nodelist << create_variable(token, parse_context) - @blank = false else - @nodelist << token - @blank &&= !!(token =~ /\A\s*\z/) + raise_missing_tag_terminator(token, parse_context) end + when token.start_with?(VARSTART) + @nodelist << create_variable(token, parse_context) + @blank = false + else + @nodelist << token + @blank &&= !!(token =~ /\A\s*\z/) end - rescue SyntaxError => e - e.line_number ||= parse_context.line_number - raise end parse_context.line_number = tokenizer.line_number end diff --git a/lib/liquid/parser_switching.rb b/lib/liquid/parser_switching.rb index 631a9a55..363e93e7 100644 --- a/lib/liquid/parser_switching.rb +++ b/lib/liquid/parser_switching.rb @@ -8,7 +8,6 @@ module Liquid begin return strict_parse_with_error_context(markup) rescue SyntaxError => e - e.line_number = line_number @options.warnings << e return lax_parse(markup) end @@ -20,6 +19,7 @@ module Liquid def strict_parse_with_error_context(markup) strict_parse(markup) rescue SyntaxError => e + e.line_number = line_number e.markup_context = markup_context(markup) raise e end diff --git a/lib/liquid/template.rb b/lib/liquid/template.rb index 07d56b29..57aa291d 100644 --- a/lib/liquid/template.rb +++ b/lib/liquid/template.rb @@ -120,6 +120,9 @@ module Liquid @root = Document.parse(tokenize(source), parse_context) @warnings = parse_context.warnings self + rescue SyntaxError => e + e.line_number ||= parse_context.line_number + raise end def registers From c45310170b93a4fe549750f930f85944f72502ca Mon Sep 17 00:00:00 2001 From: Dylan Thacker-Smith Date: Wed, 8 Jul 2015 19:21:04 -0400 Subject: [PATCH 3/4] Use parse_context or options instead of @options. --- lib/liquid/block.rb | 10 +++++----- lib/liquid/block_body.rb | 8 ++++---- lib/liquid/parser_switching.rb | 4 ++-- lib/liquid/tag.rb | 4 ++-- lib/liquid/tags/raw.rb | 6 +++--- lib/liquid/variable.rb | 2 ++ 6 files changed, 18 insertions(+), 16 deletions(-) diff --git a/lib/liquid/block.rb b/lib/liquid/block.rb index 71506423..eed04757 100644 --- a/lib/liquid/block.rb +++ b/lib/liquid/block.rb @@ -26,14 +26,14 @@ module Liquid def unknown_tag(tag, _params, _tokens) case tag when 'else'.freeze - raise SyntaxError.new(options[:locale].t("errors.syntax.unexpected_else".freeze, + raise SyntaxError.new(parse_context.locale.t("errors.syntax.unexpected_else".freeze, block_name: block_name)) when 'end'.freeze - raise SyntaxError.new(options[:locale].t("errors.syntax.invalid_delimiter".freeze, + raise SyntaxError.new(parse_context.locale.t("errors.syntax.invalid_delimiter".freeze, block_name: block_name, block_delimiter: block_delimiter)) else - raise SyntaxError.new(options[:locale].t("errors.syntax.unknown_tag".freeze, tag: tag)) + raise SyntaxError.new(parse_context.locale.t("errors.syntax.unknown_tag".freeze, tag: tag)) end end @@ -48,12 +48,12 @@ module Liquid protected def parse_body(body, tokens) - body.parse(tokens, options) do |end_tag_name, end_tag_params| + body.parse(tokens, parse_context) do |end_tag_name, end_tag_params| @blank &&= body.blank? return false if end_tag_name == block_delimiter unless end_tag_name - raise SyntaxError.new(@options[:locale].t("errors.syntax.tag_never_closed".freeze, block_name: block_name)) + raise SyntaxError.new(parse_context.locale.t("errors.syntax.tag_never_closed".freeze, block_name: block_name)) end # this tag is not registered with the system diff --git a/lib/liquid/block_body.rb b/lib/liquid/block_body.rb index 9efa2141..60b28b53 100644 --- a/lib/liquid/block_body.rb +++ b/lib/liquid/block_body.rb @@ -105,12 +105,12 @@ module Liquid raise_missing_variable_terminator(token, parse_context) end - def raise_missing_tag_terminator(token, options) - raise SyntaxError.new(options[:locale].t("errors.syntax.tag_termination".freeze, token: token, tag_end: TagEnd.inspect)) + def raise_missing_tag_terminator(token, parse_context) + raise SyntaxError.new(parse_context.locale.t("errors.syntax.tag_termination".freeze, token: token, tag_end: TagEnd.inspect)) end - def raise_missing_variable_terminator(token, options) - raise SyntaxError.new(options[:locale].t("errors.syntax.variable_termination".freeze, token: token, tag_end: VariableEnd.inspect)) + def raise_missing_variable_terminator(token, parse_context) + raise SyntaxError.new(parse_context.locale.t("errors.syntax.variable_termination".freeze, token: token, tag_end: VariableEnd.inspect)) end def registered_tags diff --git a/lib/liquid/parser_switching.rb b/lib/liquid/parser_switching.rb index 363e93e7..3aa664ab 100644 --- a/lib/liquid/parser_switching.rb +++ b/lib/liquid/parser_switching.rb @@ -1,14 +1,14 @@ module Liquid module ParserSwitching def parse_with_selected_parser(markup) - case @options.error_mode + case parse_context.error_mode when :strict then strict_parse_with_error_context(markup) when :lax then lax_parse(markup) when :warn begin return strict_parse_with_error_context(markup) rescue SyntaxError => e - @options.warnings << e + parse_context.warnings << e return lax_parse(markup) end end diff --git a/lib/liquid/tag.rb b/lib/liquid/tag.rb index 36993c07..8375477a 100644 --- a/lib/liquid/tag.rb +++ b/lib/liquid/tag.rb @@ -1,7 +1,7 @@ module Liquid class Tag - attr_accessor :options - attr_reader :nodelist, :tag_name, :line_number + attr_reader :nodelist, :tag_name, :line_number, :options + alias_method :parse_context, :options include ParserSwitching class << self diff --git a/lib/liquid/tags/raw.rb b/lib/liquid/tags/raw.rb index 26222be5..73ac74c7 100644 --- a/lib/liquid/tags/raw.rb +++ b/lib/liquid/tags/raw.rb @@ -3,11 +3,11 @@ module Liquid Syntax = /\A\s*\z/ FullTokenPossiblyInvalid = /\A(.*)#{TagStart}\s*(\w+)\s*(.*)?#{TagEnd}\z/om - def initialize(tag_name, markup, options) + def initialize(tag_name, markup, parse_context) super unless markup =~ Syntax - raise SyntaxError.new(@options[:locale].t("errors.syntax.tag_unexpected_args".freeze, tag: tag_name)) + raise SyntaxError.new(parse_context.locale.t("errors.syntax.tag_unexpected_args".freeze, tag: tag_name)) end end @@ -21,7 +21,7 @@ module Liquid @body << token unless token.empty? end - raise SyntaxError.new(@options[:locale].t("errors.syntax.tag_never_closed".freeze, block_name: block_name)) + raise SyntaxError.new(parse_context.locale.t("errors.syntax.tag_never_closed".freeze, block_name: block_name)) end def render(_context) diff --git a/lib/liquid/variable.rb b/lib/liquid/variable.rb index 344bed05..b96181ac 100644 --- a/lib/liquid/variable.rb +++ b/lib/liquid/variable.rb @@ -12,6 +12,8 @@ module Liquid class Variable FilterParser = /(?:\s+|#{QuotedFragment}|#{ArgumentSeparator})+/o attr_accessor :filters, :name, :line_number + attr_reader :options + alias_method :parse_context, :options include ParserSwitching def initialize(markup, parse_context) From 73834a7e520742774bfa90ecc2e7d8b0b01308a1 Mon Sep 17 00:00:00 2001 From: Dylan Thacker-Smith Date: Wed, 8 Jul 2015 19:27:24 -0400 Subject: [PATCH 4/4] Use reject rather than dup and delete. --- lib/liquid/parse_context.rb | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/lib/liquid/parse_context.rb b/lib/liquid/parse_context.rb index 99274816..baf63af9 100644 --- a/lib/liquid/parse_context.rb +++ b/lib/liquid/parse_context.rb @@ -27,9 +27,7 @@ module Liquid if dont_pass == true { locale: locale } elsif dont_pass.is_a?(Array) - opts = @template_options.dup - dont_pass.each { |o| opts.delete(o) } - opts + @template_options.reject { |k, v| dont_pass.include?(k) } else @template_options end