843758 - Use script to generate currency digits information in Intl.js

Status: RESOLVED FIXED

(Core :: JavaScript: Internationalization API, defect)

Description

[Norbert Lindenberg]

Intl.js contains a variable currencyDigits, which has information about all currencies that use fewer or more than 2 decimal digits. This information is derived from a table maintained by the ISO 4217 maintenance authority:
http://www.currency-iso.org/en/home/tables/table-a1.html
Since the table changes occasionally, the variable should be derived from the table by a Python script similar to make_intl_data.py (or part of it).

Comment 1

[Gordon P. Hemsley [:GPHemsley]]

Mass-moving existing Intl-related bugs to the new Core :: JavaScript: Internationalization API component.

If you think this bug has been moved in error, feel free to move it back to Core :: JavaScript Engine.

[Mass change filter: core-js-intl-api-move]

Comment 2

[Karl Dubost💡 :karlcow]

Attachment: currencydigits.py

Note there are only a couple of types of Currency Units

egrep 'CcyMnrUnts' table_a1.xml  | sort | uniq -c 

  32 			<CcyMnrUnts>0</CcyMnrUnts>
 222 			<CcyMnrUnts>2</CcyMnrUnts>
   7 			<CcyMnrUnts>3</CcyMnrUnts>
   1 			<CcyMnrUnts>4</CcyMnrUnts>
  13 			<CcyMnrUnts>N.A.</CcyMnrUnts>

The current table is 

var currencyDigits = {
     BHD: 3,
     BIF: 0,
     BYR: 0,
     …
 };

I created a script which gives 

→ python currencydigits.py 
Last Updated: 2015-01-01
Currencies List: 
{'DZD': '2', 'NAD': '2', 'KMF': '0', 'EGP': '2', 'BOV': '2', 'PAB': '2', 'TMT': '2', 'BOB': '2', 'XBA': 'null', 'DKK': '2', 'XBC': 'null', 'XBB': 'null', 'BWP': '2', 'LBP': '2', 'TZS': '2', 'VND': '0', 'AOA': '2', 'KHR': '2', 'GHS': '2', 'KYD': '2', 'LYD': '3', 'UAH': '2', 'JOD': '3', 'AWG': '2', 'SAR': '2', 'XPT': 'null', 'HKD': '2', 'CHE': '2', 'CHF': '2', 'GIP': '2', 'BYR': '0', 'XPF': '0', 'XPD': 'null', 'MRO': '2', 'BGN': '2', 'HRK': '2', 'DJF': '0', 'SZL': '2', 'THB': '2', 'XAF': '0', 'BND': '2', 'ISK': '0', 'UYU': '2', 'NIO': '2', 'LAK': '2', 'NPR': '2', 'SYP': '2', 'MAD': '2', 'UYI': '0', 'MZN': '2', 'PHP': '2', 'ZAR': '2', 'XUA': 'null', 'ALL': '2', 'ZWL': '2', 'XSU': 'null', 'NGN': '2', 'CRC': '2', 'AED': '2', 'GBP': '2', 'XBD': 'null', 'MWK': '2', 'LKR': '2', 'PKR': '2', 'HUF': '2', 'BMD': '2', 'LSL': '2', 'MNT': '2', 'AMD': '2', 'UGX': '0', 'QAR': '2', 'XDR': 'null', 'JMD': '2', 'GEL': '2', 'SHP': '2', 'AFN': '2', 'SBD': '2', 'KPW': '2', 'MKD': '2', 'TRY': '2', 'BDT': '2', 'YER': '2', 'HTG': '2', 'SLL': '2', 'MGA': '2', 'ANG': '2', 'LRD': '2', 'XCD': '2', 'NOK': '2', 'MXV': '2', 'MOP': '2', 'SSP': '2', 'INR': '2', 'MXN': '2', 'CZK': '2', 'TJS': '2', 'BTN': '2', 'COP': '2', 'MYR': '2', 'COU': '2', 'MUR': '2', 'IDR': '2', 'HNL': '2', 'FJD': '2', 'ETB': '2', 'PEN': '2', 'BZD': '2', 'CHW': '2', 'ILS': '2', 'DOP': '2', 'TWD': '2', 'MDL': '2', 'BSD': '2', 'SEK': '2', 'MVR': '2', 'XTS': 'null', 'AUD': '2', 'SRD': '2', 'CUP': '2', 'CLF': '4', 'BBD': '2', 'KRW': '0', 'GMD': '2', 'VEF': '2', 'GTQ': '2', 'CUC': '2', 'CLP': '0', 'ZMW': '2', 'EUR': '2', 'CDF': '2', 'RWF': '0', 'KZT': '2', 'RUB': '2', 'XAG': 'null', 'TTD': '2', 'OMR': '3', 'BRL': '2', 'MMK': '2', 'PLN': '2', 'PYG': '0', 'KES': '2', 'SVC': '2', 'USD': '2', 'AZN': '2', 'USN': '2', 'TOP': '2', 'VUV': '0', 'GNF': '0', 'WST': '2', 'IQD': '3', 'ERN': '2', 'BAM': '2', 'SCR': '2', 'CAD': '2', 'CVE': '2', 'KWD': '3', 'BIF': '0', 'XXX': 'null', 'PGK': '2', 'SOS': '2', 'SGD': '2', 'UZS': '2', 'STD': '2', 'IRR': '2', 'CNY': '2', 'XOF': '0', 'TND': '3', 'GYD': '2', 'NZD': '2', 'FKP': '2', 'KGS': '2', 'ARS': '2', 'RON': '2', 'RSD': '2', 'BHD': '3', 'JPY': '0', 'SDG': '2', 'XAU': 'null'}


