Ensure sum
filter calls to_liquid
on evaluated property value
#1726
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Consider an array,
products
, in whichproduct.price
for eachproduct
returns a Liquid object that represents the product's price.Current behaviour:
{{ products | map: 'price' | sum }}
returns the correct sum of the prices.{{ products | sum: 'price' }}
returns0
.Expected behaviour:
{{ products | map: 'price' | sum }}
and{{ products | sum: 'price' }}
return the same value, the correct sum of the prices.Explanation of current behaviour
The current implementation of
sum
callsUtils#to_number
on the value resulting from each property lookup. If the value is a Liquid object,Utils#to_number
will return0
(unless the object responds to:to_number
).The
map
filter sends:map
(which uses:each
) to an instance ofInputIterator
, whose implementation ofeach
sends:to_liquid
to each element before yielding.So, for Liquid objects who respond to
:to_liquid
with a numeric, the elements of{{ products | map: 'price' }}
will be the result of:to_liquid
for each element, and the sum will return the expected value. This same:to_liquid
parsing does not currently happen in thesum
filter.Proposed change
To make this filter consistent with others, such as
map
, if the value ofitem[property]
responds toto_liquid
, it should be parsed to liquid before being coerced into a number and summed.Proposed logic for the with-property branch of
StandardFilters#sum
:values_for_sum
, which is the result of mapping eachitem
toitem[property]
.values_for_sum
as anInputIterator
, sinceInputIterator#each
sendsto_liquid
to any element before yielding the element.Utils.to_number(item)
for eachitem
ofInputIterator.new(values_for_sum, ...)
.With this change, I have also refactored to make the method more concise and included additional tests for the expected
to_liquid
behaviour.