liquid icon indicating copy to clipboard operation
liquid copied to clipboard

Support float in variable range

Open peterzhu2118 opened this issue 4 years ago • 2 comments

Ranges with floats get converted to ranges of integers (e.g. 1.1..5.5 gets converted to range 1..5) but this is not the case with ranges of variables (e.g. if x == 1.1 and y == 5.5, x..y results in a Liquid error: invalid integer). This is because floats are not handled in RangeLookup#to_integer so it delegates to Utils.to_integer(input), which first converts the float to a string and then tries to convert it back to an integer.

peterzhu2118 avatar Nov 13 '20 14:11 peterzhu2118

Was the behaviour consistent between liquid and liquid-c?

Ranges with floats get converted to ranges of integers (e.g. 1.1..5.5 gets converted to range 1..5)

I don't think that is a good thing. If the template author actually wanted a float range, then the silent rounding will be a more subtle bug. If they actually wanted 1..5, then they would write that.

If we do want to support a float range in the future, this coercion would make it harder to add that support, since this change would make it more likely for templates to depend on it.

If we want the behaviour to be consistent between static and dynamic ranges, then we should use Liquid::Usage.increment to see if there are dependencies on a float literal being converted to an integer.

dylanahsmith avatar Nov 13 '20 16:11 dylanahsmith

Was the behaviour consistent between liquid and liquid-c?

Yes, see eab12e1240ece8b6722f74b7d81af0b96242c2f3 (this commit is before the Gemfile was updated to point to the branch on liquid-c).

peterzhu2118 avatar Nov 13 '20 16:11 peterzhu2118