Rubocop: Cop idea: number conversion

[mockdeep]

It would be great to have a cop that enforces usage of Integer() and Float() over .to_i and .to_f:

# bad
some_input.to_i
some_input.to_f

# good
Integer(some_input)
Float(some_input)

.to_i and .to_f can mask subtle bugs in the inputs.

[bbatsov]

I'm fine with adding such a cop, although there are some situations when it's fine to return nil from a conversion method.

[mikegee]

I disagree with the premise that the conversion functions are the safer version of #to_* methods. They serve different purposes. @avdi's book "Confident Ruby" discusses this topic in depth.

[mockdeep]

@mikegee can you provide some more context? I don't have Avdi's book. I don't know about safer, but it's more explicit that you expect the input to be clearly an integer. There are cases in our codebase where casting invalid input to 0 would cause subtle and hard to trace errors. In other cases, we might not care so much, but I think it's good to be explicit about that by either ignoring the rule inline, or catching the error from Integer and assigning a reasonable default:

begin
  Integer(params[:page_num])
rescue TypeError
  1
end

(I could see wrapping that into a function, similar to Hash#fetch)

[avdi]

They are certainly safer in the sense that they don't hide mistakes the way
the explicit conversion methods can.

They are not safer in the sense that they are always preferable. I use the
explicit conversion methods (#to_i, etc) regularly---but after careful
thought.

For instance, if I am summing date in a table full of messy data scraped
from a web page, and I know that any cells containing empty strings or
non-numeric strings (e.g. repeated header lines), I use cell.to_i to
gracefully ignore the garbage (by converting it to zero).

On Wed, Aug 24, 2016 at 1:54 PM, Robert Fletcher [email protected]
wrote:

@mikegee https://github.com/mikegee can you provide some more context?
I don't have Avdi's book. I don't know about safer, but it's more explicit
that you expect the input to be clearly an integer. There are cases in our
codebase where casting invalid input to 0 would cause subtle and hard to
trace errors. In other cases, we might not care so much, but I think it's
good to be explicit about that by either ignoring the rule inline, or
catching the error from Integer and assigning a reasonable default:

begin
Integer(params[:page_num])rescue TypeError
1end

(I could see wrapping that into a function, similar to Hash#fetch)

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/bbatsov/rubocop/issues/3437#issuecomment-242153018,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAAD1lIxBh6lzwPK6huOY2npEFoS1dWXks5qjIVHgaJpZM4JrPBk
.

Avdi Grimm
http://avdi.org

[mockdeep]

@avdi thanks for the example. I agree it's something to decide which approach is more appropriate based on the situation. I think the one thing that is preferable for me is to demonstrate in some explicit way that we have thought about and decided to use .to_i vs just forgetting to use the more strict Integer. Probably the cleanest way, as far as I'm concerned is to do something like:

thing.to_i # rubocop:disable Lint/NumberConversion

[backus]

@mockdeep if you use mutant then it will always try to mutate to Integer() which would mean that to_i is only used when there is a valid test case for it. Seems better than having disables everywhere

[mockdeep]

@backus yeah, that might make sense, too. I actually wouldn't expect to have too many disables, anyway. If anything, I could see putting a helper function somewhere like the code above to provide a reasonable default.

[mbj]

From my perspective Integer(random_input) is the more primitive method as it "returns something valid or bails" as compared to #to_i that "returns something valid or hides invalid behind something that can also be produced by valid input".

Mutant always mutates to less powerful primitives (the ones with less semantics) and wants "proof" that the one with the more semantics (#to_i with its extra default semantics) is actually used in your code.

Hence I consider this a valid cop.

[albertpaulp]

@mockdeep @bbatsov I've raised a PR which addresses above Cop requirement, it'll be great if you can take a look at it. Do I have to add autocorrect along with this PR ?

[mockdeep]

@albertpaulp I made a few comments, but looks pretty good to me overall. I'll leave it to @bbatsov and others, though, since I'm just a bystander with an opinion.

[mockdeep]

My comments are on this commit instead of the PR because of morning brain.