Repository navigation
Alpha Version - #1
Conversation
|
Caution Review failedThe pull request is closed. 📝 WalkthroughSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughRestructures the repository into focused packages (calculator, currency, exchange, parser, exception, format, money), removes legacy base/ files, adds Manager and Converter abstractions, implements DB/JSON hooks, extensive factory and aggregator helpers, introduces comprehensive tests and CI workflows, and adds tooling/Makefile and .gitignore updates. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller
participant Manager as money.Manager
participant Parser
participant CurrencyMgr as currency.Manager
participant Exchange
participant Calculator
Caller->>Manager: CreateFromString("1,234.56", "USD")
Manager->>Parser: ParseAmount("1,234.56", "USD")
Parser-->>Manager: (amountFloat, "USD")
Manager->>CurrencyMgr: Resolve("USD")
CurrencyMgr-->>Manager: *Currency (fraction=2)
Manager->>Calculator: Parse float -> minor units (int64) using fraction
Calculator-->>Manager: amount (int64)
Manager-->>Caller: *Money(amount, currency)
Caller->>Manager: Convert(money, "EUR")
Manager->>Exchange: GetRate("USD","EUR")
Exchange-->>Manager: rate (float64)
Manager->>Calculator: ConvertAmount(amount,int fractions,rate)
Calculator-->>Manager: convertedAmount (int64)
Manager->>CurrencyMgr: Resolve("EUR")
Manager-->>Caller: *Money(convertedAmount, EUR)
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Areas to prioritise during review:
Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (2 inconclusive)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (32)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 32
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
money/db.go (1)
25-69: Add support for[]byteinput in Money.Scan to handle database drivers that pass textual columns as bytesThe current
Scanimplementation only acceptsstringtypes, but many database/sql drivers (including go-sql-driver/mysql and lib/pq) pass textual column values as[]byte. This causes scanning to fail with valid data like"1234|USD"from TEXT/VARCHAR columns, even though the content is correct.The
Currency.DbScanmethod has the same limitation. To fix, add a[]bytecase to the type switch that converts to string first:- switch src.(type) { - case string: - parts := strings.Split(src.(string), DBMoneyValueSeparator) + switch v := src.(type) { + case string: + parts = strings.Split(v, DBMoneyValueSeparator) + case []byte: + parts = strings.Split(string(v), DBMoneyValueSeparator)Move the validation and remaining logic outside the switch. This preserves error messages while supporting both
stringand[]byteinputs from real-world database drivers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (41)
.gitignore(1 hunks)Makefile(1 hunks)README.md(1 hunks)base/Makefile(0 hunks)base/calculator.go(0 hunks)base/currency_test.go(0 hunks)base/db_test.go(0 hunks)base/money.go(0 hunks)base/money_example_test.go(0 hunks)base/money_test.go(0 hunks)base/mutator.go(0 hunks)calculator/calculator.go(1 hunks)calculator/calculator_test.go(1 hunks)currencies/constants.go(4 hunks)currencies/currency.go(1 hunks)currencies/type.go(5 hunks)exception/exception.go(1 hunks)exchange/converter.go(1 hunks)exchange/exchange.go(1 hunks)exchange/tests/converter_test.go(1 hunks)exchange/tests/exchange_test.go(1 hunks)exchange/type.go(1 hunks)formatter/formatter.go(3 hunks)formatter/formatter_test.go(9 hunks)main.go(0 hunks)money/aggregator.go(1 hunks)money/db.go(3 hunks)money/exchange.go(1 hunks)money/factory.go(1 hunks)money/json.go(1 hunks)money/money.go(1 hunks)money/tests/aggregator_test.go(1 hunks)money/tests/db_test.go(1 hunks)money/tests/exchange_test.go(1 hunks)money/tests/extreme_edge_cases_test.go(1 hunks)money/tests/factory_test.go(1 hunks)money/tests/money_methods_test.go(1 hunks)money/type.go(1 hunks)parser/parser.go(1 hunks)parser/parser_test.go(1 hunks)parser/symbols.go(1 hunks)
💤 Files with no reviewable changes (9)
- base/money_example_test.go
- base/mutator.go
- base/db_test.go
- base/money_test.go
- main.go
- base/currency_test.go
- base/Makefile
- base/money.go
- base/calculator.go
🧰 Additional context used
🧬 Code graph analysis (20)
money/tests/factory_test.go (3)
money/type.go (2)
Money(13-16)Amount(9-9)money/factory.go (36)
FromUSD(6-8)FromEUR(11-13)FromGBP(16-18)FromJPY(21-23)FromCHF(26-28)FromCAD(31-33)FromAUD(36-38)FromCNY(41-43)FromINR(46-48)FromBRL(51-53)FromMXN(56-58)FromNZD(61-63)FromSGD(66-68)FromHKD(71-73)FromNOK(76-78)FromSEK(81-83)FromDKK(86-88)FromRUB(91-93)FromZAR(96-98)FromTRY(101-103)FromKRW(106-108)FromPLN(111-113)FromTHB(116-118)FromIDR(121-123)FromMYR(126-128)FromPHP(131-133)FromCZK(136-138)FromHUF(141-143)FromILS(146-148)FromCLP(151-153)FromAED(156-158)FromSAR(161-163)FromARS(166-168)FromCOP(171-173)FromTWD(176-178)FromVND(181-183)currencies/constants.go (36)
USD(153-153)EUR(53-53)GBP(56-56)JPY(76-76)CHF(34-34)CAD(31-31)AUD(12-12)CNY(38-38)INR(70-70)BRL(25-25)MXN(103-103)NZD(112-112)SGD(131-131)HKD(64-64)NOK(110-110)SEK(130-130)DKK(47-47)RUB(124-124)ZAR(181-181)TRY(147-147)KRW(82-82)PLN(119-119)THB(142-142)IDR(68-68)MYR(105-105)PHP(117-117)CZK(45-45)HUF(67-67)ILS(69-69)CLP(37-37)AED(5-5)SAR(126-126)ARS(11-11)COP(39-39)TWD(149-149)VND(160-160)
money/tests/exchange_test.go (4)
money/money.go (1)
NewMoney(12-17)currencies/constants.go (2)
USD(153-153)EUR(53-53)money/exchange.go (2)
Convert(8-25)ConvertWithRate(28-43)exception/exception.go (2)
ErrCurrencyConversionNotFound(9-9)ErrInvalidExchangeRate(8-8)
money/tests/aggregator_test.go (5)
money/aggregator.go (4)
Sum(9-23)Min(27-48)Max(52-73)Avg(77-98)exception/exception.go (2)
ErrNoMoneyToSum(10-10)ErrCurrencyMismatch(6-6)money/money.go (1)
NewMoney(12-17)currencies/constants.go (2)
USD(153-153)EUR(53-53)money/type.go (1)
Amount(9-9)
exception/exception.go (3)
parser/parser.go (1)
New(13-15)formatter/formatter.go (1)
New(17-25)exchange/exchange.go (1)
New(9-13)
parser/symbols.go (1)
currencies/constants.go (9)
BRL(25-25)CAD(31-31)AUD(12-12)USD(153-153)EUR(53-53)GBP(56-56)JPY(76-76)INR(70-70)CHF(34-34)
money/aggregator.go (2)
money/type.go (1)
Money(13-16)exception/exception.go (1)
ErrNoMoneyToSum(10-10)
exchange/converter.go (1)
exchange/type.go (2)
Exchange(23-25)Converter(28-30)
formatter/formatter_test.go (1)
formatter/formatter.go (1)
New(17-25)
money/db.go (1)
money/type.go (2)
Money(13-16)Amount(9-9)
parser/parser_test.go (2)
currencies/constants.go (5)
USD(153-153)EUR(53-53)CAD(31-31)AUD(12-12)BRL(25-25)parser/parser.go (2)
New(13-15)ParseDecimal(65-69)
money/tests/money_methods_test.go (5)
money/money.go (2)
NewMoney(12-17)NewFromFloat(21-27)currencies/constants.go (3)
USD(153-153)JPY(76-76)EUR(53-53)money/type.go (2)
Amount(9-9)Money(13-16)exception/exception.go (2)
ErrCurrencyMismatch(6-6)ErrInvalidJSONUnmarshal(7-7)money/json.go (2)
MarshalJSON(14-14)UnmarshalJSON(13-13)
money/json.go (2)
exception/exception.go (1)
ErrInvalidJSONUnmarshal(7-7)money/money.go (1)
NewMoney(12-17)
money/tests/db_test.go (3)
money/money.go (1)
NewMoney(12-17)money/type.go (2)
Money(13-16)Amount(9-9)currencies/currency.go (1)
NewCurrency(56-58)
exchange/exchange.go (2)
exchange/type.go (1)
Exchange(23-25)exception/exception.go (2)
ErrInvalidExchangeRate(8-8)ErrCurrencyConversionNotFound(9-9)
money/tests/extreme_edge_cases_test.go (5)
money/money.go (1)
NewMoney(12-17)currencies/constants.go (3)
JPY(76-76)USD(153-153)EUR(53-53)money/type.go (1)
Amount(9-9)money/exchange.go (1)
ConvertWithRate(28-43)money/db.go (1)
DBMoneyValueSeparator(16-16)
currencies/currency.go (2)
currencies/type.go (2)
Currencies(14-14)Currency(4-12)formatter/formatter.go (1)
Formatter(9-15)
calculator/calculator_test.go (1)
calculator/calculator.go (3)
NewCalculator(12-14)Amount(6-6)Calculator(9-9)
money/money.go (5)
money/type.go (2)
Money(13-16)Amount(9-9)currencies/currency.go (1)
NewCurrency(56-58)exception/exception.go (2)
ErrCurrencyMismatch(6-6)ErrNoMultipliersProvided(13-13)calculator/calculator.go (1)
NewCalculator(12-14)formatter/formatter.go (1)
Formatter(9-15)
exchange/tests/converter_test.go (3)
exchange/exchange.go (1)
New(9-13)exchange/converter.go (1)
NewConverter(3-7)exchange/type.go (1)
Converter(28-30)
money/factory.go (3)
money/type.go (1)
Money(13-16)money/money.go (1)
NewMoney(12-17)currencies/constants.go (36)
USD(153-153)EUR(53-53)GBP(56-56)JPY(76-76)CHF(34-34)CAD(31-31)AUD(12-12)CNY(38-38)INR(70-70)BRL(25-25)MXN(103-103)NZD(112-112)SGD(131-131)HKD(64-64)NOK(110-110)SEK(130-130)DKK(47-47)RUB(124-124)ZAR(181-181)TRY(147-147)KRW(82-82)PLN(119-119)THB(142-142)IDR(68-68)MYR(105-105)PHP(117-117)CZK(45-45)HUF(67-67)ILS(69-69)CLP(37-37)AED(5-5)SAR(126-126)ARS(11-11)COP(39-39)TWD(149-149)VND(160-160)
🪛 checkmake (0.2.2)
Makefile
[warning] 1-1: Missing required phony target "all"
(minphony)
[warning] 1-1: Missing required phony target "clean"
(minphony)
🪛 LanguageTool
README.md
[uncategorized] ~292-~292: Possible missing comma found.
Context: ...ies This library supports all ISO 4217 currencies including: - Major: USD, EUR, GBP,...
(AI_HYDRA_LEO_MISSING_COMMA)
[locale-violation] ~370-~370: License must be spelled with a “c” when used as a noun in British English. Use “licence”.
Context: ...ystem with explicit rate management ## License This project is licensed under the MIT...
(LICENCE_LICENSE_NOUN_SINGULAR)
[locale-violation] ~372-~372: License must be spelled with a “c” when used as a noun in British English. Use “licence”.
Context: ... This project is licensed under the MIT License - see the LICENSE file for d...
(LICENCE_LICENSE_NOUN_SINGULAR)
[locale-violation] ~372-~372: LICENSE must be spelled with a “c” when used as a noun in British English. Use “licence”.
Context: ...censed under the MIT License - see the LICENSE file for details. ## Credits...
(LICENCE_LICENSE_NOUN_SINGULAR)
[style] ~380-~380: Consider using a less common alternative to make your writing sound more unique and professional.
Context: ...ontributing Contributions are welcome! Please feel free to submit a Pull Request. ## Resources -...
(FEEL_FREE_TO_STYLE_ME)
🔇 Additional comments (39)
.gitignore (1)
11-11: Good placement in the Go artifacts section.The addition of
.gocacheis logically grouped with other Go workspace files. However, please verify that.gocacheis the correct cache directory pattern used by your project's tooling, as the standard Go build cache typically resides at$HOME/.cache/go-buildrather than a project-relative.gocachedirectory.exception/exception.go (1)
5-14: Centralised error definitions look appropriateThe shared error values cover the main failure modes (currency mismatch, parsing, JSON, exchange, aggregation) and will make it easier to compare errors across packages. The messages are clear and neutral; no changes needed here.
parser/parser.go (1)
11-15: LGTM!The
Parserstruct andNew()constructor are simple and appropriate for this use case.currencies/type.go (4)
16-17: LGTM!Renaming
currenciestocurrenciesMapavoids shadowing the package name and improves clarity. The variable naming convention is appropriate.
37-53: LGTM!The added ISO 4217 fund codes (BOV, CHE, CHW, COU) are valid additions that expand the library's coverage for specialised financial instruments.
167-170: LGTM!USN (US Dollar Next Day), UYI (Uruguay Peso en Unidades Indexadas), and UYW (Unidad Previsional) are valid ISO 4217 fund codes.
179-192: LGTM!The precious metals (XPD, XPT), bond market units (XBA-XBD), and special codes (XSU, XTS, XUA, XXX) are valid ISO 4217 additions. XXX (no currency) and XTS (testing) are particularly useful for edge case handling.
exchange/converter.go (1)
9-15: LGTM!The nil receiver check in
GetExchangeis a defensive pattern that prevents panics when called on a nilConverter.exchange/tests/converter_test.go (1)
9-20: LGTM!Good test coverage verifying that
NewConverterreturns a non-nilConverterand thatGetExchangereturns the correct exchange instance.money/tests/exchange_test.go (1)
12-46: Conversion tests cover core success and error paths wellThe test cases exercise same‑currency behaviour, missing conversion rate handling, explicit‑rate conversion and invalid‑rate errors in a clear way using
errors.Is; this looks solid and aligned with the money/exchange APIs.money/type.go (1)
8-16: Amount alias and Money struct wiring look soundAliasing
Amounttocalculator.Amountand embedding*currencies.Currencywith DB tags gives a clean, cohesive core type for the money package, with unexported fields enforcing use of helpers.money/tests/db_test.go (2)
10-89: Money DB serialisation / scan tests give good coverageThe
Money.ValueandMoney.Scantests cover happy paths (including negatives) and key failure modes (missing separator, empty components, wrong type), which matches theValue/Scancontract and should catch most persistence regressions.
91-150: Currency DbValue/DbScan tests accurately capture expected behaviourThe currency tests validate upper‑casing, handling of lower‑case input, and appropriate errors for invalid types or codes, which aligns well with a strict currency model and centralises assumptions around valid codes.
formatter/formatter_test.go (1)
1-188: Formatter tests provide strong coverage and robust float comparisonsThe expanded table‑driven tests for
FormatandToMajorUnitsexercise a wide range of formats and edge cases, and switchingToMajorUnitsto an epsilon‑based check withmath.Absis a sensible way to avoid fragile float equality.currencies/constants.go (1)
1-184: LGTM!The expanded ISO 4217 currency constant set is well-structured and correctly includes funds, precious metals, bond market units, supranational currencies, and test codes. The alphabetical ordering is maintained throughout.
formatter/formatter.go (1)
1-71: LGTM!The refactoring improves consistency:
- Constructor renamed to
Newfollowing Go conventions- Pointer receivers are now consistent across all methods
- Field documentation enhances readability
math.Pow10simplifies the calculation inToMajorUnitsmoney/tests/money_methods_test.go (2)
1-446: Comprehensive test coverage.The test suite thoroughly covers:
- Constructors and accessors
- Currency validation and comparison
- Arithmetic operations with error handling
- Sign checks and absolute/negative conversions
- Splitting and allocation
- JSON marshalling/unmarshalling with error cases
- Comparison edge cases
Good use of table-driven tests and
errors.Isfor error assertions.
350-352: Remove the suggestion to change the JSON format in the test.The MarshalJSON implementation in
money/json.go:86explicitly usesfmt.Sprintf(\{"amount": %d, "currency": "%s"}`, ...)` which produces JSON with spaces after colons. The test assertion at lines 350-352 correctly matches this actual output format.Likely an incorrect or invalid review comment.
money/tests/extreme_edge_cases_test.go (1)
61-76: Test expectation is correct.The conversion calculation is accurate: 123 minor units USD (Fraction: 2) × 1.5 = 184.5 minor units USD → 1.845 major units USD. When converting to JPY (Fraction: 0), the conversion yields 1.845 major JPY, which truncates to 1 JPY via
int64()cast. Since JPY has no decimal fraction,result.Amount() == 1is the correct expected value.money/exchange.go (1)
17-17: No validation required—Get()safely returns a default Currency struct.The concern is unfounded.
NewCurrency(toCurrency).Get()never returns nil or a zero-value struct. TheGet()method checks if the currency exists incurrenciesMapand returns it if found; otherwise, it returns a defaultCurrencystruct withFraction: 2viaGetDefault(). Therefore, accessing.Fractionat lines 17 and 34 is safe and cannot cause a nil pointer dereference, even with invalid currency codes.Likely an incorrect or invalid review comment.
exchange/exchange.go (3)
9-13: LGTM!Constructor correctly initialises the nested map structure for storing exchange rates.
15-27: LGTM!
AddRateproperly validates the receiver and rate, lazily initialises the inner map, and stores the rate. The validation forrate <= 0correctly rejects zero and negative rates.
29-52: LGTM!
GetRatehandles identity conversion (same currency), direct lookup, and inverse rate calculation correctly. The fallback chain is well-structured.money/json.go (2)
11-25: LGTM!The overridable hooks pattern allows runtime customisation of JSON behaviour without modifying the type methods. The delegation is clean.
80-89: LGTM!The marshalling correctly handles zero-value Money by substituting with a properly initialised instance before formatting. The output format is consistent.
currencies/currency.go (4)
22-38: LGTM!
CurrencyByCodecorrectly uses map lookup, andAddproperly updates the currencies map. The fluent return ofCurrenciesenables method chaining.
40-54: LGTM!
AddCurrencyprovides a convenient way to define custom currencies with all formatting parameters and automatically registers them in the global map.
56-70: LGTM!
NewCurrencyandGetCurrencycorrectly handle case normalisation withstrings.ToUpper.GetCurrencyByNumericCodedelegates appropriately.
107-131: LGTM!Database serialisation is well-implemented.
DbScancorrectly validates the source type and handles the case whereGetCurrencyreturns nil for unknown codes.money/money.go (9)
12-17: LGTM!
NewMoneycorrectly initialises the Money struct with the amount and retrieves the full currency details viaNewCurrency(code).Get().
29-47: LGTM!Accessor methods and currency comparison are straightforward and correct.
49-98: LGTM!Comparison methods correctly check currency consistency before comparing amounts. The
CompareAmountswitch pattern is idiomatic.
100-124: LGTM!Sign check methods and
Absolute/Negativetransformations are correctly implemented, delegating arithmetic to the calculator.
126-170: LGTM!
AddandSubtractcorrectly validate currency consistency for each operand and accumulate values using the calculator. Empty operand handling returns self, which is sensible.
172-188: LGTM!
Multiplycorrectly rejects empty multiplier lists and compounds all multipliers before applying to the amount.
199-229: LGTM!
Splitcorrectly handles the division with leftover distribution using round-robin allocation. The sign handling for negative amounts ensures leftovers are distributed correctly in either direction.
231-286: LGTM!
Allocatehas comprehensive validation (negative ratios, overflow protection) and correctly distributes leftovers. The early return for zero-sum ratios is a good edge case handling.
288-299: LGTM!
DisplayandAsMajorUnitscorrectly delegate to the currency's formatter for consistent output.money/factory.go (1)
1-58: LGTM!Factory functions provide convenient currency-specific constructors. The pattern is consistent and delegates correctly to
NewMoney.
No description provided.