Add a way to register custom currencies #41
Conversation
| // - currencySymbols | ||
| // | ||
| // It returns an error if the code already exists in the ISO list, or if the code is empty. | ||
| func RegisterCurrency(code string, opts RegisterCurrencyOptions) error { |
There was a problem hiding this comment.
This func name gives you currency.RegisterCurrency, which stutters. You want to have currency.Register instead.
code should be currencyCode like in the other functions in currency.go.
| // | ||
| // It returns an error if the code already exists in the ISO list, or if the code is empty. | ||
| func RegisterCurrency(code string, opts RegisterCurrencyOptions) error { | ||
| if code == "" { |
There was a problem hiding this comment.
If the currency code is empty, this func should exit silently, making the operation a no-op. That makes it consistent with other functions in currency.go (such as GetDigits).
| if code == "" { | ||
| return EmptyCurrencyCodeError{} | ||
| } | ||
| if _, isoExists := currencies[code]; isoExists { |
There was a problem hiding this comment.
I would argue that you don't want to have this check and error.
That would allow people to use currency.Register to override existing currencies, e.g. to change their default number of digits, or the default symbols. We had a discussion about digits recently in #37 where this was identified as a valid use case.
| @@ -0,0 +1,51 @@ | |||
| package currency | |||
There was a problem hiding this comment.
This library uses black box testing, all other tests are in currency_test, so this one needs to be as well.
| } | ||
|
|
||
| // RegisterCurrencyOptions defines parameters for registering a new or custom currency. | ||
| type RegisterCurrencyOptions struct { |
There was a problem hiding this comment.
Let's call this type just Definition. That gives you this DX:
currency.Register("BTC", currency.Definition{
NumericCode: "000",
Digits: 2,
})
| // RegisterCurrencyOptions defines parameters for registering a new or custom currency. | ||
| type RegisterCurrencyOptions struct { | ||
| // NumericCode is usually a three-digit code, for example "999". | ||
| NumericCode string |
There was a problem hiding this comment.
We also need a CountryCode here (first in the struct, above NumericCode), to provide data for currency.ForCountryCode
| } | ||
|
|
||
| // Also append to currencyCodes, so that GetCurrencyCodes() is aware of it. | ||
| currencyCodes = append(currencyCodes, code) |
There was a problem hiding this comment.
This code also needs to modify countryCurrencies, see my comment in the param struct above.
| // {Symbol: "₿", Locales: []string{"en"}}, | ||
| // {Symbol: "BTC", Locales: []string{"uk"}}, | ||
| // } | ||
| SymbolData []SymbolData |
There was a problem hiding this comment.
It feels odd to call this SymbolData. Why not just Symbols?
I would expect to see Symbols map[string]currency.Locale, used like this:
Symbols: map[string][]currency.Locale{
"₿": {
currency.Locale{Language: "en"},
},
"BTC": {
currency.Locale{Language: "uk"}},
}
},
That would make it match the GetSymbol API:
// GetSymbol returns the symbol for a currency code.
func GetSymbol(currencyCode string, locale Locale) (symbol string, ok bool) {
Having to initialize a currency.Locale is a bit annoying, but that's how all other parts of this package currently accept a locale, so we're locked in.
As a bonus, use of the map means you no longer need a separate struct for the symbol data.
| } | ||
|
|
||
| // RegisterCurrency adds a non-ISO currency to the global structures: | ||
| // - currencies |
There was a problem hiding this comment.
It doesn't make sense for public docs to mention internal structures, as they could change at any time. You want to mention what the function does, not how it does it.
|
|
||
| // If SymbolData is provided, insert symbols into currencySymbols. | ||
| if len(opts.SymbolData) > 0 { | ||
| // Ensure there's a slice for 'code' in currencySymbols. |
There was a problem hiding this comment.
Comments like these two are not helpful, because they say the same thing that the code does. I'd expect code comments to focus on the "why", not the "what".
|
@dejurin Hi, will you be addressing this PR's feedback in the next 30 days? I'd like to plan the next release. |
|
Hello, I didn't plan on development, so I have to decline. Thank you very much for the package. |
Feature Request: Add a way to register custom (non-ISO) currencies