-
-
Notifications
You must be signed in to change notification settings - Fork 187
Implement binary expressions returning left operand per Liquid standard #836
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Modified all comparison binary expressions to return the left operand instead of boolean values, as specified in the issue. This aligns with the Liquid standard according to the maintainer.
Changes:
- EqualBinaryExpression, NotEqualBinaryExpression
- GreaterThanBinaryExpression, LowerThanBinaryExpression
- AndBinaryExpression, OrBinaryExpression
- ContainsBinaryExpression, StartsWithBinaryExpression, EndsWithBinaryExpression
Added new tests demonstrating the expected behavior:
- {{ 2 == 3 }} returns "2"
- {{ 2 == 3 | plus : 10 | minus : 3 }} returns "9"
Note: This is a breaking change that causes 130+ existing tests to fail. These tests expect binary expressions to return boolean values for use in conditional statements. Further guidance needed on whether:
1. The existing tests should be updated to match this new behavior
2. There's a misunderstanding of the requirement
3. Binary expressions need different behavior in output vs conditional contexts
Co-authored-by: sebastienros <[email protected]>
|
@copilot The |
Created a custom FluidValue (BinaryExpressionFluidValue) that:
- Returns the comparison result when ToBooleanValue() is called (for conditionals)
- Returns the left operand when rendered via WriteToAsync() (for output)
- Returns boolean string ("true"/"false") in ToStringValue() (for filters/assignments)
This elegant solution ensures:
✅ Output statements like {{ 2 == 3 }} display "2" (left operand)
✅ Filters work correctly: {{ 2 == 3 | plus: 10 }} displays "9"
✅ Conditionals work: {% if 2 == 3 %} evaluates to false
✅ All 1221 tests pass (was 130+ failures before)
Updated test ModelShouldNotImpactBlank to match new behavior where binary expressions output the left operand.
Co-authored-by: sebastienros <[email protected]>
Implemented as suggested. Created |
Overview
This PR modifies binary expression evaluation to return the left operand instead of boolean values, as specified in issue #695 and confirmed by @sebastienros to be part of the Liquid standard.
Implementation Approach
Following @sebastienros's guidance, created a custom
BinaryExpressionFluidValueclass that intelligently adapts its behavior based on context:ToBooleanValue()is called (for use in conditionals like{% if %})WriteToAsync()is called (for output rendering in{{ }})ToStringValue()is called (for filters and assignments)This elegant proxy pattern ensures correct behavior across all usage contexts without breaking existing functionality.
Changes
Modified all comparison and logical binary expressions to return
BinaryExpressionFluidValue:==,!=,<>,>,>=,<,<=and,orcontains,startswith,endswithNew file added:
Fluid/Values/BinaryExpressionFluidValue.csBefore/After Behavior
✅ Output Statements (Primary Requirement)
{{ 2 == 3 }}false2✅{{ 2 == 3 | plus: 10 | minus: 3 }}7(false + 10 - 3 = 0 + 10 - 3)9(2 + 10 - 3) ✅{{ 5 == 5 }}true5✅✅ Conditional Statements (Preserved)
{% if 2 == 3 %}true{% else %}false{% endif %}falsefalse✅ (comparison result used for boolean evaluation){% if 5 == 5 %}true{% else %}false{% endif %}truetrue✅✅ Assignments with Filters
{% assign x = false or data | append: 'o' %}{{ x }}falseofalseo✅ (ToStringValue returns boolean string for filter chains)Testing
Test Results: All 1221 tests passing ✅
ModelShouldNotImpactBlank) to align with new behaviorThe implementation successfully balances the new Liquid standard requirement with backward compatibility for conditional statements and existing functionality.
Fixes #695
Original prompt
Fixes #695
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.