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

Release 0.7.0 - Great Refactor #50

Merged
merged 12 commits into from
Jan 8, 2025
Merged

Release 0.7.0 - Great Refactor #50

merged 12 commits into from
Jan 8, 2025

Conversation

Motivation:

Decimal provides the correct semantics for representing currency as it
guarantees representation of values exactly, as opposed to floating
point representations that may be incorrect. This also allows for
correct representations of fractional values during arithmetic, delaying
rounding until the user needs it to avoid "penny shaving".

In addition, the new implementation has much tighter semantics of how
its expected to use a `Currency` value, by not conforming to
`AdditiveArithmetic` and not providing the ability to multiply or divide
two `Currency` values

Modifications:
- Add: New plain protocol `Currency`
- Add: Tests for new protocols and extensions
- Change: Code generation of ISO currencies to include copies conforming
to the new `Currency` protocol
- Rename: `CurrencyMetadata` to `CurrencyDescriptor`
- Remove: `distributedProportionally` algorithm from `Currency`

Result:
The `Currency` type is more correct, with better ergonomics for usage in
a wider array of contexts, while avoiding common pitfalls.
Motivation:

CurrencyMint is a useful abstraction that allows creation of instances
of Currency types dynamically, such as from API responses. The existing
implementation doesn't correctly allow working with existential
instances. There are also language features available in 5.7+ to better
express things in the type system.

Modifications:

- Change: `CurrencyMint` to work with `any Currency` instead of
`AnyCurrency`
- Change: Codgen to return the new `Currency` conforming types rather
than `AnyCurrency`
- Change: Unit tests to be smaller units that better cover the different
behaviors of the type

Result:

`CurrencyMint` is more flexible in the type system and plays better with
modern Swift language features.
#42)

Motivation:

The new implementation has been proven to work, and the old
implementation is ready to be replaced.

Modifications:

- Add: Usage examples of Currency as an existential
- Change: ISO Currency type codegen to drop `_New_` prefix on new
`Currency` conformances
- Remove: `AnyCurrency`, `CurrencyProtocol` protocols and the codegen
conformances
- Remove: Unused `Decimal` extensions

Result:

The new `Currency` implementation is now the canonical representation in
the library.
Motivation:

We're pulling in a dependency for CSV parsing as part of the build tool,
forcing a transitive dependency on all users.

We simply need a way of storing the original definitions from the ISO
Standard committee and parsing them as part of the build process, and
JSON + `Codable` solves that for us easily.

Modifications:

- Change: Code to be a bit more legible on what's going on with the
codegen
- Change: Data parsing from CSV to JSON using `Decodable`
- Rename: Files to be a bit more easily understandable of what's in each
file
- Remove: **SwiftCSV** dependency

Result:

The build plugin is a bit easier to maintain, and there is now no
dependencies in the project.
Motivation:

The minimum version of our library is now Swift 5.7, and our CI does not properly reflect that. In addition, our CI file is manually defined, making it difficult to maintain as platforms come and go.

Modifications:

- Change: CI workflow to use `strategy: matrix` for defining jobs
- Change: CI workflow to cover Linux & macOS across Swift 5.7 - 5.10
- Change: Codecov workflow to make use of Vapor's re-useable GitHub Action

Result:

Maintainers of the library will have an easier time maintaining the platforms that tests run under as well as end-users will have greater confidence that the library will work as advertised on their desired platform.
Motivation:

Swift has a hard time disambiguating symbol references between modules
and types when they share the same symbol name (eg. Currency as the
module name and protocol), especially in downstream dependency chains
when doing certain things.

The current name does not work when users are trying to write extensions
or typealiases on the protocol, making the usability much more limited
than intended.

Modifications:

- Rename: `Currency` protocol to `CurrencyValue`

Result:

Downstream users are now able to easily reference the core protocol in
their own code.
Motivations:

