This updates some of the sub unit values#12
This updates some of the sub unit values#12matthewsnell wants to merge 2 commits intovimeo:masterfrom matthewsnell:master
Conversation
nickyr
left a comment
There was a problem hiding this comment.
Hey! I really appreciate you taking the time to do this.
Of course, because currencies are a pain, Facebook's list and the list @shridharama provided in the original ticket don't agree on all currencies. I commented where I saw discrepancies.
Any thoughts from either of you about how we should address this? The ISO link may be more technically accurate, but the Facebook link may be more useful in practice. I'm sort of leaning towards using the mentality "Facebook knows more than me" and accepting all the proposed changes here, but I'd love input from others.
| 'display_name': 'Colombian Peso', | ||
| 'numeric_code': 170, | ||
| 'default_fraction_digits': 2, | ||
| 'sub_unit': 100, |
There was a problem hiding this comment.
Facebook might be wrong on this one. The link @shridharama provided and Wikipedia both indicate that 100 is correct.
There was a problem hiding this comment.
It's possible Facebook isn't "wrong" here per se but is just setting arbitrary rules for the minimum allowed bid on their ad units, which sometimes matches up with established convention for that currency and sometimes doesn't (see here in the docs). In that case, if the sub unit isn't defunct we may want to go with the conventional usage as opposed to Facebook's rules. Thoughts?
There was a problem hiding this comment.
Ooh good find, @kaylafuchs. @matthewsnell, maybe we should leave Colombia and Costa Rica as they were and take your changes on the others?
| 'display_name': 'Costa Rican Colon', | ||
| 'numeric_code': 188, | ||
| 'default_fraction_digits': 2, | ||
| 'sub_unit': 100, |
| 'display_name': 'Forint', | ||
| 'numeric_code': 348, | ||
| 'default_fraction_digits': 2, | ||
| 'sub_unit': 100, |
There was a problem hiding this comment.
now for this one, the two lists disagree but Wikipedia says the sub unit is defunct, so maybe Facebook's is more accurate here
There was a problem hiding this comment.
Was in Hungary this summer & did not ever encounter any denomination smaller than a Forint. Inflation is crazy there so it makes sense that the sub unit would be defunct. Based on those Facebook docs it looks like 1 indicates that there is no sub unit allowed, so it seems they actually agree here!
| 'display_name': 'Rupiah', | ||
| 'numeric_code': 360, | ||
| 'default_fraction_digits': 2, | ||
| 'sub_unit': 100, |
There was a problem hiding this comment.
here's another where wikipedia says the subunit is obsolete
| 'display_name': 'New Taiwan Dollar', | ||
| 'numeric_code': 901, | ||
| 'default_fraction_digits': 2, | ||
| 'sub_unit': 100, |
There was a problem hiding this comment.
For this one, the lists disagree and wikipedia says the subunit is "rarely" used
|
I'll definitely release this with a new major version |
|
I would probably lean more towards using Facebook's values as well. |
|
@shridharama @jcmanzo @kaylafuchs @DanayaMel @brendanpeters @lorenzocastillo I will merge this on Wednesday if nobody objects. |
|
No objections here! I would be inclined to trust Facebook API docs over Wikipedia but it would be nice to have a more authoritative source for this kind of thing. 🤔 Edit: after looking more closely at those Facebook docs it seems that they're tweaking some currency rules in order to set the minimum allowed bid on their ad units. In that case we may not want to trust them. |
|
Looks like this never got merged. I'll take care of it later today.. |
|
@jcmanzo I think there's still an open question here about trusting the Facebook docs for Colombia and Costa Rica because they're using sub units to set a minimum allowed bid on their ad units - https://developers.facebook.com/docs/marketing-api/currencies/#example-offset-1 |
|
I guess the canonical source of truth here should be ISO 4217 which delineates currency designators, country codes and sub units. Wikipedia states:
Even when you encounter sub units rarely in some countries (e.g. for the Forint in Hungary), we should still stick to the Standard if it nominally allows sub units. The latest version of the Standard was published on 29 August, 2018 and can be found here as an XML or XLS file. I ran a small script that loads the XLS file, runs every currency through the I would suggest to approach this as follows:
If we implement these changes, we could add a remark to the We could create an integration test that runs the ISO 4217 file through the |
This fixes the issue #9
Values were taken from https://developers.facebook.com/docs/marketing-api/currencies/