Moving number helper from ActionView to Active Support #6315

Merged
merged 1 commit into from May 28, 2012

Conversation

Projects
None yet
9 participants
Contributor

amutz commented May 15, 2012

Hi,

In an earlier pull request I hoped we could decouple ActiveSupport from ActionView:

#5675

It was suggested by @josevalim that instead of that approach, we move the number helpers from ActionView to ActiveSupport. In addition to his suggestion, this approach was a feature request in a separate issue:

#3214

In this pull request I've moved the functionality from NumberHelper in ActionView to ActiveSupport and added the methods to Numeric. So instead of including NumberHelper and writing "number_to_human(123)", you could just include ActiveSupport and write "123.to_human". The original NumberHelper methods are all left in place and accept the same options and behave the same (and just pass through to the AS implementation in Numeric).

Other interested parties may include: @carlosantoniodasilva, @fxn, and @yfeldblum.

Let me know if you'd like any changes to this implementation.

Thanks,
Andrew.

Owner

rafaelfranca commented May 15, 2012

hey @amutz, this pull request cannot be automatically merged. Please rebase it against the master

Contributor

amutz commented May 15, 2012

Hi @rafaelfranca, I've rebased this against master. Did that fix the problem?

Thanks,
Andrew

Member

steveklabnik commented May 15, 2012

Yep, it's totally merge-able now.

@josevalim josevalim commented on an outdated diff May 15, 2012

...ort/lib/active_support/core_ext/numeric/formatting.rb
+ delimiter = options[:delimiter] || "-"
+ extension = options[:extension]
+ country_code = options[:country_code]
+
+ if area_code
+ number.gsub!(/(\d{1,3})(\d{3})(\d{4}$)/,"(\\1) \\2#{delimiter}\\3")
+ else
+ number.gsub!(/(\d{0,3})(\d{3})(\d{4})$/,"\\1#{delimiter}\\2#{delimiter}\\3")
+ number.slice!(0, 1) if number.start_with?(delimiter) && !delimiter.blank?
+ end
+
+ str = ''
+ str << "+#{country_code}#{delimiter}" unless country_code.blank?
+ str << number
+ str << " x #{extension}" unless extension.blank?
+ ERB::Util.html_escape(str)
@josevalim

josevalim May 15, 2012

Member

This is not a responsibility of Numeric, should probably be in the Action View only.

@josevalim josevalim commented on an outdated diff May 15, 2012

...ort/lib/active_support/core_ext/numeric/formatting.rb
+ currency[:negative_format] ||= "-" + currency[:format] if currency[:format]
+
+ defaults = DEFAULT_CURRENCY_VALUES.merge(defaults_translations(options[:locale])).merge!(currency)
+ defaults[:negative_format] = "-" + options[:format] if options[:format]
+ options = defaults.merge!(options)
+
+ unit = options.delete(:unit)
+ format = options.delete(:format)
+
+ if number.to_f < 0
+ format = options.delete(:negative_format)
+ number = number.respond_to?("abs") ? number.abs : number.sub(/^-/, '')
+ end
+
+ formatted_number = format.gsub('%n', self.with_precision(number, options)).gsub('%u', unit)
+ if (valid_float?(number) || number.html_safe?)

@josevalim josevalim commented on an outdated diff May 15, 2012

...ort/lib/active_support/core_ext/numeric/formatting.rb
+ # 100.to_percentage(:format => "%n %") # => 100 %
+ def to_percentage(options = {})
+ self.class.to_percentage(self, options)
+ end
+
+ def self.to_percentage(number, options = {})
+ return unless number
+ options = options.symbolize_keys
+
+ defaults = format_translations('percentage', options[:locale])
+ options = defaults.merge!(options)
+
+ format = options[:format] || "%n%"
+
+ formatted_number = format.gsub('%n', self.with_precision(number, options))
+ if (valid_float?(number) || number.html_safe?)

@josevalim josevalim commented on an outdated diff May 15, 2012

...ort/lib/active_support/core_ext/numeric/formatting.rb
+ # 98765432.98.with_delimiter(:delimiter => " ", :separator => ",")
+ # # => 98 765 432,98
+ def with_delimiter(options = {})
+ self.class.with_delimiter(self, options)
+ end
+
+ def self.with_delimiter(number, options = {})
+ options = options.symbolize_keys
+
+ return number unless valid_float?(number)
+
+ options = defaults_translations(options[:locale]).merge(options)
+
+ parts = number.to_s.to_str.split('.')
+ parts[0].gsub!(/(\d)(?=(\d\d\d)+(?!\d))/, "\\1#{options[:delimiter]}")
+ parts.join(options[:separator]).html_safe
@josevalim

josevalim May 15, 2012

Member

Ditto with html_safe

@josevalim josevalim commented on an outdated diff May 15, 2012

...ort/lib/active_support/core_ext/numeric/formatting.rb
@@ -0,0 +1,553 @@
+require 'active_support/core_ext/big_decimal/conversions'
+require 'active_support/core_ext/object/blank'
+require 'active_support/core_ext/string/output_safety'
@josevalim

josevalim May 15, 2012

Member

The output_safety and erb requires should probably be out. We need to bring some hash requires in (at least extract_options! one, but i think we also need others).

@josevalim josevalim commented on an outdated diff May 15, 2012

...ort/lib/active_support/core_ext/numeric/formatting.rb
+ options = options.symbolize_keys
+
+ return number unless valid_float?(number)
+ number = Float(number)
+
+ defaults = format_translations('human', options[:locale])
+ options = defaults.merge!(options)
+
+ #for backwards compatibility with those that didn't add strip_insignificant_zeros to their locale files
+ options[:strip_insignificant_zeros] = true if not options.key?(:strip_insignificant_zeros)
+
+ storage_units_format = I18n.translate(:'number.human.storage_units.format', :locale => options[:locale], :raise => true)
+
+ base = options[:prefix] == :si ? 1000 : 1024
+
+ if number.to_i < base
@josevalim

josevalim May 15, 2012

Member

Ditto html_safe in both branches.

@josevalim josevalim commented on an outdated diff May 15, 2012

...ort/lib/active_support/core_ext/numeric/formatting.rb
+ number_exponent = number != 0 ? Math.log10(number.abs).floor : 0
+ display_exponent = unit_exponents.find{ |e| number_exponent >= e } || 0
+ number /= 10 ** display_exponent
+
+ unit = case units
+ when Hash
+ units[DECIMAL_UNITS[display_exponent]]
+ when String, Symbol
+ I18n.translate(:"#{units}.#{DECIMAL_UNITS[display_exponent]}", :locale => options[:locale], :count => number.to_i)
+ else
+ I18n.translate(:"number.human.decimal_units.units.#{DECIMAL_UNITS[display_exponent]}", :locale => options[:locale], :count => number.to_i)
+ end
+
+ decimal_format = options[:format] || I18n.translate(:'number.human.decimal_units.format', :locale => options[:locale], :default => "%n %u")
+ formatted_number = self.with_precision(number, options)
+ decimal_format.gsub(/%n/, formatted_number).gsub(/%u/, unit).strip.html_safe
Member

josevalim commented May 15, 2012

Great job! I really like this. I have added some comments regarding the code, and besides that, two small things:

  1. I didn't like much the names of with_precision and with_delimiter methods. I would choose something like to_s(:delimiter) or to_s_with_delimiter. Feedback here is welcome /cc @fxn

  2. I don't like that we have both class and instance methods. I am aware why they exist (to support both string and integer values) but there is a way to get rid of it, I would love to

@josevalim josevalim commented on an outdated diff May 15, 2012

...ort/lib/active_support/core_ext/numeric/formatting.rb
+ units[DECIMAL_UNITS[display_exponent]]
+ when String, Symbol
+ I18n.translate(:"#{units}.#{DECIMAL_UNITS[display_exponent]}", :locale => options[:locale], :count => number.to_i)
+ else
+ I18n.translate(:"number.human.decimal_units.units.#{DECIMAL_UNITS[display_exponent]}", :locale => options[:locale], :count => number.to_i)
+ end
+
+ decimal_format = options[:format] || I18n.translate(:'number.human.decimal_units.format', :locale => options[:locale], :default => "%n %u")
+ formatted_number = self.with_precision(number, options)
+ decimal_format.gsub(/%n/, formatted_number).gsub(/%u/, unit).strip.html_safe
+ end
+
+
+ private
+
+ def self.format_translations(namespace, locale)
@josevalim

josevalim May 15, 2012

Member

Notice that since those are class methods, they are not really being private here. You need to wrap them inside a class << self block.

class Foo
private
  def self.gotcha!
    1
  end
end

Foo.gotcha! #=> 1
Contributor

alexeymuranov commented May 15, 2012

👍 It was weird to have include ActionView::Helpers::NumberHelper in my models.

+1 to the pull request.

+1 to removing every aspect of SafeBuffer and #html_safe from these methods.

@carlosantoniodasilva carlosantoniodasilva commented on an outdated diff May 15, 2012

activesupport/test/core_ext/numeric_ext_test.rb
+ assert_equal '10 MB', 9961472.to_human_size(:precision=>0)
+ assert_equal '40 KB', 41010.to_human_size(:precision => 1)
+ assert_equal '40 KB', 41100.to_human_size(:precision => 2)
+ assert_equal '1.0 KB', kilobytes(1.0123).to_human_size(:precision => 2, :strip_insignificant_zeros => false)
+ assert_equal '1.012 KB', kilobytes(1.0123).to_human_size(:precision => 3, :significant => false)
+ assert_equal '1 KB', kilobytes(1.0123).to_human_size(:precision => 0, :significant => true) #ignores significant it precision is 0
+ end
+
+ def test_number_to_human_size_with_custom_delimiter_and_separator
+ assert_equal '1,01 KB', kilobytes(1.0123).to_human_size(:precision => 3, :separator => ',')
+ assert_equal '1,01 KB', kilobytes(1.0100).to_human_size(:precision => 4, :separator => ',')
+ assert_equal '1.000,1 TB', terabytes(1000.1).to_human_size(:precision => 5, :delimiter => '.', :separator => ',')
+ end
+
+
+
@carlosantoniodasilva

carlosantoniodasilva May 15, 2012

Owner

Whitespaces ✂️

Contributor

gazay commented May 15, 2012

👍 great

Owner

fxn commented May 15, 2012

👍 to all the remarks of @josevalim, would favor to_s in particular for 1).

To be complete, this pull request should also update the AS guide.

I need to double-check if the presence of a locale in this code is fine, not sure we can count on a locale for core extensions since people should be able to cherry-pick just one file (see the top of the AS guide), and that should work outside Ruby on Rails.

@fxn just as a side note, I have an open pull request to add defaults to all these number helpers, but I was using I18n as defaults. After talking to @josevalim we decided that it's better to have these defaults as constants, so I have to update the pull request, but now I think I should wait for this change.

Contributor

amutz commented May 15, 2012

Ok, great. I'll make the changes mentioned by @josevalim to keep certain concerns in NumberHelper, I'll rename the instance methods to to_s, and I'll fix the method protection of the class method. And I will update the AS guide, per the request by @fxn.

Regarding a way to avoid having class versions of each, I'm open to suggestions. In order to support things like "number_to_currency('123a456') => $123a456" (which there are explicit tests for), we need to be able to have methods that operate on strings (as @josevalim correctly pointed out). I considered having only instance methods and having an option that could be passed to include a string (like 0.to_currency(:number => "123a456") => $123a456), but that seemed uglier than class methods. Any suggestions here are welcome.

I'll update the pull request with the changes in a day or two. Thanks for all the feedback!

The ActionView methods may need to retain their interface, but the new ActiveSupport methods do not.

The ActiveSupport methods, especially the core-ext methods, can change the interface. For example, the to_currency in ActiveSupport can operate only on numbers. The ActionView methods are free to take strings, convert them to numbers, and then call the ActiveSupport methods.

Owner

fxn commented May 15, 2012

Agreed with @yfeldblum, in Action View you need to be string-friendly, but in core extensions you implement the natural interface only.

Member

josevalim commented May 15, 2012

@fxn @yfeldblum the problem is that you may lose information in the process, like converting this phone number "043 321 932" to an integer and then back to a phone number.

@josevalim #to_phone should take a string, not a numeric.

module Numeric
  delegate :to_phone, :to => :to_s
end

class String
  def to_phone
    ...
  end
end
Contributor

amutz commented May 15, 2012

Thanks for the ideas. @yfeldblum that solution works, but it feels unnatural to me to put the implementation of what are NumberHelper methods injected to all strings. Yes, we want to support translating a string that is numberish into a phone number, but for most strings these methods wont be relevant. It would surprise me if I looked at the methods for "banana" and found a "to_phone" method.

They don't have to be methods on String or Number necessarily. But they should be in ActiveSupport not ActionView. The phone-related methods should take strings, and convert numbers to strings, while other methods should take numbers, and convert strings to numbers.

Contributor

alexeymuranov commented May 16, 2012

How about a class Locale with instance methods number_to_currency, number_to_phone, etc., and a module with a method current_locale, to include it into ActiveRecord and ActiveView (or a class method Locale::current)?

P.S. Maybe even better: Locale#money(number) or Locale#money_from_number(number).

P.P.S. BTW, as functions in Ruby are written right-to-left, i prefer #currency_from_number over #number_to_currency.

Update 2012-05-18. Here is what i am thinking about:

french = Locale.new(:fr)
ten_euros_in_french = french.currency_from_number(10)
Contributor

amutz commented May 17, 2012

Thanks for all the feedback! I've made the following improvements:

I've moved the output safety concerns back to NumberHelper, fixed the class method protection and updated the ActiveSupport guide.

The methods now are versions of to_s:

1235551234.to_s(:phone)                              # => 123-555-1234
1234567890.50.to_s(:currency)                        # => $1,234,567,890.50
100.to_s(:percentage)                                # => 100.000%
12345678.to_s(:delimited)                            # => 12,345,678
111.2345.to_s(:rounded)                              # => 111.235
123.to_s(:human_size)                                # => 123 Bytes
1234567890123456.to_s(:human)                        # => "1.23 Quadrillion"

The class versions are still around, because I could not find a good way to get rid of them. I have changed the names of the "with_" methods, to have more consistency:

Numeric.to_phone(5551234)                            # => 555-1234
Numeric.to_currency(1234567890.50)                   # => $1,234,567,890.50
Numeric.to_percentage(100)                           # => 100.000%
Numeric.to_delimited(12345678)                       # => 12,345,678
Numeric.to_rounded(111.2345)                         # => 111.235
Numeric.to_human_size(123)                           # => 123 Bytes
Numeric.to_human(123)                                # => "123"

Regarding the I18n concerns, I've added the AS en.yml file to the I18n load_path when it's not already there. As a result, you can load just formatting.rb alone and it should function properly.

Thanks again for all the feedback! Let me know if there's any more changes desired and I can make them.

@josevalim josevalim commented on an outdated diff May 17, 2012

...ort/lib/active_support/core_ext/numeric/formatting.rb
+ I18n.translate(:"#{units}.#{DECIMAL_UNITS[display_exponent]}", :locale => options[:locale], :count => number.to_i)
+ else
+ I18n.translate(:"number.human.decimal_units.units.#{DECIMAL_UNITS[display_exponent]}", :locale => options[:locale], :count => number.to_i)
+ end
+
+ decimal_format = options[:format] || I18n.translate(:'number.human.decimal_units.format', :locale => options[:locale], :default => "%n %u")
+ formatted_number = self.to_rounded(number, options)
+ decimal_format.gsub(/%n/, formatted_number).gsub(/%u/, unit).strip
+ end
+
+
+ [Float, Fixnum, Bignum, BigDecimal].each do |klass|
+ klass.send(:alias_method, :to_s_without_formatting, :to_s)
+
+ klass.send(:define_method, :to_s) do |*args|
+ if args.size > 0 && args[0].is_a?(Symbol)
@josevalim

josevalim May 17, 2012

Member

Don't need to check for the size. args[0].is_a? Symbol is enough.

Member

josevalim commented May 17, 2012

I don't think the I18n path munging is required at all. It is a small change and since it is Rails 4, it is fine to change the contents of .yml files. We just need to mention it in the CHANGELOG.

Member

josevalim commented May 17, 2012

@amutz the naming is much better now, thanks!

@fxn what is the usual practice in Active Support, are we overriding to_s or creating a to_formatted_s method?

Contributor

amutz commented May 17, 2012

Ok great thanks. I've removed the unnecessary check of the args.size.

Regarding removing the i18n load_path file stuff, just to be clear, this will mean that if you cherry pick this single file from ActiveSupport and load it alone, these methods won't work properly. According to @fxn, people should be able to load a single file in AS and have it work.

Let me know what you'd like me to do with the i18n stuff, and if the methods should be named to_formatted_s instead.

Thanks again for all the feedback!

Member

josevalim commented May 17, 2012

Regarding removing the i18n load_path file stuff, just to be clear, this will mean that if you cherry pick this single file from ActiveSupport and load it alone, these methods won't work properly. According to @fxn, people should be able to load a single file in AS and have it work.

Then can't you simply require "active_support/i18n" at the top instead?

Contributor

amutz commented May 17, 2012

Ok, I misunderstood what you were saying. You are correct. I've updated the code.

Thanks!

Based on @josevalim's comments, I would still suggest moving the implementation of to_phone from Numeric to String. Numeric.to_phone could basically do String.to_phone(input.to_s).

I'm also not necessarily sure I would stick the formatting methods on Numeric or String directly. Perhaps on ActiveSupport::Format::Numeric and ActiveSupport::Format::String.

Contributor

amutz commented May 22, 2012

@yfeldblum, in my opinion, methods that format numbers and numberish strings make more sense on Numeric than on String. Having them sit in a separate module works, but feels about as clean as having them be class methods, to me. If the community as a whole feels that I should move these class methods to a separate, ActiveSupport-specific module, I can do that.

Is there anything more I can do to move this forward? Are we waiting on @fxn 's input on whether the methods should be to_s or to_formatted_s?

Let me know if there's any way I can help. Thanks.

Contributor

alexeymuranov commented May 22, 2012

I am for keeping formatting methods in a separate module

Owner

fxn commented May 22, 2012

Yes, to_formatted_s is more common in Active Support.

The blank lines between RDoc and method definitions should be removed, and we would need a CHANGELOG entry in Active Support.

@josevalim other than that the patch is OK for you?

Member

josevalim commented May 23, 2012

@fxn should we still monkey patch to_s if we are defining to_formatted_s?

Also, since we are monkey patching to_formatted_s in Numeric anyway, what about defining the "class" methods inside a module (ActiveSupport::Numeric or ActiveSupport::NumberHelpers)? With the module we have better options because:

  1. We will still be able to call it as AS::Numeric.to_currency, etc
  2. But also include it in a class if we would like to

Nonetheless @amutz, thanks for your work. I am aware that you have already changed things many times, but those changes are big and not so trivial to take.

Contributor

amutz commented May 24, 2012

Absolutely, I don't mind changing things, it's my pleasure to help. I'll move the methods from being injected on the Numeric class, to a module. I may open a separate pull request because there have been a lot of changes and rebasing started to get weird on me and I'm not exactly a git ninja. I will make the changes in a few days.

Regarding monkeypatching to_s, in all the to_formatted_s examples I've seen, to_s is monkeypatched:

activesupport/lib/active_support/core_ext/array/conversions.rb
activesupport/lib/active_support/core_ext/bigdecimal/conversions.rb
activesupport/lib/active_support/core_ext/date/conversions.rb
activesupport/lib/active_support/core_ext/date_time/conversions.rb
activesupport/lib/active_support/core_ext/range/conversions.rb
activesupport/lib/active_support/core_ext/time/conversions.rb

(as a side note, perhaps conversions.rb is a more appropriate for the file than formatting.rb, given that to_formatted_s lives in conversions.rb for other core_ext code)

Member

josevalim commented May 24, 2012

OK. So let's move the class methods to a module that extends itself and feel free to rename the file to conversions. Please rebase everything and let's merge it! :D

josevalim was assigned May 26, 2012

Contributor

amutz commented May 28, 2012

Ok, I've made the desired changes and squashed and rebased them into a single commit.

Per @josevalim 's request, the methods now reside in ActiveSupport::NumberHelper and can be accessed via Numeric.to_s and Numeric.to_formatted_s. The core_ext file that monkeypatches to_s is now named conversions.rb, in order to be consistent with other parts of the AS codebase.

Let me know if you'd like any other changes. Thanks for all the feedback!

@josevalim josevalim added a commit that referenced this pull request May 28, 2012

@josevalim josevalim Merge pull request #6315 from appfolio/moving_number_helper_methods_t…
…o_active_support

Moving number helper from ActionView to Active Support
135f620

@josevalim josevalim merged commit 135f620 into rails:master May 28, 2012

Member

josevalim commented May 28, 2012

Beautiful, thanks!

@claudiofullscreen claudiofullscreen pushed a commit to claudiofullscreen/squid that referenced this pull request Aug 5, 2015

@claudiob claudiob Require ActiveSupport >= 4.0
That’s because `number_to_rounded` was only added in rails/rails#6315
6949b71

@claudiofullscreen claudiofullscreen pushed a commit to claudiofullscreen/squid that referenced this pull request Aug 5, 2015

@claudiob claudiob Require ActiveSupport >= 4.0
That’s because `number_to_rounded` was only added in rails/rails#6315
cf7a32c
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment