numbers: implement currency formatting with long display names. #585
Conversation
Codecov Report
@@ Coverage Diff @@
## master #585 +/- ##
=========================================
+ Coverage 90.22% 90.3% +0.07%
=========================================
Files 24 24
Lines 4082 4105 +23
=========================================
+ Hits 3683 3707 +24
+ Misses 399 398 -1
Continue to review full report at Codecov.
|
|
Some notes: Step 1 of the algorithm involves checking for an exact match against number 0 or 1. However, none of the locale data actually has data defined for this. Since I would have been producing untestable code, I thought it best not to implement this. It could be added as a refinement when the need comes up. Step 5 refers to applying It would appear that there is just one affected locale which actually defines this currencyDecimal value - babel doesn't use it though: Contrast this with Whether you want to address this odd corner case is a matter for another issue I think. |
|
Based on a cursory review, this looks great! Clean and simple :) Mind making an issue out of that |
scripts/import_cldr.py
Outdated
| @@ -903,6 +904,14 @@ def parse_currency_formats(data, tree): | |||
| currency_formats[type] = numbers.parse_pattern(pattern) | |||
|
|
|||
|
|
|||
| def parse_currency_unit_patterns(data, tree): | |||
| currency_unit_patterns = data.setdefault('currency_unit_patterns', {}) | |||
| for unit_pattern_elem in tree.findall('.//currencyFormats/unitPattern'): | |||
There was a problem hiding this comment.
This should ignore non-latn currency formats at present, just in case there are both latn and non-latn currencyFormats elements within a single locale.
You can (rebase and) use the _should_skip_number_elem() function introduced in #579 to do that.
There was a problem hiding this comment.
Fixed in 4b5097f
babel/numbers.py
Outdated
|
|
||
| # Step 3. | ||
| try: | ||
| unit_pattern = locale._data['currency_unit_patterns'][plural_category] |
There was a problem hiding this comment.
I mildly like pulling things out of locale._data from outside the locale object, but we haven't been that strict about it before either.
There was a problem hiding this comment.
I had a partial go at fixing this in 4c7e9b6 - it is at least more consistent now, and the resulting code is much flatter as well.
In detail: 1. Use the already existing get_currency_name function which does the plural form logic already. 2. Create a similar `get_currency_unit_pattern` function. This function could be useful if someone wanted to write something very similar to the _format_currency_long_name functionality but with some different customizations, so it is now publicly documented. 3. Simplify the _format_currency_long_name function - it is now much flatter. 4. Add more tests to ensure certain cases are really covered. (e.g. different unit patterns depending on the count) An upshot of the changes is that we have reduced (and made more consistent) the number of places where we need to peek into `Locale._data` - get_currency_name and get_currency_unit_pattern are the only places that babel.numbers does this.
|
Copying my message from the 2nd commit in case it gets a bit lost: 4c7e9b6 Simplify format_currency code by pulling out/using helpers. In detail:
An upshot of the changes is that we have reduced (and made more consistent) the number of places where we need to peek into In fact I already have a use case for having |
|
Thank you, @spookylukey! 😻 |
|
Thanks for merging, and maintaining babel - it is one of the those projects that is often a thankless task when it comes to maintaining it, rarely making any headlines, but a dependency of a huge number of projects! |
|
@akx - do you have plans to do a release soon? I'm not pressuring you to do so, it would just be nice to know - there is another library I'm preparing a patch for that could be improved if I could use the feature you merged in this PR (and be able to refer to the specific babel version it was added in for dependencies). Thanks! |
Fixes #578