Tell me if you need something else. If I should propose a patch for mozilla-central, etc. Right now it's just under a git on my local computer.

Comment 3

[Jeff Walden [:Waldo]]

Nice!  Indeed it would be good to have something in-tree.  Probably we should update make_intl_data.js to generate both this, *and* the data it already generates, into IntlData.js.  We'd want to remove the |currencyDigits| var out of Intl.js out of there, and generate it into IntlData.js.  Note that *only* the non-2 value entries are needed, or useful (and of course they're only needed a single time in the overall hash).  The current interface to the Python script, that takes a URL to the one file being downloaded now, will have to change.  Not immediately clear the best way, but that's something a little use of optparse would make pretty easy to fix, I think.

I just did an ad hoc update for the semi-recent change to CLF, so our current data is 100% up-to-date, and there's no real rush to make this script-driven.  Feel free to get to it whenever, or throw it back in the pool (unassign yourself) if you don't expect to get to it soon.

Comment 4

[André Bargull [:anba]]

Attachment: bug843758.patch

This patch creates a new file (js/src/builtin/IntlCurrency.js) instead of adding the currency information to the existing IntlData.js file, because it's easier to do it this way.

Comment 5

[Jeff Walden [:Waldo]]

Comment on attachment 8818951 [details] [diff] [review]
bug843758.patch

Review of attachment 8818951 [details] [diff] [review]:
-----------------------------------------------------------------

We smoosh all the self-hosted files together, so a separate file's no big deal, seems like.

::: js/src/builtin/make_intl_data.py
@@ +943,5 @@
> +
> +        if reIntMinorUnits.match(minorUnits):
> +            yield (currency, int(minorUnits))
> +        else:
> +            yield (currency, None)

Couldn't you just not yield anything for such fake currencies?  That seems better than filtering them here, then filtering them down below as well.

@@ +961,5 @@
> +* Spec: ISO 4217 Currency and Funds Code List.
> +* http://www.currency-iso.org/en/home/tables/table-a1.html
> +*/""")
> +        println(u"var currencyDigits = {")
> +        for (_, entries) in groupby(sorted(currencies, key=itemgetter(0)), itemgetter(0)):

What does the groupby stuff do, that simply |sorted(currencies, key=itemgetter(0))| doesn't?  I am confuse.

@@ +962,5 @@
> +* http://www.currency-iso.org/en/home/tables/table-a1.html
> +*/""")
> +        println(u"var currencyDigits = {")
> +        for (_, entries) in groupby(sorted(currencies, key=itemgetter(0)), itemgetter(0)):
> +            (currency, minorUnits) = next(entries)

I assume standalone next is idiomatic, but if it happens not to be, I'd prefer |entries.next()| to clarify what function is called.  I've just never seen this before, myself, and it confused me for a bit.

@@ +965,5 @@
> +        for (_, entries) in groupby(sorted(currencies, key=itemgetter(0)), itemgetter(0)):
> +            (currency, minorUnits) = next(entries)
> +            if minorUnits is None or minorUnits == 2:
> +                continue
> +            println(u"    {}: {},".format(currency, minorUnits))

It'd be nice for readers if we tacked on region/country name and currency name as comments, to help readers of the file understand which currencies these are.  Shouldn't be too hard tacking more items onto the currency-tuples, no?

@@ +1012,5 @@
> +    (thisDir, thisFile) = os.path.split(os.path.abspath(sys.argv[0]))
> +    thisDir = os.path.normpath(thisDir)
> +    if "/".join(thisDir.split(os.sep)[-3:]) != "js/src/builtin":
> +        raise RuntimeError("%s must reside in js/src/builtin" % sys.argv[0])
> +    topsrcdir = "/".join(thisDir.split(os.sep)[:-3])

