From 38600338cf65e8157b726c97b75fe5bdcd6640b8 Mon Sep 17 00:00:00 2001 From: Peter Zhu Date: Thu, 12 Nov 2020 15:58:15 -0500 Subject: [PATCH] Address comments --- lib/liquid/condition.rb | 8 +++ lib/liquid/parser.rb | 9 +-- lib/liquid/range_lookup.rb | 18 +++-- lib/liquid/tags/if.rb | 16 ++--- lib/liquid/variable.rb | 2 +- lib/liquid/variable_lookup.rb | 92 ++++++++++++++------------ test/integration/expression_test.rb | 18 ++--- test/unit/parser_unit_test.rb | 18 +++-- test/unit/variable_lookup_unit_test.rb | 7 +- 9 files changed, 104 insertions(+), 84 deletions(-) diff --git a/lib/liquid/condition.rb b/lib/liquid/condition.rb index 8b7dd65..007291d 100644 --- a/lib/liquid/condition.rb +++ b/lib/liquid/condition.rb @@ -49,6 +49,14 @@ module Liquid @@method_literals[markup] || parse_context.parse_expression(markup) end + def self.strict_parse_expression(parse_context, p) + if p.look(:id) && !p.look(:dot, 1) && !p.look(:open_square, 1) + parse_expression(parse_context, p.consume) + else + p.expression + end + end + attr_reader :attachment, :child_condition attr_accessor :left, :operator, :right diff --git a/lib/liquid/parser.rb b/lib/liquid/parser.rb index 1656d9d..742d2ab 100644 --- a/lib/liquid/parser.rb +++ b/lib/liquid/parser.rb @@ -60,18 +60,15 @@ module Liquid when :string consume[1..-2] when :number - Expression.parse(consume) + num_str = consume + num_str.include?('.') ? num_str.to_f : num_str.to_i when :open_round consume first = expression consume(:dotdot) last = expression consume(:close_round) - if first.respond_to?(:evaluate) || last.respond_to?(:evaluate) - RangeLookup.new(first, last) - else - first.to_i..last.to_i - end + RangeLookup.build(first, last) else raise SyntaxError, "#{token} is not a valid expression" end diff --git a/lib/liquid/range_lookup.rb b/lib/liquid/range_lookup.rb index fc8c8e6..9f80c5d 100644 --- a/lib/liquid/range_lookup.rb +++ b/lib/liquid/range_lookup.rb @@ -5,6 +5,10 @@ module Liquid def self.parse(start_markup, end_markup) start_obj = Expression.parse(start_markup) end_obj = Expression.parse(end_markup) + build(start_obj, end_obj) + end + + def self.build(start_obj, end_obj) if start_obj.respond_to?(:evaluate) || end_obj.respond_to?(:evaluate) new(start_obj, end_obj) else @@ -12,21 +16,21 @@ module Liquid end end - attr_reader :start_obj, :end_obj + attr_reader :start_expr, :end_expr - def initialize(start_obj, end_obj) - @start_obj = start_obj - @end_obj = end_obj + def initialize(start_expr, end_expr) + @start_expr = start_expr + @end_expr = end_expr end def evaluate(context) - start_int = to_integer(context.evaluate(@start_obj)) - end_int = to_integer(context.evaluate(@end_obj)) + start_int = to_integer(context.evaluate(@start_expr)) + end_int = to_integer(context.evaluate(@end_expr)) start_int..end_int end def ==(other) - self.class == other.class && start_obj == other.start_obj && end_obj == other.end_obj + self.class == other.class && start_expr == other.start_expr && end_expr == other.end_expr end private diff --git a/lib/liquid/tags/if.rb b/lib/liquid/tags/if.rb index 932a547..1391a26 100644 --- a/lib/liquid/tags/if.rb +++ b/lib/liquid/tags/if.rb @@ -75,6 +75,10 @@ module Liquid Condition.parse_expression(parse_context, markup) end + def strict_parse_expression(p) + Condition.strict_parse_expression(parse_context, p) + end + def lax_parse(markup) expressions = markup.scan(ExpressionsAndOperators) raise SyntaxError, options[:locale].t("errors.syntax.if") unless expressions.pop =~ Syntax @@ -114,23 +118,15 @@ module Liquid end def parse_comparison(p) - a = parse_operand_expression(p) + a = strict_parse_expression(p) if (op = p.consume?(:comparison)) - b = parse_operand_expression(p) + b = strict_parse_expression(p) Condition.new(a, op, b) else Condition.new(a) end end - def parse_operand_expression(p) - if p.look(:id) && !p.look(:dot, 1) && !p.look(:open_square, 1) - parse_expression(p.consume) - else - p.expression - end - end - class ParseTreeVisitor < Liquid::ParseTreeVisitor def children @node.blocks diff --git a/lib/liquid/variable.rb b/lib/liquid/variable.rb index 4d31b6e..e8b88fa 100644 --- a/lib/liquid/variable.rb +++ b/lib/liquid/variable.rb @@ -69,7 +69,7 @@ module Liquid while p.consume?(:pipe) filtername = p.consume(:id) filterargs = p.consume?(:colon) ? p.arguments : [[]] - @filters << [filtername] + filterargs + @filters << [filtername, *filterargs] end p.consume(:end_of_string) end diff --git a/lib/liquid/variable_lookup.rb b/lib/liquid/variable_lookup.rb index cb59ddb..5fa983c 100644 --- a/lib/liquid/variable_lookup.rb +++ b/lib/liquid/variable_lookup.rb @@ -7,56 +7,60 @@ module Liquid attr_reader :name, :lookups - def self.lax_parse(markup) - lookups = markup.scan(VariableParser) + class << self + def lax_parse(markup) + lookups = markup.scan(VariableParser) - name = lookups.shift - if name =~ SQUARE_BRACKETED - name = Expression.parse(Regexp.last_match(1)) - end - - command_flags = 0 - - lookups.each_index do |i| - lookup = lookups[i] - if lookup =~ SQUARE_BRACKETED - lookups[i] = Expression.parse(Regexp.last_match(1)) - elsif COMMAND_METHODS.include?(lookup) - command_flags |= 1 << i + name = lookups.shift + if name =~ SQUARE_BRACKETED + name = Expression.parse(Regexp.last_match(1)) end - end - new(name, lookups, command_flags) - end + command_flags = 0 - def self.strict_parse(p) - if p.look(:id) - name = p.consume - else - p.consume(:open_square) - name = p.expression - p.consume(:close_square) - end - - lookups = [] - command_flags = 0 - - loop do - if p.consume?(:open_square) - lookups << p.expression - p.consume(:close_square) - elsif p.consume?(:dot) - lookup = p.consume(:id) - lookups << lookup - if COMMAND_METHODS.include?(lookup) - command_flags |= 1 << (lookups.length - 1) + lookups.each_index do |i| + lookup = lookups[i] + if lookup =~ SQUARE_BRACKETED + lookups[i] = Expression.parse(Regexp.last_match(1)) + elsif COMMAND_METHODS.include?(lookup) + command_flags |= 1 << i end - else - break end + + new(name, lookups, command_flags) end - new(name, lookups, command_flags) + def strict_parse(p) + if p.look(:id) + name = p.consume + else + p.consume(:open_square) + name = p.expression + p.consume(:close_square) + end + + lookups = [] + command_flags = 0 + + loop do + if p.consume?(:open_square) + lookups << p.expression + p.consume(:close_square) + elsif p.consume?(:dot) + lookup = p.consume(:id) + lookups << lookup + if COMMAND_METHODS.include?(lookup) + command_flags |= 1 << (lookups.length - 1) + end + else + break + end + end + + new(name, lookups, command_flags) + end + + private :new end def initialize(name, lookups, command_flags) @@ -112,9 +116,9 @@ module Liquid lookups.each do |lookup| str += if lookup.instance_of?(String) - '.' + lookup + "['#{lookup}']" else - '[' + lookup.to_s + ']' + "[#{lookup}]" end end str diff --git a/test/integration/expression_test.rb b/test/integration/expression_test.rb index e2577ce..5301985 100644 --- a/test/integration/expression_test.rb +++ b/test/integration/expression_test.rb @@ -40,13 +40,15 @@ class ExpressionTest < Minitest::Test private def parse_and_eval(markup, **assigns) - if Liquid::Template.error_mode == :strict - p = Liquid::Parser.new(markup) - p.expression - else - expression = Liquid::Expression.parse(markup) - context = Liquid::Context.new(assigns) - context.evaluate(expression) - end + expression = + if Liquid::Template.error_mode == :strict + p = Liquid::Parser.new(markup) + p.expression + else + Liquid::Expression.parse(markup) + end + + context = Liquid::Context.new(assigns) + context.evaluate(expression) end end diff --git a/test/unit/parser_unit_test.rb b/test/unit/parser_unit_test.rb index d2e5e49..77b7c0b 100644 --- a/test/unit/parser_unit_test.rb +++ b/test/unit/parser_unit_test.rb @@ -47,9 +47,9 @@ class ParserUnitTest < Minitest::Test def test_expressions p = Parser.new("hi.there hi?[5].there? hi.there.bob") - assert_equal(VariableLookup.new('hi', ['there'], 0), p.expression) - assert_equal(VariableLookup.new('hi?', [5, 'there?'], 0), p.expression) - assert_equal(VariableLookup.new('hi', ['there', 'bob'], 0), p.expression) + assert_equal(VariableLookup.send(:new, 'hi', ['there'], 0), p.expression) + assert_equal(VariableLookup.send(:new, 'hi?', [5, 'there?'], 0), p.expression) + assert_equal(VariableLookup.send(:new, 'hi', ['there', 'bob'], 0), p.expression) p = Parser.new("nil true false") assert_nil(p.expression) @@ -67,15 +67,21 @@ class ParserUnitTest < Minitest::Test p = Parser.new("(5..7) (1.5..9.6) (young..old) (hi[5].wat..old)") assert_equal(5..7, p.expression) assert_equal(1..9, p.expression) - assert_equal(RangeLookup.new(VariableLookup.new('young', [], 0), VariableLookup.new('old', [], 0)), p.expression) - assert_equal(RangeLookup.new(VariableLookup.new('hi', [5, "wat"], 0), VariableLookup.new('old', [], 0)), p.expression) + assert_equal( + RangeLookup.new(VariableLookup.send(:new, 'young', [], 0), VariableLookup.send(:new, 'old', [], 0)), + p.expression + ) + assert_equal( + RangeLookup.new(VariableLookup.send(:new, 'hi', [5, "wat"], 0), VariableLookup.send(:new, 'old', [], 0)), + p.expression + ) end def test_arguments p = Parser.new("filter: hi.there[5], keyarg: 7") assert_equal('filter', p.consume(:id)) assert_equal(':', p.consume(:colon)) - assert_equal([[VariableLookup.new("hi", ["there", 5], 0)], { "keyarg" => 7 }], p.arguments) + assert_equal([[VariableLookup.send(:new, "hi", ["there", 5], 0)], { "keyarg" => 7 }], p.arguments) end def test_invalid_expression diff --git a/test/unit/variable_lookup_unit_test.rb b/test/unit/variable_lookup_unit_test.rb index daf6849..f6f7cd7 100644 --- a/test/unit/variable_lookup_unit_test.rb +++ b/test/unit/variable_lookup_unit_test.rb @@ -17,10 +17,13 @@ class VariableLookupUnitTest < Minitest::Test def test_to_s lookup = parse_variable_lookup('a.b.c') - assert_equal('a.b.c', lookup.to_s) + assert_equal("a['b']['c']", lookup.to_s) lookup = parse_variable_lookup('a[b.c].d') - assert_equal('a[b.c].d', lookup.to_s) + assert_equal("a[b['c']]['d']", lookup.to_s) + + lookup = parse_variable_lookup('a["foo.bar"].d') + assert_equal("a['foo.bar']['d']", lookup.to_s) end private