liquid icon indicating copy to clipboard operation
liquid copied to clipboard

The `round` filter - Ruby 2.7 vs 3.1

Open jg-rp opened this issue 2 years ago • 2 comments

There's a subtle difference in the round filter's behaviour when using Ruby 3.1, vs Ruby 2.7. Specifically, when the input value is a float (or anything that gets converted to a BigDecimal) and round is given an argument that gets coerced to 0, but is not 0, Ruby 2.7 will output a decimal point with a single zero, whereas Ruby 3.1 will not output a decimal point or trailing zero.

Template

{{ 1.0 | round }}
{{ 1.0 | round: 0 }}
{{ 1.0 | round: "0" }}
{{ 1.0 | round: "abc" }}
{{ 1.0 | round: nosuchthing }}

Output using Ruby 2.7.6

1
1
1.0
1.0
1.0

Output using Ruby 3.1.2

1
1
1
1
1

As far as I can tell, this is due to some fixes in Ruby's BigDecimal library.

The cases with the default argument and an explicit 0 are not affected because of the following line: https://github.com/Shopify/liquid/blob/698130573651a7a0accc00a953057a9d424836c5/lib/liquid/standardfilters.rb#L772

I'm unsure how significant of a difference this is, but thought it was at least worth mentioning.

jg-rp avatar Jul 04 '22 11:07 jg-rp

Seems like it is an upstream bug fix which is desirable to inherit in liquid. I'm not aware of it causing problems with Shopify upgrading ruby, so it doesn't seem like a significant enough change to be concerned about it.

Given that, perhaps it should also be safe to make the following change that would given the more expected result, including on ruby 2.7

diff --git a/lib/liquid/standardfilters.rb b/lib/liquid/standardfilters.rb
index ed7136ab..c0d8ec15 100644
--- a/lib/liquid/standardfilters.rb
+++ b/lib/liquid/standardfilters.rb
@@ -771,7 +771,8 @@ module Liquid
     # @liquid_syntax number | round
     # @liquid_return [number]
     def round(input, n = 0)
-      result = Utils.to_number(input).round(Utils.to_number(n))
+      n = Utils.to_number(n)
+      result = Utils.to_number(input).round(n)
       result = result.to_f if result.is_a?(BigDecimal)
       result = result.to_i if n == 0
       result

I think that would better match the intention of that code.

dylanahsmith avatar Aug 24 '22 20:08 dylanahsmith

Actually, ruby 2.7 is only supported for security maintenance upstream. It looks like we could instead just remove that result = result.to_i if n == 0 line once ruby 2.7 no longer needs to be supported.

diff --git a/lib/liquid/standardfilters.rb b/lib/liquid/standardfilters.rb
index ed7136ab..66eaa322 100644
--- a/lib/liquid/standardfilters.rb
+++ b/lib/liquid/standardfilters.rb
@@ -773,7 +773,6 @@ module Liquid
     def round(input, n = 0)
       result = Utils.to_number(input).round(Utils.to_number(n))
       result = result.to_f if result.is_a?(BigDecimal)
-      result = result.to_i if n == 0
       result
     rescue ::FloatDomainError => e
       raise Liquid::FloatDomainError, e.message
diff --git a/test/integration/standard_filter_test.rb b/test/integration/standard_filter_test.rb
index f413e6f9..31fba927 100644
--- a/test/integration/standard_filter_test.rb
+++ b/test/integration/standard_filter_test.rb
@@ -686,6 +686,7 @@ class StandardFiltersTest < Minitest::Test
     assert_template_result("5", "{{ input | round }}", 'input' => 4.6)
     assert_template_result("4", "{{ '4.3' | round }}")
     assert_template_result("4.56", "{{ input | round: 2 }}", 'input' => 4.5612)
+    assert_template_result("4", "{{ '4.3' | round: '0' }}")
     assert_raises(Liquid::FloatDomainError) do
       assert_template_result("4", "{{ 1.0 | divided_by: 0.0 | round }}")
     end

dylanahsmith avatar Aug 24 '22 20:08 dylanahsmith