The current `CurrencyValue` protocol has a few requirements with default
implementations that are not meant to be extension points on the
protocol.

In addition, a few aspects of how the protocol can be used are a bit
rough in either performance or generic ergonomics.

Modifications:

- Add: `AdditiveArithmetic` conformance for free
- Add: `@inlinable` annotations to remaining mutating and localization
methods
- Change: `minorUnits` and `roundedAmount` properties / initializers to
be pure extensions

Result:

Developers are able to use `CurrencyValue` in greater generic contexts
with slightly better performance capabilities, while also having more
protection from incorrectly override expected behavior.
Motivation:

As raised in pull request #40, the behavior of dynamically comparing two
different `CurrencyValue` types first by their descriptor is unexpected
behavior. This can cause confusion for end users with no way to work
around the default behavior.

Modifications:

- Change: `Comparable` conformance to only support comparing two of the
same distinct types with `Self` requirements

Result:

Developers will be forced to explicitly state their intention for
comparing two separate `CurrencyValue` instances themselves, giving more
clarity to the behavior and not surprising developers.
Motivation:

Sometimes developers are working in type-erased contexts where they
can't directly refer to the name of the type of the `CurrencyValue` and
need access to the descriptor.

In this cases, having an instance member property to provide the value
is straight forward and convenient.

Modifications:

- Add: `descriptor` instance property to `CurrencyValue`
- Add: `@inlinable` annotations to both the static and instance member
`descriptor` properties

Result:

Developers now have access to the `CurrencyValue` descriptor type either
at the type or instance level.
@Mordil Mordil added the semver-major Require SemVer Major bump label Jan 7, 2025
Copy link

codecov bot commented Jan 7, 2025

Codecov Report

Attention: Patch coverage is 98.67211% with 13 lines in your changes missing coverage. Please review.

Please upload report for BASE (main@ab3dd40). Learn more about missing BASE report.

Files with missing lines Patch % Lines
Sources/Currency/CurrencyValue+Arithmetic.swift 95.06% 4 Missing ⚠️
.../CurrencyTests/CurrencyValue+AlgorithmsTests.swift 91.48% 4 Missing ⚠️
Tests/CurrencyTests/CurrencyValueTests.swift 97.69% 3 Missing ⚠️
.../Currency/CurrencyValue+StringRepresentation.swift 92.00% 2 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main      #50   +/-   ##
=======================================
  Coverage        ?   98.72%           
=======================================
  Files           ?       15           
  Lines           ?     1020           
  Branches        ?        0           
=======================================
  Hits            ?     1007           
  Misses          ?       13           
  Partials        ?        0           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

santicarmo31
santicarmo31 previously approved these changes Jan 8, 2025
Motivation:

A hallmark of a good library is its documentation, and much of the
library was re-written. The documentation needs to be updated to avoid
giving incorrect information to developers, but also can be made more
robust by going in depth on certain topics.

In addition, the README needed to be updated for several broken links
and new infrastructure in use such as for GitHub Actions and Swift
Package Index documentation hosting.

Modifications:

- Add: Unit tests of code samples used in documentation
- Change: README to have updated links and information
- Change: Symbol documentation and articles significantly to be more
in-depth, professional, and up to date
- Remove: No longer used typealias for `CurrencyDescriptor`

Result:

Developers looking to learn about the library and its concepts should
have a more straight forward and clear understanding from within our
code base itself.
Motivation:

Across the ecosystem it has become an idiom to have the Package name match the repository name. SwiftPM even assumes as such in several cases.

The current name conflicts with this expected idiom.

Modifications:

- Rename: Package from `Currency` to `swift-currency`

Result:

The package should match expected ecosystem idioms for naming.
@Mordil Mordil merged commit f5e6101 into main Jan 8, 2025
33 checks passed
@Mordil Mordil deleted the 0.7.0 branch January 8, 2025 02:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-major Require SemVer Major bump
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants