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

Use value#to_i instead of Kernel#Integer() to coerce page number into integer. #329

Closed
wants to merge 1 commit into from

Conversation

botandrose
Copy link

Integer() is pickier than #to_i. Integer("5/") throws an exception, while "5/".to_i returns 5.

… integer.

Integer() is pickier than #to_i. Integer("5/") throws an exception, while "5/".to_i returns 5.
@mislav
Copy link
Owner

mislav commented Aug 6, 2013

The exception is deliberate. However, future versions of will_paginate might configure Rails to handle the exception better. For now, you should handle it yourself. See #271

@mislav mislav closed this Aug 6, 2013
@botandrose
Copy link
Author

Roger that. Just out of curiosity, what is the value of throwing an exception instead of performing a coercion? Thanks for will_paginate!

@mislav
Copy link
Owner

mislav commented Aug 6, 2013

Exceptions are almost always a better idea than silent coercion between types. The value "37signals" is not the same as the number 37.

@botandrose
Copy link
Author

In general, I agree! Years of PHP and JavaScript masochism has definitely drilled that notion into my skull. :) But personally, I think this is one of those rare times where coercion is the desirable option, and its an explicit coercion at that! We're looking for an integer here, and only an integer, so there's no chance of something getting coerced into the wrong format. Worst-case scenario, someone passes us garbage and we turn it into a 0. From a client code perspective, I'd rather deal with a page number of 0 than a page number of "asdf" + catching an exception. Just my $0.02, though.

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

Successfully merging this pull request may close these issues.

2 participants