ActionView: Ensure that NumberHelper is requireable on its own #6414

Closed
wants to merge 1 commit into
from

Projects

None yet

3 participants

@mcmire
mcmire commented May 20, 2012

I would like to start a discussion about ensuring that every helper is requireable on its own. In my project at work, I have tests that sits outside of Rails. One of my classes uses a helper out of ActionView. I shouldn't have to require all of ActionView or ActionPack just to be able to use one method from one helper. I should be able to just require the file I need and keep going.

Unfortunately, this is not the case for NumberHelper -- if you require it and then try to use one of its methods, you get:

~/code/github/forks/rails/actionpack on ruby 1.9.3-p125-falcon (master)
$ ruby -I actionpack/lib -r action_view/helpers/number_helper -e 'h = Object.new.extend(ActionView::Helpers::NumberHelper); h.number_to_currency(2)'
/Users/elliot/code/github/forks/rails/actionpack/lib/action_view/helpers/number_helper.rb:131:in `number_to_currency': undefined method `symbolize_keys' for {}:Hash (NoMethodError)
    from -e:1:in `<main>'

I've quickly patched NumberHelper so that this is possible. However, I imagine that some of the other helpers are this way, too, and if so I'd be happy to fix them and add tests to confirm that this works.

I appreciate how Yehuda Katz tried to keep this in mind when working on Rails 3, but it seems that this mindset has been abandoned since he left, which is a bit of a shame. What's the consensus on this issue?

@rafaelfranca
Member

@mcmire thank you for the pull request.

First, @wycats didn't left the Rails core team, and the mindset that the things should be modulas was not abandoned. We are trying to improve the Action View now.

About the NumberHelpers, #6315 is going to move some helpers to Active Support, so we can decouple Active Support from Action View, as requested in #5675.

I agree with you that some helpers should be requireable on its own, so if you can, please add tests to ensure this behavior to the NumberHelpers. I'll be glad to discuss with the core team to get this pull request merged.

@mcmire
mcmire commented May 21, 2012

Okay that's good to hear. I didn't realize that was going on, so that's awesome. I'll sit back and see what happens with that, then -- no point in me fussing over the requireability of helpers right now if the code's going to get moved.

In the meantime, side note: one thing I'm noticing is that in the case of NumberHelper it's not simply enough to require the right files. Many helpers are also dependent on i18n. Loading i18n isn't as easy as requiring 'activesupport/i18n', because that doesn't seem to have an effect. You still need to initialize i18n in some way -- at least load the default locale file. And it's unclear on how to do this exactly. The i18n railtie that ActiveSupport provides probably does this, but I feel like there should be a simpler way to do it.

@rafaelfranca
Member

Yes, you are right. @carlosantoniodasilva is working in #6290 to decouple they from the i18n.

@mcmire
mcmire commented May 21, 2012

Sweet. Ok well... good work fellas ;P

@carlosantoniodasilva

Side note: not actually to decouple %100, but to have sensible defaults that not rely on I18n.

@mcmire
mcmire commented May 21, 2012

Yup, understood, that's what I was going after.

@carlosantoniodasilva carlosantoniodasilva added a commit that closed this pull request May 29, 2012
@carlosantoniodasilva carlosantoniodasilva Review requires from number helper
Some of these requires are now only necessary in
ActiveSupport::NumberHelper. Add hash/keys require due to symbolize_keys
usage in number helpers. Also remove some whitespaces.

Closes #6414
400c5fe
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment