Consider an array, products, in which product.price for each product 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' }} returns 0.
Expected behaviour:
{{ products | map: 'price' | sum }} and {{ products | sum: 'price' }} return the same value, the correct sum of the prices.
Explanation of current behaviour
TLDR: map calls to_liquid on the values resulting from property lookup, while the current implementation of sum does not.
The current implementation of sum calls Utils#to_number on the value resulting from each property lookup. If the value is a Liquid object, Utils#to_number will return 0 (unless the object responds to :to_number).
The map filter sends :map (which uses :each) to an instance of InputIterator, whose implementation of each 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 the sum filter.
Proposed change
To make this filter consistent with others, such as map, if the value of item[property] responds to to_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:
Construct an array, values_for_sum, which is the result of mapping each item to item[property].
Decorate values_for_sum as an InputIterator, since InputIterator#each sends to_liquid to any element before yielding the element.
Return the sum of Utils.to_number(item) for each item of InputIterator.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.
Consider an array,
products, in whichproduct.pricefor eachproductreturns 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
sumcallsUtils#to_numberon the value resulting from each property lookup. If the value is a Liquid object,Utils#to_numberwill return0(unless the object responds to:to_number).The
mapfilter sends:map(which uses:each) to an instance ofInputIterator, whose implementation ofeachsends:to_liquidto each element before yielding.So, for Liquid objects who respond to
:to_liquidwith a numeric, the elements of{{ products | map: 'price' }}will be the result of:to_liquidfor each element, and the sum will return the expected value. This same:to_liquidparsing does not currently happen in thesumfilter.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 eachitemtoitem[property].values_for_sumas anInputIterator, sinceInputIterator#eachsendsto_liquidto any element before yielding the element.Utils.to_number(item)for eachitemofInputIterator.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_liquidbehaviour.