Skip to content
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

Add a way to register custom currencies #41

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
91 changes: 91 additions & 0 deletions register.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,91 @@
package currency

import (
"fmt"
)

// EmptyCurrencyCodeError indicates that the currency code was empty.
type EmptyCurrencyCodeError struct{}

func (e EmptyCurrencyCodeError) Error() string {
return "register currency error: empty currency code"
}

// CurrencyAlreadyExistsError indicates that the currency code already exists in the ISO list.
type CurrencyAlreadyExistsError struct {
Code string
}

func (e CurrencyAlreadyExistsError) Error() string {
return fmt.Sprintf("register currency error: code %q already exists in ISO list", e.Code)
}

// RegisterCurrencyOptions defines parameters for registering a new or custom currency.
type RegisterCurrencyOptions struct {
Copy link
Owner

@bojanz bojanz Jan 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's call this type just Definition. That gives you this DX:

	currency.Register("BTC", currency.Definition{
		NumericCode: "000",
		Digits:      2,
	})

// NumericCode is usually a three-digit code, for example "999".
NumericCode string
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We also need a CountryCode here (first in the struct, above NumericCode), to provide data for currency.ForCountryCode


// Digits is the number of decimal fraction digits.
Digits uint8

// SymbolData is a list of possible symbols and the locales
// in which each symbol is used.
//
// Example:
// []SymbolData{
// {Symbol: "₿", Locales: []string{"en"}},
// {Symbol: "BTC", Locales: []string{"uk"}},
// }
SymbolData []SymbolData
Copy link
Owner

@bojanz bojanz Jan 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

// SymbolData describes one symbol and the set of locales
// for which that symbol applies.
type SymbolData struct {
Symbol string
Locales []string
}

// RegisterCurrency adds a non-ISO currency to the global structures:
// - currencies
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// - currencyCodes
// - 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 {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if code == "" {
Copy link
Owner

@bojanz bojanz Jan 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

return EmptyCurrencyCodeError{}
}
if _, isoExists := currencies[code]; isoExists {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return CurrencyAlreadyExistsError{Code: code}
}

// Insert into the global `currencies` map.
currencies[code] = currencyInfo{
numericCode: opts.NumericCode,
digits: opts.Digits,
}

// Also append to currencyCodes, so that GetCurrencyCodes() is aware of it.
currencyCodes = append(currencyCodes, code)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code also needs to modify countryCurrencies, see my comment in the param struct above.


// If SymbolData is provided, insert symbols into currencySymbols.
if len(opts.SymbolData) > 0 {
// Ensure there's a slice for 'code' in currencySymbols.
Copy link
Owner

@bojanz bojanz Jan 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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".

if _, ok := currencySymbols[code]; !ok {
currencySymbols[code] = []symbolInfo{}
}
// Add each entry from opts.SymbolData.
for _, s := range opts.SymbolData {
currencySymbols[code] = append(
currencySymbols[code],
symbolInfo{
symbol: s.Symbol,
locales: s.Locales,
},
)
}
}

return nil
}
51 changes: 51 additions & 0 deletions register_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
package currency
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This library uses black box testing, all other tests are in currency_test, so this one needs to be as well.


import (
"testing"
)

func TestRegisterCurrencyBTC(t *testing.T) {
err := RegisterCurrency("BTC", RegisterCurrencyOptions{
NumericCode: "1000",
Digits: 8,
SymbolData: []SymbolData{
{
Symbol: "₿",
Locales: []string{"en"},
},
{
Symbol: "BTC",
Locales: []string{"uk"},
},
},
})
if err != nil {
t.Errorf("RegisterCurrency returned an error for BTC: %v", err)
}

if !IsValid("BTC") {
t.Error("Expected 'BTC' to be valid after registration, but IsValid returned false.")
}

d, ok := GetDigits("BTC")
if !ok {
t.Error("Expected 'BTC' to be found, but GetDigits says not ok.")
} else if d != 8 {
t.Errorf("Expected 'BTC' digits=8, got %d", d)
}

symEN, _ := GetSymbol("BTC", NewLocale("en")) // "₿"
if symEN != "₿" {
t.Errorf("Expected '₿' for locale 'en', got '%s'", symEN)
}

symRU, _ := GetSymbol("BTC", NewLocale("uk")) // "BTC"
if symRU != "BTC" {
t.Errorf("Expected 'BTC' for locale 'uk', got '%s'", symRU)
}

err = RegisterCurrency("BTC", RegisterCurrencyOptions{})
if err == nil {
t.Error("Expected an error when re-registering code 'BTC', but got nil.")
}
}
Loading