From e911eea3df4465a45d231b41abdf6cadee66801f Mon Sep 17 00:00:00 2001 From: "Charles-P. Clermont" Date: Thu, 4 Dec 2025 15:12:25 -0500 Subject: [PATCH] Remove Condition.op, Condition.right - Remove comparison expression logic --- lib/liquid/condition.rb | 133 ++----------------------------- lib/liquid/tags/case.rb | 4 +- lib/liquid/tags/if.rb | 4 - test/unit/condition_unit_test.rb | 47 ++++++----- 4 files changed, 33 insertions(+), 155 deletions(-) diff --git a/lib/liquid/condition.rb b/lib/liquid/condition.rb index 893706d0..b30ec486 100644 --- a/lib/liquid/condition.rb +++ b/lib/liquid/condition.rb @@ -5,51 +5,15 @@ module Liquid # # Example: # - # c = Condition.new(1, '==', 1) + # c = Condition.new(expr) # c.evaluate #=> true # class Condition # :nodoc: - @@operators = { - '==' => ->(cond, left, right) { cond.send(:equal_variables, left, right) }, - '!=' => ->(cond, left, right) { !cond.send(:equal_variables, left, right) }, - '<>' => ->(cond, left, right) { !cond.send(:equal_variables, left, right) }, - '<' => :<, - '>' => :>, - '>=' => :>=, - '<=' => :<=, - 'contains' => lambda do |_cond, left, right| - if left && right && left.respond_to?(:include?) - right = right.to_s if left.is_a?(String) - left.include?(right) - else - false - end - rescue Encoding::CompatibilityError - # "✅".b.include?("✅") raises Encoding::CompatibilityError despite being materially equal - left.b.include?(right.b) - end, - } - @@method_literals = { - 'blank' => MethodLiteral.new(:blank?, '').freeze, - 'empty' => MethodLiteral.new(:empty?, '').freeze, - } - - def self.operators - @@operators - end - - def self.parse_expression(parser) - markup = parser.expression_string - @@method_literals[markup] || parser.unsafe_parse_expression(markup) - end - attr_reader :attachment, :child_condition - attr_accessor :left, :operator, :right + attr_accessor :left - def initialize(left = nil, operator = nil, right = nil) - @left = left - @operator = operator - @right = right + def initialize(left = nil) + @left = left @child_relation = nil @child_condition = nil @@ -59,7 +23,7 @@ module Liquid condition = self result = nil loop do - result = interpret_condition(condition.left, condition.right, condition.operator, context) + result = context.evaluate(condition.left) case condition.child_relation when :or @@ -101,92 +65,6 @@ module Liquid attr_reader :child_relation private - - def equal_variables(left, right) - if left.is_a?(MethodLiteral) - return call_method_literal(left, right) - end - - if right.is_a?(MethodLiteral) - return call_method_literal(right, left) - end - - left == right - end - - def call_method_literal(literal, value) - method_name = literal.method_name - - # If the object responds to the method (e.g., ActiveSupport is loaded), use it - if value.respond_to?(method_name) - value.send(method_name) - else - # Emulate ActiveSupport's blank?/empty? to make Liquid invariant - # to whether ActiveSupport is loaded or not - case method_name - when :blank? - liquid_blank?(value) - when :empty? - liquid_empty?(value) - else - false - end - end - end - - # Implement blank? semantics matching ActiveSupport - # blank? returns true for nil, false, empty strings, whitespace-only strings, - # empty arrays, and empty hashes - def liquid_blank?(value) - case value - when NilClass, FalseClass - true - when TrueClass, Numeric - false - when String - # Blank if empty or whitespace only (matches ActiveSupport) - value.empty? || value.match?(/\A\s*\z/) - when Array, Hash - value.empty? - else - # Fall back to empty? if available, otherwise false - value.respond_to?(:empty?) ? value.empty? : false - end - end - - # Implement empty? semantics - # Note: nil is NOT empty. empty? checks if a collection has zero elements. - def liquid_empty?(value) - case value - when String, Array, Hash - value.empty? - else - value.respond_to?(:empty?) ? value.empty? : false - end - end - - def interpret_condition(left, right, op, context) - # If the operator is empty this means that the decision statement is just - # a single variable. We can just poll this variable from the context and - # return this as the result. - return context.evaluate(left) if op.nil? - - left = Liquid::Utils.to_liquid_value(context.evaluate(left)) - right = Liquid::Utils.to_liquid_value(context.evaluate(right)) - - operation = self.class.operators[op] || raise(Liquid::ArgumentError, "Unknown operator #{op}") - - if operation.respond_to?(:call) - operation.call(self, left, right) - elsif left.respond_to?(operation) && right.respond_to?(operation) && !left.is_a?(Hash) && !right.is_a?(Hash) - begin - left.send(operation, right) - rescue ::ArgumentError => e - raise Liquid::ArgumentError, e.message - end - end - end - def deprecated_default_context warn("DEPRECATION WARNING: Condition#evaluate without a context argument is deprecated " \ "and will be removed from Liquid 6.0.0.") @@ -197,7 +75,6 @@ module Liquid def children [ @node.left, - @node.right, @node.child_condition, @node.attachment ].compact diff --git a/lib/liquid/tags/case.rb b/lib/liquid/tags/case.rb index 8f58d842..fcc0809f 100644 --- a/lib/liquid/tags/case.rb +++ b/lib/liquid/tags/case.rb @@ -99,8 +99,8 @@ module Liquid parser = @parse_context.new_parser(markup) loop do - expr = Condition.parse_expression(parser) - block = Condition.new(@left, '==', expr) + expr = BinaryExpression.new(@left, '==', Condition.parse_expression(parser)) + block = Condition.new(expr) block.attach(body) @blocks << block diff --git a/lib/liquid/tags/if.rb b/lib/liquid/tags/if.rb index b07a91c7..97d8ba61 100644 --- a/lib/liquid/tags/if.rb +++ b/lib/liquid/tags/if.rb @@ -73,10 +73,6 @@ module Liquid block.attach(new_body) end - def parse_expression(parser) - Condition.parse_expression(parser) - end - def parse_markup(markup) p = @parse_context.new_parser(markup) condition = parse_binary_comparisons(p) diff --git a/test/unit/condition_unit_test.rb b/test/unit/condition_unit_test.rb index d822547e..60350f31 100644 --- a/test/unit/condition_unit_test.rb +++ b/test/unit/condition_unit_test.rb @@ -9,11 +9,6 @@ class ConditionUnitTest < Minitest::Test @context = Liquid::Context.new end - def test_basic_condition - assert_equal(false, Condition.new(1, '==', 2).evaluate(Context.new)) - assert_equal(true, Condition.new(1, '==', 1).evaluate(Context.new)) - end - def test_default_operators_evalute_true assert_evaluates_true(1, '==', 1) assert_evaluates_true(1, '!=', 2) @@ -72,11 +67,11 @@ class ConditionUnitTest < Minitest::Test end def test_hash_compare_backwards_compatibility - assert_nil(Condition.new({}, '>', 2).evaluate(Context.new)) - assert_nil(Condition.new(2, '>', {}).evaluate(Context.new)) - assert_equal(false, Condition.new({}, '==', 2).evaluate(Context.new)) - assert_equal(true, Condition.new({ 'a' => 1 }, '==', 'a' => 1).evaluate(Context.new)) - assert_equal(true, Condition.new({ 'a' => 2 }, 'contains', 'a').evaluate(Context.new)) + assert_evaluates_nil({}, '>', 2) + assert_evaluates_nil(2, '>', {}) + assert_evaluates_false({}, '==', 2) + assert_evaluates_true({ 'a' => 1 }, '==', 'a' => 1) + assert_evaluates_true({ 'a' => 2 }, 'contains', 'a') end def test_contains_works_on_arrays @@ -110,29 +105,30 @@ class ConditionUnitTest < Minitest::Test end def test_or_condition - condition = Condition.new(1, '==', 2) + false_expr = Parser.new('1 == 2').expression + true_expr = Parser.new('1 == 1').expression + + condition = Condition.new(false_expr) assert_equal(false, condition.evaluate(Context.new)) - condition.or(Condition.new(2, '==', 1)) - + condition.or(Condition.new(false_expr)) assert_equal(false, condition.evaluate(Context.new)) - condition.or(Condition.new(1, '==', 1)) - + condition.or(Condition.new(true_expr)) assert_equal(true, condition.evaluate(Context.new)) end def test_and_condition - condition = Condition.new(1, '==', 1) + false_expr = Parser.new('1 == 2').expression + true_expr = Parser.new('1 == 1').expression + condition = Condition.new(true_expr) assert_equal(true, condition.evaluate(Context.new)) - condition.and(Condition.new(2, '==', 2)) - + condition.and(Condition.new(true_expr)) assert_equal(true, condition.evaluate(Context.new)) - condition.and(Condition.new(2, '==', 1)) - + condition.and(Condition.new(false_expr)) assert_equal(false, condition.evaluate(Context.new)) end @@ -149,7 +145,8 @@ class ConditionUnitTest < Minitest::Test end _out, err = capture_io do - assert_equal(true, Condition.new(1, '==', 1).evaluate) + expr = Parser.new('1 == 1').expression + assert_equal(true, Condition.new(expr).evaluate) end expected = "DEPRECATION WARNING: Condition#evaluate without a context argument is deprecated " \ @@ -345,6 +342,14 @@ class ConditionUnitTest < Minitest::Test private + def assert_evaluates_nil(left, op, right) + expr = BinaryExpression.new(left, op, right) + assert_nil( + Condition.new(expr).evaluate(@context), + "Evaluated not nil: #{left.inspect} #{op} #{right.inspect}", + ) + end + def assert_evaluates_true(left, op, right) expr = BinaryExpression.new(left, op, right) assert(