Address comments

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
 

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.build(first, last)
-          RangeLookup.new(first, last)
-        else
-          first.to_i..last.to_i
-        end
       else
         raise SyntaxError, "#{token} is not a valid expression"
       end

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)
+    def initialize(start_expr, end_expr)
-      @start_obj = start_obj
+      @start_expr = start_expr
-      @end_obj   = end_obj
+      @end_expr   = end_expr
     end
 
     def evaluate(context)
-      start_int = to_integer(context.evaluate(@start_obj))
+      start_int = to_integer(context.evaluate(@start_expr))
-      end_int   = to_integer(context.evaluate(@end_obj))
+      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

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

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

lib/liquid/variable_lookup.rb

@@ -7,56 +7,60 @@ module Liquid
 
     attr_reader :name, :lookups
 
-    def self.lax_parse(markup)
+    class << self
-      lookups = markup.scan(VariableParser)
+      def lax_parse(markup)
+        lookups = markup.scan(VariableParser)
 
-      name = lookups.shift
+        name = lookups.shift
-      if name =~ SQUARE_BRACKETED
+        if name =~ SQUARE_BRACKETED
-        name = Expression.parse(Regexp.last_match(1))
+          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
         end
-      end
 
-      new(name, lookups, command_flags)
+        command_flags = 0
-    end
 
-    def self.strict_parse(p)
+        lookups.each_index do |i|
-      if p.look(:id)
+          lookup = lookups[i]
-        name = p.consume
+          if lookup =~ SQUARE_BRACKETED
-      else
+            lookups[i] = Expression.parse(Regexp.last_match(1))
-        p.consume(:open_square)
+          elsif COMMAND_METHODS.include?(lookup)
-        name = p.expression
+            command_flags |= 1 << i
-        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
+
+        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

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
+    expression =
-      p = Liquid::Parser.new(markup)
+      if Liquid::Template.error_mode == :strict
-      p.expression
+        p = Liquid::Parser.new(markup)
-    else
+        p.expression
-      expression = Liquid::Expression.parse(markup)
+      else
-      context = Liquid::Context.new(assigns)
+        Liquid::Expression.parse(markup)
-      context.evaluate(expression)
+      end
-    end
+
+    context = Liquid::Context.new(assigns)
+    context.evaluate(expression)
   end
 end

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.send(:new, 'hi', ['there'], 0), p.expression)
-    assert_equal(VariableLookup.new('hi?', [5, 'there?'], 0), p.expression)
+    assert_equal(VariableLookup.send(:new, 'hi?', [5, 'there?'], 0), p.expression)
-    assert_equal(VariableLookup.new('hi', ['there', 'bob'], 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(
-    assert_equal(RangeLookup.new(VariableLookup.new('hi', [5, "wat"], 0), VariableLookup.new('old', [], 0)), p.expression)
+      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

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