Skip to content

Conversation

@peterzhu2118
Copy link
Contributor

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.

@dylanahsmith
Copy link
Contributor

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.

@peterzhu2118
Copy link
Contributor Author

Was the behaviour consistent between liquid and liquid-c?

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants