Support minor currencies #329
Conversation
permits the app to differentiate between major and minor source currency codes (eg GBP vs GBp)
achannarasappa
left a comment
achannarasappa
left a comment
There was a problem hiding this comment.
Thank you for the fix contribution! One suggestion:
- It would be a good improvement at all add error log output where the currency rates are used (
internal/monitor/yahoo/monitor-price/monitor.goandinternal/monitor/coinbase/monitor-price/monitor.go) to detect if an unsupported minor currency is returned
| } | ||
|
|
||
| // This map is derived from https://www.six-group.com/dam/download/financial-information/data-center/iso-currrency/lists/list-one.xls | ||
| var _minorCurrencyCodeByMajorCurrencyCode = map[string]minorCurrency{ |
There was a problem hiding this comment.
Could this list be limited to only trading currencies for major exchanges? I expect most of these currencies do not have trading pairs on any exchanges so the list could be slimed down. This like should be a good start and based on reported issues, others can be added.
USD, JPY, CNY, HKD, GBP, EUR, CAD, INR, AUD, CHF, KRW, TWD, ZAR
There was a problem hiding this comment.
I reduced the list of minor currencies considered to those you suggest.
internal/cli/cli.go
Outdated
| } | ||
|
|
||
| if len(config.Currency) > 0 && (strings.ToUpper(config.Currency) != config.Currency || len(config.Currency) != 3) { | ||
| return errors.New("invalid config: Display currency may only be an ISO 4712 major currency or blank (eg GBP not GBp; default: USD)") //nolint:goerr113 |
There was a problem hiding this comment.
Typo here - should be ISO 4217
There was a problem hiding this comment.
Corrected above and in other places
internal/cli/cli_test.go
Outdated
| }) | ||
| }) | ||
|
|
||
| PWhen("a non-ISO 4712 currency is specified in the config file", func() { |
There was a problem hiding this comment.
To do this properly I'd have to included the full list of ISO 4217 codes. But if you want just the ones suggested above to be included then I'd be happy to do that. Or the full list, whichever you prefer.
| // If any currency in the map has a minor form then add it to the map as well (with a modified rate.) | ||
| minorCurrencyRates := make(map[string]c.CurrencyRate) | ||
| for fromCurrency, currencyRate := range currencyRates { | ||
| if ok, minorCurrencyCode, minorUnit := MinorUnitForCurrencyCode(fromCurrency); ok { |
There was a problem hiding this comment.
Could this be added while iterating through the quotes above?
| }) | ||
| }) | ||
|
|
||
| Describe("currency", func() { |
There was a problem hiding this comment.
Thank you for improving test coverage here!
Bring PR up to date with master
I'm struggling with this, to be honest - hitting the limits of my golang. I've started off by adding this in ...
// check if any currencies weren't found
for _, currency := range fromCurrencies {
missingCurrencies := make([]string, 0)
if _, exists := m.currencyRateCache[currency]; !exists {
missingCurrencies = append(missingCurrencies, currency)
}
if len(missingCurrencies) > 0 {
strings.Join(missingCurrencies, ",")
err := fmt.Errorf("unable to get conversion rates from the following currencies: %s. (Are they unsupported minor currenies?)", strings.Join(missingCurrencies, ","))
m.chanError <- err
continue
}
}
...just between I'll have another crack at it on the weekend. |
2 participants
Thank you for this excellent app!
I'm based in the UK and, while setting it up for my use case, I noticed that although stocks with prices published in Pounds are well handled, those with prices in Pence are not. This has already been reported in issue #282 and this PR attempts to address the issue.
My primary source of information about this was this Wikipedia page and this list of minor currencies referenced from the Wikipedia article.
The changes I've made seem to work correctly but with the limitation that only minor currencies that use mixed case are supported (eg GBp & USd). The alternative minor currency format (eg GBX), which does not seem to be used by Yahoo, is not supported.
Another limitation is that although I've added a validation to check that the display currency in the configuration file is well-formed (blank, 3 uppercase characters (ie major currencies only)), it does not go as far as checking that it's a valid ISO 4217 code.
I don't often use Golang so please don't hesitate to suggest any improvements.