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

Add support of String#to_boolean #9667

Closed
wants to merge 1 commit into from

Conversation

cfabianski
Copy link
Contributor

No description provided.

# "0".to_boolean #=> false
# "foo".to_boolean #=> ArgumentError: invalid boolean
def to_boolean
return true if %w(true t 1).include?(self.downcase)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it's a good idea to use a regexp for that purpose as it's time-consuming and it should be processed twice (true and false).
include? better fits this needs IMO

@steveklabnik
Copy link
Member

Can you demonstrate a real-world use case of this making code cleaner? We don't add extensions to String unless it's really important.

@vijaydev
Copy link
Member

👎

@cfabianski
Copy link
Contributor Author

if string.to_boolean instead of if string == "true" || string == "t" || string == "1"
For instance, when you have to check the value of _destroy in accepts_nested_attributes

@josevalim
Copy link
Contributor

That's an issue of accept nested attributes for. Maybe it should yield this
parameter by default?

In any case, -1 for this PR, even more because its implementation is very
couple to what Active Record consider to be Boolean conversions.

José Valim
www.plataformatec.com.br
Skype: jv.ptec
Founder and Lead Developer

@ptn
Copy link
Contributor

ptn commented Mar 11, 2013

It would seem to be overkill to extend a Ruby core class to make the code in this one very particular use case a little terser. How about a helper function in ApplicationController?

@rafaelfranca
Copy link
Member

I agree with @josevalim. Closing.

Thank you for the pull request.

@cfabianski
Copy link
Contributor Author

Thanks for reviewing and your fast feedbacks :-)
I won't touch core_ext anymore :-)

@johnnyshields
Copy link
Contributor

This is a nice utility function to have when importing other people's data. Was expecting ActiveSupport would have something like it.

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.

None yet

7 participants