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

Allow specifying a delimiter pattern #981

Merged
merged 4 commits into from
Mar 11, 2021
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions AUTHORS
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ Edwin Vlieg
Eloy
Evan Alter
Exoth
Ferran Pelayo Monfort
Filipe Goncalves
Francisco Trindade
François Beausoleil
Expand Down
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,9 @@
# Changelog

## 6.14.2
Copy link
Member

Choose a reason for hiding this comment

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

you will want to bump to 6.15.0 since this is an additive change and not a bug fix.


- Add :delimiter_pattern option to the Formatter

## 6.14.1

- Fix CHF format regression introduced in v6.14.0
Expand Down
19 changes: 9 additions & 10 deletions lib/money/money/formatter.rb
Original file line number Diff line number Diff line change
Expand Up @@ -211,6 +211,12 @@ class Formatter
# Money.new(89000, :btc).format(drop_trailing_zeros: true) #=> B⃦0.00089
# Money.new(110, :usd).format(drop_trailing_zeros: true) #=> $1.1
#
# @option rules [Boolean] :delimiter_pattern (/(\d)(?=(?:\d{3})+(?:[^\d]{1}|$))/) Regular expression to set the placement
# for the thousands delimiter
#
# @example
# Money.new(89000, :btc).format(delimiter_pattern: /(\d)(?=\d)/) #=> B⃦8,9,0.00
#
# @option rules [String] :format (nil) Provide a template for formatting. `%u` will be replaced
# with the symbol (if present) and `%n` will be replaced with the number.
#
Expand Down Expand Up @@ -330,7 +336,9 @@ def free_text

def format_whole_part(value)
# Apply thousands_separator
value.gsub regexp_format, "\\1#{thousands_separator}"
value.gsub(rules[:delimiter_pattern]) do |digit_to_delimit|
"#{digit_to_delimit}#{thousands_separator}"
end
end

def extract_whole_and_decimal_parts
Expand Down Expand Up @@ -366,15 +374,6 @@ def lookup(key)
(Money.locale_backend && Money.locale_backend.lookup(key, currency)) || DEFAULTS[key]
end

def regexp_format
if rules[:south_asian_number_formatting]
# from http://blog.revathskumar.com/2014/11/regex-comma-seperated-indian-currency-format.html
/(\d+?)(?=(\d\d)+(\d)(?!\d))(\.\d+)?/
else
/(\d)(?=(?:\d{3})+(?:[^\d]{1}|$))/
end
end

def symbol_value_from(rules)
if rules.has_key?(:symbol)
if rules[:symbol] === true
Expand Down
10 changes: 10 additions & 0 deletions lib/money/money/formatting_rules.rb
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ def initialize(currency, *raw_rules)
@rules = localize_formatting_rules(@rules)
@rules = translate_formatting_rules(@rules) if @rules[:translate]
@rules[:format] ||= determine_format_from_formatting_rules(@rules)
@rules[:delimiter_pattern] ||= delimiter_pattern_rule(@rules)

warn_about_deprecated_rules(@rules)
end
Expand Down Expand Up @@ -90,6 +91,15 @@ def determine_format_from_formatting_rules(rules)
end
end

def delimiter_pattern_rule(rules)
if rules[:south_asian_number_formatting]
# from http://blog.revathskumar.com/2014/11/regex-comma-seperated-indian-currency-format.html
/(\d+?)(?=(\d\d)+(\d)(?!\d))(\.\d+)?/
else
I18n.t('number.currency.format.delimiter_pattern', default: nil) || /(\d)(?=(?:\d{3})+(?:[^\d]{1}|$))/
Copy link
Contributor

Choose a reason for hiding this comment

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

@ferranpm one note here — I18n does not have a default number.currency.format.delimiter_pattern key (have a look at the definitions Rails is using in https://github.com/svenfuchs/rails-i18n) and these lookups are actually not cheap. I think we should to remove it and just fallback to a RegExp straight away

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a shame because it integrates well with the activesupport key but I understand your concern. I've created this PR #985 in order to remove it.

Nonetheless, could we consider adding a global configuration for formatting then?
Maybe a faster way would be to allow configuring this when setting up the gem

Money.formatting = {
  fr: { decimal_mark: ',', delimiter_pattern: ... }
  ...
}

And then merging those options with the ones provided to the #format method?

Copy link
Contributor

Choose a reason for hiding this comment

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

The only time when the I18n lookup is expensive is when you don't have that key. Otherwise it gets cached and loaded from memory.

What you are suggesting does make sense. I wonder how many exception cases would we see there. Are there a lot of currencies you expect delimiter_pattern to be useful for?

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've only encountered one case for a weird delimiter_pattern.
But the idea is not only for this option but for all the formatting options.
It will make it possible to save users to pass custom options every time they need to format a Money object

Copy link
Contributor

Choose a reason for hiding this comment

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

Right I see. So this is more like an override for formatting options. We already do look up I18n's formatting setup, but being able to override it does sound like a useful feature

end
end

def symbol_position_from(rules)
if rules.has_key?(:symbol_position)
if [:before, :after].include?(rules[:symbol_position])
Expand Down
2 changes: 1 addition & 1 deletion lib/money/version.rb
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
class Money
VERSION = '6.14.1'
VERSION = '6.14.2'
Copy link
Member

Choose a reason for hiding this comment

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

6.15.0 please

end
6 changes: 5 additions & 1 deletion spec/money/formatting_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@
I18n.locale = :de
I18n.backend.store_translations(
:de,
number: { currency: { format: { delimiter: ".", separator: "," } } }
number: { currency: { format: { delimiter: ".", separator: ",", delimiter_pattern: /(\d)(?=\d)/ } } }
)
end

Expand All @@ -105,6 +105,10 @@
it "should use ',' as the decimal mark" do
expect(money.decimal_mark).to eq ','
end

it "should use delimiter pattern" do
expect(Money.new(1_456_00, "EUR").format).to eq "€1.4.5.6,00"
end
end

context "with number.currency.symbol.*" do
Expand Down