Project

General

Profile

Actions

Defect #38145

closed

Unreachable branch in ApplicationHelper#format_object due to the use of the deprecated Fixnum class

Added by Go MAEDA over 1 year ago. Updated over 1 year ago.

Status:
Closed
Priority:
Normal
Assignee:
Category:
Code cleanup/refactoring
Target version:
Start date:
Due date:
% Done:

0%

Estimated time:
Resolution:
Fixed
Affected version:

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

38145.patch (1.62 KB) 38145.patch Go MAEDA, 2023-01-08 09:37
38145-v2.patch (1.61 KB) 38145-v2.patch Go MAEDA, 2023-01-11 15:43
Actions #1

Updated by Thomas Löber over 1 year 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
Actions #2

Updated by Go MAEDA over 1 year ago

Thomas Löber wrote:

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:

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.

Actions #3

Updated by Thomas Löber over 1 year 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
Actions #4

Updated by Go MAEDA over 1 year ago

Thomas Löber wrote:

However, I don't think the patch works, because the case expression is object.class, but it should be object.

Thank you for pointing it out, I attached the wrong patch.

I have updated the patch.

Actions #5

Updated by Go MAEDA over 1 year ago

  • Target version changed from Candidate for next major release to 5.1.0

Setting the target version to 5.1.0.

Actions #6

Updated by Go MAEDA over 1 year ago

  • Status changed from New to Closed
  • Assignee set to Go MAEDA
  • Resolution set to Fixed

Committed the fix.

Actions

Also available in: Atom PDF