Defect #38145
closedUnreachable branch in ApplicationHelper#format_object due to the use of the deprecated Fixnum class
0%
Description
There is an unreachable branch in ApplicationHelper#format_object. This is because a deprecated class name `Fixnum` is given to the `when` keyword.
Reference:
Unify Fixnum and Bignum into Integer
https://bugs.ruby-lang.org/issues/12005
While the branch is unreachable, fortunately, the result of the code below `when 'Fixnum'` is the same as the result of the code for the unknown class below `else`, so no problem is occurring for now.
The following patch fixes the issue.
Index: app/helpers/application_helper.rb
===================================================================
--- app/helpers/application_helper.rb (revision 22022)
+++ app/helpers/application_helper.rb (working copy)
@@ -263,7 +263,7 @@
format_time(object)
when 'Date'
format_date(object)
- when 'Fixnum'
+ when 'Integer'
object.to_s
when 'Float'
sprintf "%.2f", object
</diff>
Files
Updated by Thomas Löber almost 2 years ago
I think the problem (or "code smell") is that the case
argument is object.class.name
and not object
. If the code was like this, using Fixnum
would be fine because Fixnum
equals Integer
:
case object when Array formatted_objects = object.map {|o| format_object(o, html)} html ? safe_join(formatted_objects, ', ') : formatted_objects.join(', ') when Time format_time(object) when Date format_date(object) when Fixnum object.to_s when Float sprintf "%.2f", object
Updated by Go MAEDA almost 2 years ago
- File 38145.patch 38145.patch added
Thomas Löber wrote:
I think the problem (or "code smell") is that the
case
argument isobject.class.name
and notobject
. If the code was like this, usingFixnum
would be fine becauseFixnum
equalsInteger
:
Thank you for suggesting the improved code. But we should not use Fixnum
because the constant was removed in Ruby 3.2.
https://github.com/ruby/ruby/blob/ruby_3_2/NEWS.md#removed-constants
Attaching an updated patch.
Updated by Thomas Löber almost 2 years ago
I agree with the use of Integer
instead of Fixnum
.
However, I don't think the patch works, because the case
expression is object.class
, but it should be object
.
The case
statement calls ===
on the expression(s) that come after when
, with the case
expression as a parameter.
>> object = 1 => 1 >> Integer === object # Integer.===(object) => true >> Integer === object.class # Integer.===(object.class) => false
Updated by Go MAEDA almost 2 years ago
- File 38145-v2.patch 38145-v2.patch added
Thomas Löber wrote:
However, I don't think the patch works, because the
case
expression isobject.class
, but it should beobject
.
Thank you for pointing it out, I attached the wrong patch.
I have updated the patch.
Updated by Go MAEDA almost 2 years ago
- Target version changed from Candidate for next major release to 5.1.0
Setting the target version to 5.1.0.
Updated by Go MAEDA almost 2 years ago
- Status changed from New to Closed
- Assignee set to Go MAEDA
- Resolution set to Fixed
Committed the fix.