Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

The round filter - Ruby 2.7 vs 3.1 #1590

Open
jg-rp opened this issue Jul 4, 2022 · 2 comments
Open

The round filter - Ruby 2.7 vs 3.1 #1590

jg-rp opened this issue Jul 4, 2022 · 2 comments

Comments

@jg-rp
Copy link
Contributor

jg-rp commented Jul 4, 2022

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:

result = result.to_i if n == 0

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

@dylanahsmith
Copy link
Contributor

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
Copy link
Contributor

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

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

No branches or pull requests

2 participants