Skip to content

Make Quantity extensible via RawRepresentable struct#23

Open
DarkAndHungryGod wants to merge 3 commits into
NeedleInAJayStack:mainfrom
DarkAndHungryGod:feat/extensible-quantity
Open

Make Quantity extensible via RawRepresentable struct#23
DarkAndHungryGod wants to merge 3 commits into
NeedleInAJayStack:mainfrom
DarkAndHungryGod:feat/extensible-quantity

Conversation

@DarkAndHungryGod

@DarkAndHungryGod DarkAndHungryGod commented Jun 25, 2026

Copy link
Copy Markdown

Summary

Resolves the long-standing // TODO: Consider changing away from enum for extensibility in Quantity.swift by changing Quantity from a closed enum into a RawRepresentable struct that exposes the built-in dimensions as static constants (the same pattern as Notification.Name).

This lets callers define their own dimensions — e.g. a Money dimension for cost/estimating calculations — without forking the package or repurposing an unrelated base quantity like .amount:

extension Quantity {
    static let money = Quantity(rawValue: "Acme.Money")
}

A dedicated dimension can't silently cancel against unrelated units, which is exactly what you want for things like currency.

This revision also folds in the review feedback (naming, description, collision docs) — see below.

API

  • Quantity is now public struct Quantity: RawRepresentable, Hashable, Sendable, with the built-in dimensions as static let constants.
  • Naming: the static properties are lowerCamelCase (.length, .mass, .time, …) per the Swift API Design Guidelines. The former PascalCase spellings (.Length, .Mass, …) are retained as @available(*, deprecated, renamed:) aliases, so existing call sites keep compiling (with a deprecation warning) and can migrate at their own pace. The aliases can be removed in a future major release.
  • CustomStringConvertible: added, returning rawValue. This preserves the enum's behavior where "\(Quantity.length)" prints Length rather than the synthesized struct description.

Compatibility

  • [Quantity: Int] dictionary keys still work (Hashable is preserved).
  • The \Quantity.rawValue keypath used in dimensionDescription() still works.
  • Sendable is preserved.
  • Existing .Length-style call sites still compile via the deprecated aliases (lowerCamelCase is the new canonical spelling).

One behavioral change worth calling out: the enum previously synthesized a failable init?(rawValue:); the struct has a non-failable init(rawValue:), since any string is now a valid dimension. The risk is effectively zero (that init appears to be rarely used directly), but because that plus the property rename are small API changes. I deliberately did not add Comparable/Codable conformances, to keep the conformance set close to the original enum — the symbolic serialization already sorts by rawValue, so output ordering is unchanged.

Tests

  • Adds QuantityTests covering: built-in raw values, the CustomStringConvertible description, custom-dimension identity and use as a dictionary key, and arithmetic cancellation of a custom dimension ($/m^3 * m^3 -> $).
  • Existing tests are updated to the new lowerCamelCase dimension names.

Docs

  • Adds a "Custom Dimensions" subsection to the README with a Money worked example.
  • Documents that a Quantity is identified solely by its rawValue across every linked module, so two modules choosing the same raw string silently collapse into one dimension. Recommends prefixing custom raw values (e.g. "Acme.Money") to avoid collisions.
  • Updates the existing Custom Units examples to the new lowerCamelCase names.

@DarkAndHungryGod

Copy link
Copy Markdown
Author

A heads-up on the extra commit touching Tests/UnitsTests/DefinitionTests.swift — it's unrelated to the feature, but my PR caused the macOS CI failure, so I wanted to explain rather than have it look out of place. My PR doesn't touch this code, so it is a side effect of the change.

What failed

The macOS job failed (Linux 5.10/6.0/6.1 all passed) with:

DefinitionTests.swift:266:9: error: the compiler is unable to type-check
this expression in reasonable time; try breaking up the expression into
distinct sub-expressions

That's the 1°F assertion in testTemperature(), (a pre-existing line added in 2024, untouched by this PR). It does alot: a throwing optional unwrap, a mixed Int/Double literal chain (273.15 - (32 * 5.0 / 9.0)), two .measured(in:) calls, a throwing +, and a .convert(to:) into a single expression. It's been sitting right at the type-checker's expression budget.

Why this PR surfaced it

Measurement/Unit carry dimensions as [Quantity: Int]. Changing Quantity from enum: String to a RawRepresentable struct slightly raises the constraint-solver cost of every measurement expression — just enough to tip this borderline line over the budget. Because the budget is effectively wall-clock, it only trips on the slower macOS runner (Xcode latest-stable); the Linux toolchains stay under it, and I couldn't reproduce it locally on Swift 6.2.4 either.