Separate out |thisDir.split(os.sep)[-3:]| into another variable while you're moving this.

@@ +1059,5 @@
> +
> +    parser_currency = subparsers.add_parser("currency", help="Update currency digits mapping")
> +    parser_currency.add_argument("--url",
> +                                 metavar="URL",
> +                                 default="http://www.currency-iso.org/dam/downloads/lists/list_one.xml",

It appears that this can/should be https now.

Comment 6

[André Bargull [:anba]]

Attachment: bug843758.patch

Updated patch per review comments, carrying r+ from Waldo.

Comment 7

[André Bargull [:anba]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #5)
> ::: js/src/builtin/make_intl_data.py
> @@ +943,5 @@
> > +
> > +        if reIntMinorUnits.match(minorUnits):
> > +            yield (currency, int(minorUnits))
> > +        else:
> > +            yield (currency, None)
> 
> Couldn't you just not yield anything for such fake currencies?  That seems
> better than filtering them here, then filtering them down below as well.

Done.

> @@ +961,5 @@
> > +* Spec: ISO 4217 Currency and Funds Code List.
> > +* http://www.currency-iso.org/en/home/tables/table-a1.html
> > +*/""")
> > +        println(u"var currencyDigits = {")
> > +        for (_, entries) in groupby(sorted(currencies, key=itemgetter(0)), itemgetter(0)):
> 
> What does the groupby stuff do, that simply |sorted(currencies,
> key=itemgetter(0))| doesn't?  I am confuse.

There are duplicate currency codes, so I need to group them together.

> @@ +962,5 @@
> > +* http://www.currency-iso.org/en/home/tables/table-a1.html
> > +*/""")
> > +        println(u"var currencyDigits = {")
> > +        for (_, entries) in groupby(sorted(currencies, key=itemgetter(0)), itemgetter(0)):
> > +            (currency, minorUnits) = next(entries)
> 
> I assume standalone next is idiomatic, but if it happens not to be, I'd
> prefer |entries.next()| to clarify what function is called.  I've just never
> seen this before, myself, and it confused me for a bit.

Yes, it's idiomatic and useful for Python3 forward compatibility: https://www.python.org/dev/peps/pep-3114/

> @@ +965,5 @@
> > +        for (_, entries) in groupby(sorted(currencies, key=itemgetter(0)), itemgetter(0)):
> > +            (currency, minorUnits) = next(entries)
> > +            if minorUnits is None or minorUnits == 2:
> > +                continue
> > +            println(u"    {}: {},".format(currency, minorUnits))
> 
> It'd be nice for readers if we tacked on region/country name and currency
> name as comments, to help readers of the file understand which currencies
> these are.  Shouldn't be too hard tacking more items onto the
> currency-tuples, no?

Done.

> 
> @@ +1012,5 @@
> > +    (thisDir, thisFile) = os.path.split(os.path.abspath(sys.argv[0]))
> > +    thisDir = os.path.normpath(thisDir)
> > +    if "/".join(thisDir.split(os.sep)[-3:]) != "js/src/builtin":
> > +        raise RuntimeError("%s must reside in js/src/builtin" % sys.argv[0])
> > +    topsrcdir = "/".join(thisDir.split(os.sep)[:-3])
> 
> Separate out |thisDir.split(os.sep)[-3:]| into another variable while you're
> moving this.

Hmm, I don't understand this. |thisDir.split(os.sep)[-3:]| is only used once, so why does it help to store in an extra variable?

> 
> @@ +1059,5 @@
> > +
> > +    parser_currency = subparsers.add_parser("currency", help="Update currency digits mapping")
> > +    parser_currency.add_argument("--url",
> > +                                 metavar="URL",
> > +                                 default="http://www.currency-iso.org/dam/downloads/lists/list_one.xml",
> 
> It appears that this can/should be https now.

IIRC currency-iso.org didn't support https at all in December and now they appear to force https. And they've added a stupid server-side bot detection, so I need to use a fake user-agent string to avoid random "ERROR!: error class 'Server Error'; error code '500'; error text 'internal server error'" errors. :-(

Comment 8

[Jeff Walden [:Waldo]]

(In reply to André Bargull from comment #7)
> > @@ +1012,5 @@
> > > +    (thisDir, thisFile) = os.path.split(os.path.abspath(sys.argv[0]))
> > > +    thisDir = os.path.normpath(thisDir)
> > > +    if "/".join(thisDir.split(os.sep)[-3:]) != "js/src/builtin":
> > > +        raise RuntimeError("%s must reside in js/src/builtin" % sys.argv[0])
> > > +    topsrcdir = "/".join(thisDir.split(os.sep)[:-3])
> > 
> > Separate out |thisDir.split(os.sep)[-3:]| into another variable while you're
> > moving this.
> 
> Hmm, I don't understand this. |thisDir.split(os.sep)[-3:]| is only used
> once, so why does it help to store in an extra variable?

Because I misread [-3:] and [:-3] as identical.  Store the more-minimal |thisDir.split(os.sep)| into a variable, then.

> And they've added a stupid server-side bot detection,
> so I need to use a fake user-agent string to avoid random "ERROR!: error
> class 'Server Error'; error code '500'; error text 'internal server error'"
> errors. :-(

lol

Comment 9

[André Bargull [:anba]]

Attachment: bug843758.patch

Updated patch per latest review comments, carrying r+ from Waldo.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/996a1ee2414b
Generate currency information for Intl with make_intl_data.py. r=Waldo

Comment 11

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for failing spidermonkey test 11.1.1_20_c.js:

https://hg.mozilla.org/integration/mozilla-inbound/rev/724e82c14fdbe2c1d0715971be48616aad9ceabe

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=afba45b015683d24e900a31f87966af12c78cfab
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=66757365&repo=mozilla-inbound

[task 2017-01-06T15:23:48.583498Z] TEST-PASS | test262/intl402/ch11/11.1/11.1.1_17.js | (args: "--ion-eager --ion-offthread-compile=off")
[task 2017-01-06T15:23:48.587635Z] ## test262/intl402/ch11/11.1/11.1.1_20_c.js: rc = 3, run time = 0.075984
[task 2017-01-06T15:23:48.587719Z]  PASSED! 
[task 2017-01-06T15:23:48.587810Z] test262/shell.js:921:9 Error: Test262 error: Didn't get correct minimumFractionDigits for currency BYR; expected 0, got 2.
[task 2017-01-06T15:23:48.587838Z] Stack:
[task 2017-01-06T15:23:48.587866Z]   $ERROR@test262/shell.js:921:9
[task 2017-01-06T15:23:48.587901Z]   @test262/intl402/ch11/11.1/11.1.1_20_c.js:188:9
[task 2017-01-06T15:23:48.587936Z]   @test262/intl402/ch11/11.1/11.1.1_20_c.js:182:1
[task 2017-01-06T15:23:48.588037Z] TEST-UNEXPECTED-FAIL | test262/intl402/ch11/11.1/11.1.1_20_c.js | (args: "")
[task 2017-01-06T15:23:48.605088Z] TEST-PASS | test262/intl402/ch11/11.1/11.1.2.1_4.js | (args: "--ion-eager --ion-offthread-compile=off --non-writable-jitcode --ion-check-range-analysis --ion-extra-checks --no-sse3 --no-threads")
[task 2017-01-06T15:23:48.628519Z] TEST-PASS | test262/intl402/ch11/11.1/11.1.2.1_4.js | (args: "--ion-eager --ion-offthread-compile=off")
[task 2017-01-06T15:23:48.660775Z] ## test262/intl402/ch11/11.1/11.1.1_20_c.js: rc = 3, run time = 0.085366
[task 2017-01-06T15:23:48.660846Z]  PASSED! 
[task 2017-01-06T15:23:48.660912Z] test262/shell.js:921:9 Error: Test262 error: Didn't get correct minimumFractionDigits for currency BYR; expected 0, got 2.
[task 2017-01-06T15:23:48.660942Z] Stack:
[task 2017-01-06T15:23:48.660974Z]   $ERROR@test262/shell.js:921:9
[task 2017-01-06T15:23:48.661010Z]   @test262/intl402/ch11/11.1/11.1.1_20_c.js:188:9

Comment 12

[André Bargull [:anba]]

11.1.1_20_c.js expects BYR (Belarusian ruble) is still a valid currency code, but the latest currency update removed BYR (cf. ISO 4217 AMENDMENT NUMBER 161).

Comment 13

[André Bargull [:anba]]

Attachment: bug843758.patch

The failing test case is a test262 test, so we can either mark the test as failing in js/src/tests/jstests.list or simply remove the offending currency code. I've chosen the latter option so we don't reduce our test coverage for currency codes.

Comment 14

[Zibi Braniecki [:zbraniecki][:gandalf]]

Should we also file a test262 issue to remove the BYR code?

Comment 15

[André Bargull [:anba]]

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #14)
> Should we also file a test262 issue to remove the BYR code?

Yes, we should do that.

Comment 16

[Zibi Braniecki [:zbraniecki][:gandalf]]

https://github.com/tc39/test262/pull/818

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1a9af299bd52
Generate currency information for Intl with make_intl_data.py. r=Waldo

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1a9af299bd52