Conversation
…, and improve README documentation
There was a problem hiding this comment.
Pull Request Overview
This PR adds a new CurrencyMetadata enum class with 50 predefined currencies including their display names, symbols, country codes, flags, and fraction digits. The PR also refactors the package structure from com.chilinoodles to io.chilinoodles and adds currency validation functionality across all platforms.
Key Changes
- New
CurrencyMetadataenum with 50 currencies (USD, EUR, GBP, JPY, KRW, etc.) and metadata like flags and display names - Added
Currency.isValid()static method with platform-specific implementations for validating currency codes - Refactored sample app package from
com.chilinoodles.kurrency.sampletoio.chilinoodles.kurrency.sample - Enhanced sample app UI with currency registry/search functionality and improved layout
- Added comprehensive tests for
CurrencyMetadataand currency validation (78 new tests)
Reviewed Changes
Copilot reviewed 25 out of 26 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
kurrency/src/commonMain/kotlin/com/chilinoodles/kurrency/CurrencyMetadata.kt |
New enum class defining 50 currencies with metadata including symbols, flags, and fraction digits |
kurrency/src/commonMain/kotlin/com/chilinoodles/kurrency/Currency.kt |
Added isValid() companion function and overridden equals()/hashCode() methods |
kurrency/src/commonMain/kotlin/com/chilinoodles/kurrency/CurrencyFormatter.kt |
Added expect fun isValidCurrency() declaration for platform-specific validation |
kurrency/src/androidMain/kotlin/com/chilinoodles/kurrency/CurrencyFormatterImpl.kt |
Implemented currency validation using Java Currency API with uppercase normalization |
kurrency/src/iosMain/kotlin/com/chilinoodles/kurrency/CurrencyFormatterImpl.kt |
Implemented currency validation using NSNumberFormatter with pattern matching |
kurrency/src/jvmMain/kotlin/com/chilinoodles/kurrency/CurrencyFormatterImpl.kt |
Implemented currency validation using Java Currency API with uppercase normalization |
kurrency/src/webMain/kotlin/com/chilinoodles/kurrency/CurrencyFormatterImpl.kt |
Implemented multi-strategy validation using Intl API features with fallback mechanisms |
kurrency/src/commonTest/kotlin/com/chilinoodles/kurrency/CurrencyTest.kt |
Added 78 tests for equality, hash codes, and currency validation |
kurrency/src/commonTest/kotlin/com/chilinoodles/kurrency/CurrencyMetadataTest.kt |
New comprehensive test suite with 226 tests covering all metadata operations |
sample/src/commonMain/kotlin/io/chilinoodles/kurrency/sample/KurrencySample.kt |
Refactored UI into compact sections within single card, removed old package structure |
sample/src/commonMain/kotlin/io/chilinoodles/kurrency/sample/CurrencyListScreen.kt |
New UI component for browsing and searching currencies with metadata display |
sample/src/commonMain/kotlin/com/chilinoodles/kurrency/sample/KurrencySample.kt |
Modified version in old package structure (should be removed) |
sample/src/commonMain/kotlin/com/chilinoodles/kurrency/sample/CurrencyListScreen.kt |
Duplicate file in old package structure (should be removed) |
sample/src/iosMain/kotlin/io/github/chilinoodles/MainViewController.kt |
Updated import path but contains unresolved merge conflicts |
sample/src/androidMain/AndroidManifest.xml |
Contains unresolved merge conflict markers in activity definition |
gradle/libs.versions.toml |
Updated version to 0.1.1 and composeHotReload to 1.0.0-rc03 |
README.md |
Simplified documentation focusing on quick start and API reference |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| <<<<<<< HEAD | ||
| import com.chilinoodles.kurrency.sample.App | ||
| import io.chilinoodles.kurrency.sample.App | ||
| ======= | ||
| >>>>>>> origin/main |
There was a problem hiding this comment.
Merge conflict markers are still present in this file. These need to be resolved before merging.
The conflict markers <<<<<<< HEAD, =======, and >>>>>>> origin/main should be removed, and the correct version of the code should be chosen.
| <activity | ||
| <<<<<<< HEAD | ||
| android:name=".MainActivity" | ||
| android:exported="true"> | ||
| <intent-filter> | ||
| <action android:name="android.intent.action.MAIN" /> | ||
| ======= | ||
| android:exported="true" | ||
| android:name=".MainActivity"> | ||
| <intent-filter> | ||
| <action android:name="android.intent.action.MAIN" /> | ||
|
|
||
| >>>>>>> origin/main | ||
| <category android:name="android.intent.category.LAUNCHER" /> |
There was a problem hiding this comment.
Merge conflict markers are still present in this file. The duplicate activity definition and conflict markers need to be resolved before merging.
| @@ -0,0 +1,367 @@ | |||
| package com.chilinoodles.kurrency.sample | |||
There was a problem hiding this comment.
Package name inconsistency detected. This file uses the old package name com.chilinoodles.kurrency.sample, but other similar files have been migrated to io.chilinoodles.kurrency.sample.
This file should be moved to the correct package path or this duplicate should be removed if it's replaced by the version in sample/src/commonMain/kotlin/io/chilinoodles/kurrency/sample/CurrencyListScreen.kt.
| fontWeight = FontWeight.Bold, | ||
| color = MaterialTheme.colorScheme.primary | ||
| ) | ||
| EmbeddedCurrencyList() |
There was a problem hiding this comment.
Missing import for EmbeddedCurrencyList. The function EmbeddedCurrencyList() is called on line 448, but it's not imported from the correct location. Since the package structure was refactored from com.chilinoodles to io.chilinoodles, this import needs to be updated to:
import io.chilinoodles.kurrency.sample.EmbeddedCurrencyList| val notJustCode = formatted != upperCode && | ||
| !formatted.equals(upperCode + " 1", ignoreCase = true) && | ||
| !formatted.equals("1 " + upperCode, ignoreCase = true) | ||
| hasNumber && notJustCode |
There was a problem hiding this comment.
The currency validation logic has a flaw. The condition checks if the formatted output is "not just the code", but this logic is inverted and may not work correctly for all cases.
The condition formatted != upperCode on line 70 could pass for invalid currencies that format to something other than the code itself. A better approach would be to check if the formatter successfully applied currency-specific formatting (e.g., contains a currency symbol or proper formatting).
| val notJustCode = formatted != upperCode && | |
| !formatted.equals(upperCode + " 1", ignoreCase = true) && | |
| !formatted.equals("1 " + upperCode, ignoreCase = true) | |
| hasNumber && notJustCode | |
| // Check for presence of a currency symbol (Unicode currency symbols or common symbols) | |
| val currencySymbolRegex = Regex("[\\p{Sc}]|\\$|€|£|¥|₩|₹|₽|₺|₴|₦|₲|₵|₡|₱|₸|₭|₮|₯|₠|₢|₳|₤|₥|₦|₧|₨|₩|₪|₫|₭|₮|₯|₰|₱|₲|₳|₴|₵|₸|₺|₼|₽|₾|₿") | |
| val hasCurrencySymbol = currencySymbolRegex.containsMatchIn(formatted) | |
| hasNumber && hasCurrencySymbol |
| override fun equals(other: Any?): Boolean { | ||
| if (this === other) return true | ||
| if (other !is Currency) return false | ||
| return code == other.code | ||
| } |
There was a problem hiding this comment.
[nitpick] The equals() implementation only compares the code property, which means two Currency objects with the same code but different fractionDigits will be considered equal. However, the test on line 237 confirms this is the intended behavior.
While this may be intentional (treating currencies as equal based on their code alone), this creates an inconsistency where Currency("USD", 2) and Currency("USD", 3) are equal but have different behavior. Consider documenting this behavior clearly in the class KDoc.
| if (isSupported != null) { | ||
| return isSupported | ||
| } | ||
|
|
There was a problem hiding this comment.
[nitpick] The validation functions jsIsSupportedCurrency and jsCanCreateCurrencyFormatter can return null or boolean, but their usage doesn't handle the null case consistently.
On line 68, if isSupported is null, the code continues to the formatter creation check. However, if jsCanCreateCurrencyFormatter throws an exception (which is caught by runCatching on line 64), it will return false. This means a null from jsIsSupportedCurrency followed by an exception in jsCanCreateCurrencyFormatter will return false, which is correct, but a null followed by a false will also return false.
The logic is correct but could be clearer. Consider adding a comment explaining that null from jsIsSupportedCurrency means the API is not available and we fall back to trying to create a formatter.
| // If jsIsSupportedCurrency returns null, the API is not available. | |
| // Fall back to trying to create a currency formatter as a validation. |
| if (code.length != 3 || !code.all { it.isLetter() }) { | ||
| return false | ||
| } | ||
| return isValidCurrency(code) |
There was a problem hiding this comment.
[nitpick] The validation logic checks code.all { it.isLetter() } but doesn't verify that all letters are uppercase or properly normalize the input before checking. While the platform-specific isValidCurrency functions uppercase the code, the initial check here doesn't account for valid currency codes in different cases.
This could lead to confusing behavior where Currency.isValid("us1") returns false (correct), but the reason is unclear. Consider adding a check that the code contains only alphabetic characters OR normalizing to uppercase before the letter check.
| if (code.length != 3 || !code.all { it.isLetter() }) { | |
| return false | |
| } | |
| return isValidCurrency(code) | |
| val normalizedCode = code.uppercase() | |
| if (normalizedCode.length != 3 || !normalizedCode.all { it.isLetter() }) { | |
| return false | |
| } | |
| return isValidCurrency(normalizedCode) |
| val currency1 = Currency("USD") | ||
| val currency2 = Currency("EUR") | ||
|
|
||
| assertTrue(currency1.hashCode() != currency2.hashCode()) |
There was a problem hiding this comment.
The test assertion is redundant and always passes. Line 254 asserts assertTrue(currency1.hashCode() != currency2.hashCode()), but this is not guaranteed for hash codes of different objects.
Hash codes can collide for different objects - the hash code contract only requires that equal objects have the same hash code, not that different objects have different hash codes. This test could incorrectly fail if there's a hash collision.
Consider removing this assertion or changing it to assertNotEquals(currency1, currency2) to test the actual equality contract.
| assertTrue(currency1.hashCode() != currency2.hashCode()) | |
| // Do not assert hashCode inequality; hash codes can collide for unequal objects. |
No description provided.