Merge pull request #4667 from carlosantoniodasilva/av-number-helper

Action View number helpers improvements

2 files changed, 55 insertions, 44 deletions

diff --git a/actionpack/lib/action_view/helpers/number_helper.rb b/actionpack/lib/action_view/helpers/number_helper.rb
index fc1cbfcb14..b0860f87c4 100644
--- a/actionpack/lib/action_view/helpers/number_helper.rb
+++ b/actionpack/lib/action_view/helpers/number_helper.rb
@@ -57,15 +57,11 @@ module ActionView
       #  # => +1.123.555.1234 x 1343
       def number_to_phone(number, options = {})
         return unless number
+        options = options.symbolize_keys
 
-        begin
-          Float(number)
-        rescue ArgumentError, TypeError
-          raise InvalidNumberError, number
-        end if options[:raise]
+        parse_float(number, true) if options[:raise]
 
         number       = number.to_s.strip
-        options      = options.symbolize_keys
         area_code    = options[:area_code]
         delimiter    = options[:delimiter] || "-"
         extension    = options[:extension]
@@ -75,7 +71,7 @@ module ActionView
           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.starts_with?(delimiter) && !delimiter.blank?
+          number.slice!(0, 1) if number.start_with?(delimiter) && !delimiter.blank?
         end
 
         str = []
@@ -122,8 +118,7 @@ module ActionView
       #  # => 1234567890,50 £
       def number_to_currency(number, options = {})
         return unless number
-
-        options.symbolize_keys!
+        options = options.symbolize_keys
 
         currency = translations_for('currency', options[:locale])
         currency[:negative_format] ||= "-" + currency[:format] if currency[:format]
@@ -185,12 +180,10 @@ module ActionView
       #  number_to_percentage("98a", :raise => true)                      # => InvalidNumberError
       def number_to_percentage(number, options = {})
         return unless number
+        options = options.symbolize_keys
 
-        options.symbolize_keys!
-
-        defaults = defaults_translations(options[:locale]).merge(translations_for('percentage', options[:locale]))
-
-        options = options.reverse_merge(defaults)
+        defaults = format_translations('percentage', options[:locale])
+        options  = defaults.merge!(options)
 
         format = options[:format] || "%n%"
 
@@ -201,7 +194,8 @@ module ActionView
           if options[:raise]
             raise
           else
-            e.number.to_s.html_safe? ? format.gsub(/%n/, e.number).html_safe : format.gsub(/%n/, e.number)
+            formatted_number = format.gsub(/%n/, e.number)
+            e.number.to_s.html_safe? ? formatted_number.html_safe : formatted_number
           end
         end
       end
@@ -231,13 +225,11 @@ module ActionView
       #
       #  number_with_delimiter("112a", :raise => true)          # => raise InvalidNumberError
       def number_with_delimiter(number, options = {})
-        options.symbolize_keys!
+        options = options.symbolize_keys
 
-        parse_float_number(number, options[:raise]) do
-          return number
-        end
+        parse_float(number, options[:raise]) or return number
 
-        options = options.reverse_merge(defaults_translations(options[:locale]))
+        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]}")
@@ -277,15 +269,13 @@ module ActionView
       #  number_with_precision(1111.2345, :precision => 2, :separator => ',', :delimiter => '.')
       #  # => 1.111,23
       def number_with_precision(number, options = {})
-        options.symbolize_keys!
+        options = options.symbolize_keys
 
-        number = parse_float_number(number, options[:raise]) do
-          return number
-        end
+        number = (parse_float(number, options[:raise]) or return number)
 
-        defaults = defaults_translations(options[:locale]).merge(translations_for('precision', options[:locale]))
+        defaults = format_translations('precision', options[:locale])
+        options  = defaults.merge!(options)
 
-        options = options.reverse_merge(defaults)  # Allow the user to unset default values: Eg.: :significant => false
         precision = options.delete :precision
         significant = options.delete :significant
         strip_insignificant_zeros = options.delete :strip_insignificant_zeros
@@ -346,15 +336,13 @@ module ActionView
       #  number_to_human_size(1234567890123, :precision => 5)        # => "1.1229 TB"
       #  number_to_human_size(524288000, :precision => 5)            # => "500 MB"
       def number_to_human_size(number, options = {})
-        options.symbolize_keys!
+        options = options.symbolize_keys
 
-        number = parse_float_number(number, options[:raise]) do
-          return number
-        end
+        number = (parse_float(number, options[:raise]) or return number)
 
-        defaults = defaults_translations(options[:locale]).merge(translations_for('human', options[:locale]))
+        defaults = format_translations('human', options[:locale])
+        options  = defaults.merge!(options)
 
-        options = options.reverse_merge(defaults)
         #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)
 
@@ -458,15 +446,13 @@ module ActionView
       #  number_to_human(0.34, :units => :distance)                                # => "34 centimeters"
       #
       def number_to_human(number, options = {})
-        options.symbolize_keys!
+        options = options.symbolize_keys
 
-        number = parse_float_number(number, options[:raise]) do
-          return number
-        end
+        number = (parse_float(number, options[:raise]) or return number)
 
-        defaults = defaults_translations(options[:locale]).merge(translations_for('human', options[:locale]))
+        defaults = format_translations('human', options[:locale])
+        options  = defaults.merge!(options)
 
-        options = options.reverse_merge(defaults)
         #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)
 
@@ -504,6 +490,10 @@ module ActionView
 
       private
 
+      def format_translations(namespace, locale)
+        defaults_translations(locale).merge(translations_for(namespace, locale))
+      end
+
       def defaults_translations(locale)
         I18n.translate(:'number.format', :locale => locale, :default => {})
       end
@@ -512,14 +502,10 @@ module ActionView
         I18n.translate(:"number.#{namespace}.format", :locale => locale, :default => {})
       end
 
-      def parse_float_number(number, raise_error)
+      def parse_float(number, raise_error)
         Float(number)
       rescue ArgumentError, TypeError
-        if raise_error
-          raise InvalidNumberError, number
-        else
-          yield
-        end
+        raise InvalidNumberError, number if raise_error
       end
     end
   end
diff --git a/actionpack/test/template/number_helper_test.rb b/actionpack/test/template/number_helper_test.rb
index ca897a48cc..482d953907 100644
--- a/actionpack/test/template/number_helper_test.rb
+++ b/actionpack/test/template/number_helper_test.rb
@@ -268,6 +268,31 @@ class NumberHelperTest < ActionView::TestCase
     assert_nil number_to_human(nil)
   end
 
+  def test_number_helpers_do_not_mutate_options_hash
+    options = { 'raise' => true }
+
+    number_to_phone(1, options)
+    assert_equal({ 'raise' => true }, options)
+
+    number_to_currency(1, options)
+    assert_equal({ 'raise' => true }, options)
+
+    number_to_percentage(1, options)
+    assert_equal({ 'raise' => true }, options)
+
+    number_with_delimiter(1, options)
+    assert_equal({ 'raise' => true }, options)
+
+    number_with_precision(1, options)
+    assert_equal({ 'raise' => true }, options)
+
+    number_to_human_size(1, options)
+    assert_equal({ 'raise' => true }, options)
+
+    number_to_human(1, options)
+    assert_equal({ 'raise' => true }, options)
+  end
+
   def test_number_helpers_should_return_non_numeric_param_unchanged
     assert_equal("+1-x x 123", number_to_phone("x", :country_code => 1, :extension => 123))
     assert_equal("x", number_to_phone("x"))