-
Notifications
You must be signed in to change notification settings - Fork 48
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
fix(non-latin): properly handle non-latin locale scripts when initializing money. #167
fix(non-latin): properly handle non-latin locale scripts when initializing money. #167
Conversation
@@ -2471,8 +2473,8 @@ defmodule Money do | |||
{:ok, symbols} <- Cldr.Number.Symbol.number_symbols_for(locale, backend) do | |||
decimal = | |||
string | |||
|> String.replace(symbols.latn.group, "") | |||
|> String.replace(symbols.latn.decimal, ".") | |||
|> String.replace(symbol_script(symbols, locale).group, "") |
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.
@kipcole9 this is the important part.
test/money_test.exs
Outdated
Money.default_backend().put_locale("ar-EG") | ||
|
||
assert Money.new(:EGP, "100") | ||
|> Money.to_string() == {:ok, "١٠٠٫٠٠ ج.م."} |
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.
I don't think this test is exercising the change you've proposed since formatting doesn't pass through the parsing code. I think "100"
would need to change to "10,000.00"
with the appropriate localised separators.
This opens up the broader issue in that parsing still only accepts latin digits when it should accept digits in other scripts (at least digits that are non-algorithmic).
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.
@kipcole9 I updated the test to include a larger number so see the actual separator.
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.
and yes it only accepts latin digits with no separators.
Thanks very much for raising the issue and the PR. Navigating some of the A couple of questions come to mind where I'd like your thoughts:
|
defp symbol_script(symbols, locale) do | ||
script = | ||
locale | ||
|> Cldr.Locale.script_from_locale() |
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.
That you have to go through this atom/string munging is horrible. I need to update ex_cldr_numbers
to use a canonical form, which will be :Latn
. I will fix that in time for the next ex_cldr
release cycle which will be April 17th alongside CLDR 45's release.
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.
Perfect
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.
Actually I created some confusion between a script and a number system. :Latn
is the script. :latn
is a number system. So here the call should be Cldr.Number.System.number_system_from_locale/1
which will return :arab
for the locale ar-EG
as expected. I'll do that on my end, no action required on your end. And this will get rid of the horrible atom/string/atom munging.
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.
@kipcole9 sorry I must have confused both .. perfect .. thank you for the solution
|
This is very similar to what I already do in iex> Cldr.Number.Parser.parse "١٢٣٤٥", locale: "ar"
{:ok, 12345} And it also handles the separators correctly for a locale. I think the code in Given the need to make changes in both Lastly, for now, thank you for the engagement. I would really value having a collaborator in the arabic-speaking world. I have no working knowledge of arabic, or right-to-left languages. Would you mind if I ask you questions from time to time as I work on implementation? |
One question that immediately comes to mind, if I'm parsing |
@kipcole9 not at all, I would love to answer questions and also help with coding if you are ok with that. I worked with MS arabic localization team for a couple of years back in the 2000s so i have some knowledge their ... just needs a bit of brushing :) For the arabic numbers their direction is still LTR not RTL please check this article https://www.linkedin.com/pulse/why-did-arabs-ditch-arabic-numerals-ayman-el-ghazzawi#:~:text=English%20is%20written%20from%20left,written%20from%20left%20to%20right. |
Thanks Peter, thats very kind and helpful of you. I look forward to collaborating more. If I've understood you correctly, it is common to use latin numerals, at least in Are there cases where the latin separators are used? I'm trying to work out how lenient to be in parsing - as long as there's no ambiguity introduced. |
@kipcole9 sorry maybe i wasn't clear, Its common to use latin numberals with latin separators but if someone uses non-latin ( hindi numerals) they will then use also non-latin separators. |
I'm back working on this @pshoukry. And will have a fix as part of the CLDR 45 release cycle on April 17th. |
Closing as fixed. Thanks of the PR, it's much appreciated. In the end I added some slightly different code for the same purpose. I will be publishing today a new release with this fix. |
Thanks @kipcole9 |
Money.new when attempting maybe_decimal always assumes a latn script locale. ( ie: latn is hardcoded) which fails for any other locale script.