Improve parsing delimiters #200
Conversation
yukideluxe
left a comment
yukideluxe
left a comment
There was a problem hiding this comment.
Thanks for trying to make this complicate code better! I have some thoughts 🙆🏻♀️
| private | ||
|
|
||
| def normalize_number(num) | ||
| extract_major_minor(num.sub(/[\.|,]$/, "")).join(DEFAULT_DECIMAL_MARK) |
There was a problem hiding this comment.
Should you put num.sub(/[\.|,]$/, "") in a variable like for example clean_num = num.sub(/[\.|,]$/, "")? I think it makes this method a bit more readable and understandable 🙏🏻 It wasn't super obvious before either 😊
|
|
||
| def minor_has_correct_dp_for_currency_subunit?(minor) | ||
| minor.length == currency.subunit_to_unit.to_s.length - 1 | ||
| def minor_has_correct_decimal_places_for_currency?(minor) |
There was a problem hiding this comment.
thanks for renaming this method! 😍
| case used_delimiters.length | ||
| when 0 | ||
| [num, DEFAULT_MINOR] | ||
| when 2 |
There was a problem hiding this comment.
non blocking at all but am I the only one bothered that this case/when doesn't have the when in a sane order? 😂 I would not angry if you chose to order it to 0, 1, 2 😳
| num.gsub(separator, "") | ||
| end | ||
|
|
||
| def extract_with_currency_delimiters(num) |
There was a problem hiding this comment.
what do you think about calling this extract_major_minor_with_currency_delimiters to match the naming of the other method and put the methods together? 😊
PS: bear with my pickiness please 🙏🏻
| possible_major, possible_minor = split_major_minor(num, delimiter) | ||
|
|
||
| if minor_has_correct_decimal_places_for_currency?(possible_minor) | ||
| return [possible_major, possible_minor] |
There was a problem hiding this comment.
yay for not doing the split_major_minor twice!
| end | ||
|
|
||
| def extract_major_minor(num) | ||
| if Monetize.enforce_currency_delimiters |
There was a problem hiding this comment.
There's a behavior change here! Before, the Monetize.enforce_currency_delimiters was only checked when the used delimiters were just 1.
In main, if we do Monetize.parse('4,567.89', 'EUR') (parsing euros with usd delimiters) was returning <Money fractional:456789 currency:EUR> which is OK but it technically doesn't "enforce" the currency delimiters like the setting implies to do 😬
In your changes, if we do Monetize.parse('4,567.89', 'EUR'), it returns <Money fractional:457 currency:EUR>. It removes the currency thousands separator (which is . in this case) which converts the number to 4.56789 or, for Spaniards like me, "4,56789".
@RubenIgnacio I am a bit torn about this change. In one hand it does make sense because the setting does say "ENFORCE" and, in the other hand, the parsing before seemed to be "better"? Imagine an American trying to input an euro amount. There doesn't seem to be any explanation about why they added it only for one delimiter apart from fixing a "bug" #57. We don't even know how many people use this 😬
I can be convinced if you have more insight than I do but, if we do this, we should definitely add something in the CHANGELOG about this behavior change. Something like:
**Behavior change**: Changed `Monetize.enforce_currency_delimiters` behavior. Parsing numbers with two delimiters was not enforcing the currency delimiters. Inputs using delimiters that don't match the currency's configuration may be parsed differently than before when `enforce_currency_delimiters` is set to `true`. For example, parsing `Monetize.parse('4,567.89', 'EUR')` will return `<Money fractional:457 currency:EUR>
An alternative is renaming the variable to something more specific so it's clear the heuristic changes for just one delimiter 🙏🏻
Ping @sunny for his thoughts!
PS: note to self, or whoever wants to take this, improve the test suite when enforce_currency_delimiters is true/false 😳
There was a problem hiding this comment.
There doesn't seem to be any explanation about why they added it only for one delimiter
Maybe it's just obvious than when there are two different delimiters, the last one is going to be a "decimal" mark no matter what? It does make sense to me 😇
2 participants
I refactor the delimiter detection logic to improve overall readability and structure. I moved the
enforce_currency_delimiterscheck to the beginning of the process to ensure that when the option is enabled, the currency's delimiters are respected immediately before any automatic detection logic is attempted.