Fix Numeric filters #571
Fix Numeric filters #571
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #571 +/- ##
===========================================
+ Coverage 91.81% 92.18% +0.37%
===========================================
Files 72 73 +1
Lines 2784 2930 +146
Branches 660 713 +53
===========================================
+ Hits 2556 2701 +145
- Misses 165 167 +2
+ Partials 63 62 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Is this bug exclusive to Abs method? I noted several methods with the same issue in #566 |
It also shows up in at_least and at_most, but they don't return the expected behaviour when fixed! I think I will fix this bug on them, and add it to this PR, and add tests for the current incorrect behaviour. We can then fix that when we re-integrate your golden tests and resolve the issues one at a time. Not sure how you want to approach changing the behaviour when current behaviour doesn't match the Ruby/shopify behaviour. |
I think the behavior there is that null/"abc" as the input is treated the same as 0. Hence Also see my latest push for Golden tests, there is an entire set of tests marked failing for "Non-integer arguments for numeric methods". I think the minimum is Abs/Ceil/Floor as they have the same exact syntax and fix. |
|
Comments in #571 (comment) were not addressed. |
Ah, I missed the abs/ceil/floor comment. I'll sort that too. |
|
Fixed for Ceil and Floor. I've also tweaked Abs to be consistent in implementation (e.g. return decimal, not double). |
|
General behaviour seems to be
Ruby seems to limit Note: The Ruby implementation for a negative value for @microalps, @daviburg : Thoughts? |
Fixing mathematics in DotLiquid was one of my earliest contribution to the code base, and got me in trouble because 'bug fixing' meant breaking consumers of the library who took a dependency on the current output. Hence the later introduction of compatibility versioning in the project. IMHO, this is one such case that needs to go behind a version flag. |
8000
We should not artificially limit ourselves to fewer decimal places than our underlying implementation language supports. Otherwise, Liquid would be driven by the lowest common denominator amongst all implementations. |
… and simplify Round and Abs.
|
Ready for another review. I've addressed most of your comments - simplified some functions, and fixed a few small edge case bugs. I've left the class name on calls to static functions, as per David's request, and I haven't been able to reduce DoMathsOperation further. |
There was a problem hiding this comment.
Pull Request Overview
This pull request enhances the handling of numeric filters by improving consistency in coercion, conversion, and promotion of numeric types in various filters (such as Abs, AtLeast, AtMost, Round, Ceil, Floor, Plus, Minus, DividedBy, Modulo, Times) and by adding a new Sum filter. Key changes include centralized numeric conversion via NumericConverter, updates to the ExpressionUtility type promotion logic, and version attribute updates in filter methods along with modifications to improve error handling and type precision.
Reviewed Changes
Copilot reviewed 21 out of 21 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/DotLiquid/Util/NumericConverter.cs | Implements numeric type checking and conversion with new promotion rules. |
| src/DotLiquid/Util/ExpressionUtility.cs | Refactors binary result type calculation and conversion casting. |
| src/DotLiquid/StandardFilters.cs | Updates filter implementations to use NumericConverter methods and adjusts filter versioning. |
| src/DotLiquid/LegacyFilters.cs | Aligns legacy filter behaviors with new versioning and numeric handling. |
| (Other test files and project files) | Updates tests, project file references and documentation to support the numeric changes. |
Handles numeric parameters consistently across standard filters
Uses the following logic:
Closes #570 and #483.