-
Notifications
You must be signed in to change notification settings - Fork 626
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
Changes from 3 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
|
@@ -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}|$))/ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @ferranpm one note here — I18n does not have a default There was a problem hiding this comment. Choose a reason for hiding this commentThe 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?
And then merging those options with the ones provided to the There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've only encountered one case for a weird There was a problem hiding this comment. Choose a reason for hiding this commentThe 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]) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,3 @@ | ||
class Money | ||
VERSION = '6.14.1' | ||
VERSION = '6.14.2' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 6.15.0 please |
||
end |
There was a problem hiding this comment.
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.