fix(core): JPY (and other) currency fraction digits follow the currency, not the locale#11
Merged
Merged
Conversation
…dependency CurrencyState was the only Compose user in kurrency-core, dragging compose.runtime and the Compose Gradle plugins into what is meant to be the dependency-free foundation module. It is intrinsically a Compose UI state holder (mutableStateOf/derivedStateOf/@composable), so it now lives in kurrency-compose, which already depends on core. - Move CurrencyState, FormattedAmountDelegate, rememberCurrencyState and their test to org.kimplify.kurrency.compose - Remove composeMultiplatform/composeCompiler plugins and compose.runtime from kurrency-core - Drop the debug-log calls that relied on core-internal KurrencyLog - Update sample imports to the new package Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ce set The instrumented tests lived in src/androidTest with deps on the androidInstrumentedTest source set, but the androidLibrary plugin's device-test compilation uses androidDeviceTest (src/androidDeviceTest). As a result connectedAndroidDeviceTest compiled and ran zero tests. - Move instrumented tests to src/androidDeviceTest - Point test deps at androidDeviceTest via getByName (no accessor is generated) - Drop the AndroidX Test Orchestrator execution mode; its test-services APK was never provided, so runs failed with "No test results" - Remove CurrencyStateTestInstrumented (CurrencyState moved to kurrency-compose; it is covered by its commonTest) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
NumberFormat.getCurrencyInstance(locale) seeds the fraction-digit count from the locale's default currency, and java.text's setCurrency() does not update it. So formatting JPY (0 digits) under a non-JP locale produced "¥1,235.00" instead of "¥1,235" (issue #10). - JVM: pin min/max fraction digits to the currency's defaultFractionDigits - Android: switch to ICU's NumberFormat, whose setCurrency() updates the fraction digits (and matches the compact formatter, already on ICU) - Add a common regression test plus an Android instrumented one JS/WasmJs (Intl.NumberFormat) and iOS (NSNumberFormatter) already derived the digits from the currency and were unaffected. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Android's Currency.getInstance (ICU) returns an instance for any well-formed
3-letter code, so isValid("XYZ") wrongly returned true. Validate against the
set of known ISO 4217 currencies instead, matching JVM/iOS behavior.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR fixes incorrect fraction-digit handling when formatting currencies like JPY under non-native locales (issue #10), and includes supporting refactors to decouple kurrency-core from Compose plus repairs to Android instrumented/device testing so the regression is verified on-device.
Changes:
- JVM: pin
NumberFormatmin/max fraction digits to the formatted currency’sdefaultFractionDigits. - Android: switch currency formatting to ICU
NumberFormat(sosetCurrency()updates fraction digits) and tightenisValidCurrencyto ISO-4217 codes. - Project structure/testing: move
CurrencyStateintokurrency-compose, adjust sample imports, and migrate instrumented tests toandroidDeviceTestwith updated Gradle wiring.
Reviewed changes
Copilot reviewed 12 out of 17 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| sample/shared/src/commonMain/kotlin/org/kimplify/kurrency/sample/KurrencySample.kt | Updates import path for rememberCurrencyState after moving Compose state APIs. |
| sample/shared/src/commonMain/kotlin/org/kimplify/kurrency/sample/CurrencyListScreen.kt | Updates import path for rememberCurrencyState after moving Compose state APIs. |
| kurrency-core/src/jvmMain/kotlin/org/kimplify/kurrency/CurrencyFormatterImpl.kt | Forces JVM currency formatter fraction digits to follow the currency (fixes JPY decimals under US locale). |
| kurrency-core/src/commonTest/kotlin/org/kimplify/kurrency/JpyFractionDigitsTest.kt | Adds a common regression test ensuring JPY formatting has no decimals under non-JP locale. |
| kurrency-core/src/androidTest/kotlin/org/kimplify/kurrency/CurrencyStateTestInstrumented.kt | Removes Compose-dependent Android instrumented tests from core (state moved to compose module). |
| kurrency-core/src/androidMain/kotlin/org/kimplify/kurrency/CurrencyFormatterImpl.kt | Switches Android formatting to ICU NumberFormat and changes Android isValidCurrency implementation. |
| kurrency-core/src/androidDeviceTest/kotlin/org/kimplify/kurrency/LocaleFormattingTestInstrumented.kt | Adds Android device tests for formatting behavior across locales (needs minor test logic fixes). |
| kurrency-core/src/androidDeviceTest/kotlin/org/kimplify/kurrency/KurrencyLocaleTestInstrumented.kt | Adds Android device tests for locale parsing and predefined locales. |
| kurrency-core/src/androidDeviceTest/kotlin/org/kimplify/kurrency/KurrencyErrorTestInstrumented.kt | Adds Android device tests for error types/messages and Result propagation. |
| kurrency-core/src/androidDeviceTest/kotlin/org/kimplify/kurrency/JpyFractionDigitsTestInstrumented.kt | Adds Android on-device regression test for JPY decimal handling. |
| kurrency-core/src/androidDeviceTest/kotlin/org/kimplify/kurrency/CurrencyTestInstrumented.kt | Adds broad Android on-device tests for formatting, validity checks, and fraction digits. |
| kurrency-core/src/androidDeviceTest/kotlin/org/kimplify/kurrency/CurrencyMetadataTestInstrumented.kt | Adds Android on-device tests for CurrencyMetadata parsing and data integrity. |
| kurrency-core/build.gradle.kts | Removes Compose deps from core, switches Android target configuration, and wires androidDeviceTest deps. |
| kurrency-compose/src/commonTest/kotlin/org/kimplify/kurrency/compose/CurrencyStateTest.kt | Updates tests to new org.kimplify.kurrency.compose package and adds missing imports. |
| kurrency-compose/src/commonMain/kotlin/org/kimplify/kurrency/compose/CurrencyState.kt | Moves CurrencyState into compose module and removes logging calls. |
| gradle/wrapper/gradle-wrapper.properties | Updates Gradle wrapper version. |
| gradle/libs.versions.toml | Updates Android SDK/tooling and library versions (AGP/Kotlin/Compose/etc.). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Addresses PR review feedback: - createNumberFormat (JVM + Android) uppercased the code only in validation, not when formatting, so lowercase codes (e.g. "usd", which isValid accepts) could throw and silently fall back to the raw amount. Uppercase before the Currency.getInstance lookup. Kotlin's uppercase() is locale-independent. - isValidCurrency (Android) now checks a cached Set of ISO 4217 codes instead of linearly scanning getAvailableCurrencies() on every call. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
kotlinStdlib lagged at 2.3.21 while kotlin moved to 2.4.0, which diverged the JS dependency resolution from the committed yarn.lock and failed CI's kotlinStoreYarnLock check. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The committed lock was from the Kotlin 2.3.20 toolchain; after the version bump CI's kotlinStoreYarnLock failed with "Lock file was changed". Regenerated via kotlinUpgradeYarnLock. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #10. Formatting JPY under a non-Japanese locale produced
¥1,235.00instead of¥1,235, because the formatter inherited the fraction-digit count from the display locale's default currency rather than the currency being formatted.While fixing this,
kurrency-corewas decoupled from Compose (the real reason the Android tests couldn't build/run), and the Android instrumented test suite was repaired so the fix is verified on a real device.Root cause
NumberFormat.getCurrencyInstance(locale)pre-configures the formatter for the locale's default currency — both symbol and fraction-digit count.java.text'ssetCurrency()then swaps the symbol but, by contract, does not update the digit count. So a US-locale formatter kept 2 digits even for JPY. It only worked forLocale.JAPANby coincidence (its default currency is JPY).The fix
NumberFormat, whosesetCurrency()updates the fraction digits automatically (consistent with the compact formatter, already on ICU).min/maxFractionDigitsto the currency'sdefaultFractionDigits.Supporting changes
CurrencyState(the only Compose user in core) moved tokurrency-compose, and the Compose plugins +compose.runtimewere removed fromkurrency-core, restoring it as a dependency-free foundation. This also removed the broken Compose Gradle task that blocked Android tests.src/androidTest(ignored by theandroidLibraryplugin) with deps on the wrong source set, soconnectedAndroidDeviceTestran 0 tests. Moved tosrc/androidDeviceTestand wired correctly.isValidCurrencyon Android —Currency.getInstance(ICU) accepts any well-formed 3-letter code, soisValid("XYZ")wrongly returnedtrue; now validated against the known ISO 4217 set, matching JVM/iOS.Testing
CurrencyState).