Allow specifying a delimiter pattern #981
Conversation
|
Is it ok like this? I'm not sure I've done it right. Thanks |
CHANGELOG.md
Outdated
| @@ -1,5 +1,9 @@ | |||
| # Changelog | |||
|
|
|||
| ## 6.14.2 | |||
There was a problem hiding this comment.
you will want to bump to 6.15.0 since this is an additive change and not a bug fix.
lib/money/version.rb
Outdated
| @@ -1,3 +1,3 @@ | |||
| class Money | |||
| VERSION = '6.14.1' | |||
| VERSION = '6.14.2' | |||
|
Fixed |
| # 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.
@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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
To add an option to put the thousands separators according to a regex.
Use the ones previously defined in
Formatter#regexp_formatif not specifiedMove the logic to fetch this regex to
FormatterRulesBoth the name and the path for the option are taken from activesupport