-
-
Notifications
You must be signed in to change notification settings - Fork 108
Support historical currencies #104
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
base: master
Are you sure you want to change the base?
Conversation
|
So apparently I've managed to create test that displays disconnection of number code with currency code. Code 704 changed its currency from VNC to VND. When you ask for original VNC, you get number code 704. However, when you try to retrieve code 704, you get only VND. Now, is that actually a problem? If so, |
ed9a2f4 to
d81e765
Compare
|
@BenMorel Hello, I've decided to move numeric codes out of |
BenMorel
left a comment
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.
Hi, thanks a lot for tackling this, and sorry for the delay!
I've left a few comments on the code itself, mostly nitpicks.
As for the main functionality:
There are duplicities when it comes to country codes (CS = Czechoslovakia & Serbia and Montenegro). For the library I've decided to add all currencies to the code of CS
I'm OK with this. As I understand, CS is the only code that was ever reused, and both usages are historical, so it should not matter much.
Currency codes and their numeric ID might change over time (948 changed from CHC to CHW). For the library I've decided to use the most recent value when if conflict
I've noticed that too, and this is not an isolated case, so we need to document this fact in every method accepting a numeric code, and have a clear release process for these changes (like, allow such changes in minor versions).
You've deprecated using numeric codes in ISOCurrencyProvider::getCurrency(), so this will affect Currency::of() and Money::of(). Are we OK with the fact that ultimately, Money::of() will refuse to work with numeric currency codes?
This means that we'll have to do something like:
$eur = Currency::ofNumericCode(978);
$money = Money::of(1, $eur);vs today:
$money = Money::of(1, 978);While this is an extra step, I can see a couple benefits:
Money::of(1, 978)is harder to read thanMoney::of(1, 'EUR'), so the extra step might actually make the code easier to read in some cases;- it's easier to document the fact that numeric codes might change in minor versions in a method that's dedicated to returning currencies by numeric code, vs documenting it on
Money::of()and every other place that accepts a Currency instance, an alpha code and a numeric code.
There is no list of minor units when it comes to historical currencies. I've used Wiki as a source and manually went through currencies where data were missing. There might be errors, but I believe those could be fixed in the future if necessary.
Excellent, I think this is good enough indeed, thank you!
Additional point regarding to minor units is their change over time. For each of those cases I've chosen the highest number of digits. Any other approach would introduce the necessity of using dates, which I wanted to avoid.
Can you give me an example of this?
With numerical code being regularly reused, I felt necessary to move it out to separated methods. This way, users may understand issues that numerical code brings (and benefits, as numerical code seems to always point to same country).
|
Thank you. It looks like this change happened circa 1960, before ISO 4217 was established. It's only referenced in the historical data file, so I'm OK with using the latest value for decimal digits. I asked AIs about similar cases that would have happened since ISO 4217 is a thing, and they could fine none. Hopefully this will never be a problem for existing currencies. |
|
@BenMorel I reacted to all your entries, keeping up those that need a review by you. I also tried to update BGN, but Six Group made a mistake and marked BGL as the latest withdrawn currency in list-three file. Send them mail, hopefully somebody will fix that soon :) |
b62dff7 to
2945f46
Compare
You were faster than me this time, so I will revisit your comments. |
6d63eb7 to
17ad091
Compare
|
@BenMorel Done |
Fixed already, so I've included BGN recalculation into this PR. If you want to keep it separated, you may just cherry-pick my last commit. |
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.
Pull request overview
This pull request adds support for historical currencies to the brick/money library. The implementation introduces a new CurrencyType enum to distinguish between current ISO currencies (IsoCurrent), historical ISO currencies (IsoHistorical), and custom application-defined currencies (Custom). The PR processes both current and historical currency data from ISO 4217 lists, adds new methods to query currencies by type and country, and deprecates methods that accept numeric currency codes in favor of explicit alternatives.
Key Changes:
- Introduces
CurrencyTypeenum with three cases:IsoCurrent,IsoHistorical, andCustom - Adds historical currency support by importing and processing ISO 4217 list-three.xml file
- Separates current and historical currencies into distinct data files and API methods
- Deprecates
getCurrency()with integer argument andgetCurrenciesForCountry()methods
Reviewed changes
Copilot reviewed 12 out of 13 changed files in this pull request and generated 13 comments.
Show a summary per file
| File | Description |
|---|---|
| src/CurrencyType.php | New enum defining currency types (IsoCurrent, IsoHistorical, Custom) |
| src/Currency.php | Updated constructor to accept CurrencyType, added getCurrencyType() and ofNumericCode() methods |
| src/ISOCurrencyProvider.php | Added getCurrencyByNumericCode(), getCurrentCurrenciesForCountry(), and getHistoricalCurrenciesForCountry() methods; deprecated old methods |
| tests/CurrencyTest.php | Updated tests to include CurrencyType parameter and added historical currency test case |
| tests/ISOCurrencyProviderTest.php | Added tests for historical currencies and updated existing tests with CurrencyType |
| tests/AbstractTestCase.php | Updated assertCurrencyEquals helper to validate CurrencyType |
| import-currencies.php | Refactored to process both current and historical currency XML files with manual minor units mapping |
| data/iso-currencies.php | Expanded from 168 to 270+ currencies including historical ones with CurrencyType |
| data/numeric-to-currency.php | Added mappings for historical currency numeric codes |
| data/country-to-currency.php | Updated to only contain current currencies; changed BG to EUR, CW/SX to XCG, removed CUC from CU |
| data/country-to-currency-historical.php | New file mapping countries to their historical currencies |
| composer.json | Added symfony/deprecation-contracts dependency |
| .gitignore | Added .idea/ directory to ignore list |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@BenMorel Solved |
|
Thank you, sorry Copilot was a bit noisy, but had some interesting comments anyway! I'll give the PR a final review later today. |
| // Library does not support numeric currency codes of historical currencies | ||
| if ($currencyType === CurrencyType::IsoCurrent) { |
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.
This is confusing, as it does support it now! Only one of the two historical tests fails here, can we fix it, or create separate tests for string and numeric currency codes?
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 disagree, the test itself is proof that library does not handle numeric codes well. And it has to be reflected. I could not come up with any workaround for this, and creating separated tests would just require to ignore those historical cases anyway.
If anything, we should document this behavior.
8e4b3eb to
285ce85
Compare
285ce85 to
57fdb3a
Compare
Hi. This is my simplified approach to add historical currencies. It maintains the spirit of the original code (at least I hope so!).
There are some issues when it comes to historical currencies, yet I would ignore them.
Unless somebody imports generated files directly, there should be no BC breaks.
With this approach, I believe currencies can be handled immediately without issuing new major version (looking at BGN => EUR coming soon!). After all, existing differences were already processed.