The fix

Took the compiler's advice and split the expression into typed sub-expressions — identical arithmetic and assertion:

let fahrenheitOffset: Double = 273.15 - (32 * 5.0 / 9.0)
let fahrenheitScale: Double = 5.0 / 9.0
let oneFahrenheit = try fahrenheitOffset.measured(in: .kelvin) + fahrenheitScale.measured(in: .kelvin)
try XCTAssertEqual(
    XCTUnwrap(Measurement("1°F")),
    oneFahrenheit.convert(to: .fahrenheit),
    accuracy: 0.0001
)

testTemperature passes locally and the expression now type-checks well within budget. Happy to pull this into a separate PR if you'd rather keep it out of the feature change — just let me know.

@NeedleInAJayStack NeedleInAJayStack left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall this is a well-executed change — the Notification.Name pattern is the right idiom here, the call sites are source-compatible, and the tests are concrete and useful. I have a few concerns worth addressing before merge, ranging from a behavioral regression to naming and scope questions.

Summary of findings:

  1. Missing CustomStringConvertible — silent behavioral regression in string output (see inline)
  2. rawValue collision is under-warned — the core risk of the open extensibility model deserves more guidance
  3. Uppercase static property names violate Swift API Design Guidelines (opportunity to fix at the seam)
  4. Unrelated test refactor bundled in this PR — should be separated or explained

Generated by Claude Code

@NeedleInAJayStack NeedleInAJayStack left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good implementation of a long-requested feature — the struct + RawRepresentable pattern is the right call and the existing call sites hold up cleanly. Three issues need addressing before merge:

  1. CustomStringConvertible is missing — a silent behavioral regression. "\(Quantity.Length)" currently prints "Length"; after this PR it prints "Quantity(rawValue: \"Length\")". Fix: add extension Quantity: CustomStringConvertible { public var description: String { rawValue } }.

  2. Static property names are PascalCasepublic static let Length etc. violate Swift API Design Guidelines (lowerCamelCase for properties). Since you're already taking a minor-version bump, this is the right time to rename them (or ship deprecation shims alongside the new names).

  3. rawValue collision warning is too weak — the core footgun of open extensibility is silent dimensional collapse when two modules use the same raw string. The README should state the consequence explicitly and ideally recommend a namespacing convention.

The unrelated Fahrenheit test refactor in DefinitionTests.swift should be noted in the PR description or separated into its own commit.


Generated by Claude Code

///
/// ```swift
/// extension Quantity {
/// static let money = Quantity(rawValue: "Money")

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing CustomStringConvertible — this is a silent behavioral regression.

With the enum, "\(Quantity.Length)" (and String(describing:)) printed "Length". With the struct it will now print "Quantity(rawValue: \"Length\")" — the default struct description. Any code that logs or displays a Quantity value (or a [Quantity: Int] dictionary) will silently change its output.

Please add:

extension Quantity: CustomStringConvertible {
    public var description: String { rawValue }
}

This restores the prior behaviour and is consistent with how Notification.Name prints its string value.


Generated by Claude Code

case Data
public static let Angle = Quantity(rawValue: "Angle")
public static let Data = Quantity(rawValue: "Data")
}

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Swift API Design Guidelines: static properties should be lowerCamelCase.

public static let Length, public static let Time, etc. should be public static let length, public static let time, … These were enum cases before (where PascalCase is conventional), but as struct static properties the guideline is clear: lowerCamelCase.

This is a source-breaking change on existing call sites (.Length.length), but you're already accepting a minor version bump for the failable-init change, so this is the right moment to fix it. If you'd rather defer, please at least add a deprecation shim:

@available(*, deprecated, renamed: "length")
public static let Length = Quantity(rawValue: "Length")
public static let length = Quantity(rawValue: "Length")

Either way, leaving public static properties in PascalCase in a Swift library is a lint error and will generate warnings for callers on newer toolchains.


Generated by Claude Code

Comment thread README.md Outdated
```

Unlike repurposing an unrelated base quantity such as `Amount`, a dedicated dimension can't
silently cancel against unrelated units. Use a unique `rawValue` for each distinct dimension.

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rawValue collision risk deserves a stronger warning.

The current guidance ("Use a unique rawValue for each distinct dimension") is easy to overlook. The practical risk is real: if two independently-authored extensions both write Quantity(rawValue: "Energy"), their units will silently cancel against each other — which is exactly the bug this change is meant to prevent when repurposing Amount.

Consider recommending a reverse-DNS prefix convention (the same pattern used for Notification.Name in large codebases):

extension Quantity {
    // Prefix with a domain to avoid collisions with other modules.
    static let money = Quantity(rawValue: "com.myapp.Money")
}

Or at minimum call out the collision consequence explicitly: "Two dimensions with the same rawValue are treated as identical and their units will cancel against each other."


Generated by Claude Code

Comment thread Tests/UnitsTests/DefinitionTests.swift Outdated
Comment on lines +265 to +275
try XCTAssertEqual(Measurement("1°C"), (273.15.measured(in: .kelvin) + 1.measured(in: .kelvin)).convert(to: .celsius))
try XCTAssertEqual(XCTUnwrap(Measurement("1°F")), try ((273.15 - (32 * 5.0 / 9.0)).measured(in: .kelvin) + (5.0 / 9.0).measured(in: .kelvin)).convert(to: .fahrenheit), accuracy: 0.0001)
// Broken into sub-expressions with explicit types: as a single expression
// this overflows the type-checker's per-expression budget on some toolchains.
let fahrenheitOffset: Double = 273.15 - (32 * 5.0 / 9.0)
let fahrenheitScale: Double = 5.0 / 9.0
let oneFahrenheit = try fahrenheitOffset.measured(in: .kelvin) + fahrenheitScale.measured(in: .kelvin)
try XCTAssertEqual(
XCTUnwrap(Measurement("1°F")),
oneFahrenheit.convert(to: .fahrenheit),
accuracy: 0.0001
)

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unrelated change bundled here.

This Fahrenheit test refactor (splitting a complex expression to avoid type-checker timeouts) is unrelated to the Quantity extensibility work. It makes this PR harder to bisect and obscures the Quantity-only change in review.

If this is needed to fix a real toolchain issue, it should either be in a separate commit with a clear message, or squashed with a note in the PR description explaining why it's included. Currently the PR description doesn't mention it at all.


Generated by Claude Code

@DarkAndHungryGod DarkAndHungryGod force-pushed the feat/extensible-quantity branch from 24da654 to 22ddb1f Compare June 28, 2026 10:48
rstacpoole and others added 3 commits June 30, 2026 13:07
Replaces the closed `Quantity` enum with a `RawRepresentable` struct that
exposes the same built-in dimensions as static constants, resolving the
long-standing TODO about extensibility.

Callers can now define their own dimensions (e.g. a `Money` dimension for
cost calculations) with a static extension, without forking the package or
repurposing an unrelated base quantity like `Amount`:

    extension Quantity { static let money = Quantity(rawValue: "Money") }

All existing call sites are source-compatible: `.Length`-style literals,
`[Quantity: Int]` dictionary keys, and the `\\Quantity.rawValue` keypath all
continue to work. The one behavioral change is that the synthesized failable
`init?(rawValue:)` becomes a non-failable `init(rawValue:)`, since any string
is now a valid dimension.

Adds QuantityTests covering custom-dimension identity, arithmetic
cancellation, and dimension description. Documents the feature under a new
'Custom Dimensions' README section.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Addresses review feedback on the enum -> struct change:

- Rename the static dimension properties to lowerCamelCase (.length, .mass,
  ...) per the Swift API Design Guidelines. The former PascalCase names are
  kept as @available(*, deprecated, renamed:) aliases so existing call sites
  keep compiling; they can be removed in a future major release. Internal
  call sites and tests now use the new names.
- Add CustomStringConvertible returning rawValue, restoring the enum's
  behavior where "\(Quantity.length)" prints "Length" rather than the
  synthesized struct description.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- Update the Custom Units / Custom Dimensions examples to the new
  lowerCamelCase dimension properties (.length, .amount) so the docs
  don't demonstrate the now-deprecated PascalCase names.
- Spell out in the Custom Dimensions section that a Quantity is identified
  solely by its raw string across all linked modules, so two modules using
  the same raw value silently collapse into one dimension. Recommend
  prefixing custom raw values (e.g. "Acme.Money") to avoid collisions.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@DarkAndHungryGod DarkAndHungryGod force-pushed the feat/extensible-quantity branch from 22ddb1f to fe4ba2b Compare June 30, 2026 03:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants