Refactor CurrencyMint #21
Merged
Conversation
This file contains 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
Codecov Report
@@ Coverage Diff @@
## master #21 +/- ##
=========================================
Coverage ? 89.65%
=========================================
Files ? 6
Lines ? 174
Branches ? 0
=========================================
Hits ? 156
Misses ? 18
Partials ? 0
Continue to review full report at Codecov.
|
|
@thonydam This is just something that randomly got in my head. It's a super low priority thing to look at. Let me know what you think. |
Mordil
force-pushed
the
refactor-mint
branch
2 times, most recently
from
0a750bb to
50b00d8
Compare
Mordil
force-pushed
the
refactor-mint
branch
from
50b00d8 to
00442bc
Compare
|
Code Climate has analyzed commit 9c78af4 and detected 0 issues on this pull request. View more on Code Climate. |
Mordil
force-pushed
the
refactor-mint
branch
from
00442bc to
ab6c7ad
Compare
Motivation: The current implementation of CurrencyMint is highly rigid and requires a ton of support from SwiftCurrency to be of true value. Cases of custom currencies and quick fallbacks to a default type requires a fair bit of boilerplate code that can be simplified with an improved re-implementation. In addition, there were several places of code duplication that are possible to simplify with Swift's literal syntax for easier maintenance. Modifications: - Add: `CurrencyIdentifier` for CurrencyMint to use in lookup resolution methods - Add: `standard` static singleton for CurrencyMint to resolve only ISO 4217 currencies - Change: `CurrencyMint` to store a fallback lookup closure for 3rd party and default currency resolution Result: Developers using CurrencyMint should have an easier time supporting their own currencies, or unsupported/missing ISO 4217 currencies.
Mordil
force-pushed
the
refactor-mint
branch
from
ab6c7ad to
9c78af4
Compare
Mordil
added
enhancement
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.
Motivation:
The ISO 4217 standard defines a reasonable "default" currency of XXX - "No currency" - that can be used to remove the need for optionality of the
makeresult.Developers also would want to be able to easily work with CurrencyMint without having to do a bunch of boilerplate with their own custom types.
In addition, the ergonomics of working with CurrencyMint are lacking where Swift's literal syntax can assist and reduce code duplication.
Modifications:
CurrencyIdentifierfor CurrencyMint to use for its factory methodsstandardstatic singleton for CurrencyMint that resolves "XXX" by defaultResult:
Working with CurrencyMint will be easier and more native, especially when integrating custom currency types.