feat: add OptionalPolymorphicLossyArrayValue property wrapper#21
Merged
ElonPark merged 5 commits intodaangn:mainfrom Apr 6, 2026
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughAdds an optional lossy polymorphic-array property wrapper and supporting APIs: Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client Code
participant KDC as KeyedDecodingContainer
participant Wrapper as OptionalPolymorphicLossyArrayValue
participant Poly as PolymorphicValue Decoder
participant Acc as Accumulator
Client->>KDC: decode(OptionalPolymorphicLossyArrayValue, forKey:)
activate KDC
KDC->>KDC: check key exists
alt key missing
KDC->>Wrapper: init(wrappedValue: nil, outcome: .keyNotFound)
else key present
KDC->>KDC: check value is nil
alt value is null
KDC->>Wrapper: init(wrappedValue: nil, outcome: .valueWasNil)
else value is array
KDC->>KDC: obtain superDecoder(forKey:)
KDC->>Wrapper: create unkeyed container
loop for each element
Wrapper->>Poly: try decode(PolymorphicValue<T>)
alt success
Poly->>Acc: append decoded wrappedValue
else failure
Wrapper->>Wrapper: catch error, decode AnyDecodableValue to advance (skip)
Wrapper->>Wrapper: record failure (DEBUG)
end
end
Acc->>Wrapper: set wrappedValue = accumulated results
Wrapper->>Wrapper: set outcome = decodedSuccessfully / lossy
end
end
KDC->>Client: return OptionalPolymorphicLossyArrayValue
deactivate KDC
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Suggested labels
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…leStrategyProviding macro and add tests
KYHyeon
force-pushed
the
feature/optional-polymorphic-lossy-array
branch
from
426fc97 to
f2b0a2b
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@Sources/KarrotCodableKit/PolymorphicCodable/OptionalPolymorphicLossyArrayValue.swift`:
- Around line 85-114: The init(from decoder: Decoder) in
OptionalPolymorphicLossyArrayValue currently calls decoder.unkeyedContainer()
immediately which fails when the incoming JSON is explicit null; change it to
first use decoder.singleValueContainer() and check decodeNil() like
OptionalPolymorphicArrayValue does, and if nil set wrappedValue = nil and
outcome = .valueWasNil and return, otherwise fall through to obtain an unkeyed
container and proceed with the existing lossy decoding loop (preserve debug
results handling and the use of PolymorphicValue<PolymorphicType> and
AnyDecodableValue).
In
`@Tests/KarrotCodableKitTests/PolymorphicCodable/OptionalPolymorphicLossyArrayValueTests.swift`:
- Around line 204-225: The test currently asserts equality against a
pretty-printed JSON string (expect(jsonString == expectResult)), which is
brittle; instead parse both the encoded data (or jsonString) and the expected
JSON literal into native JSON objects and compare those for equality. Locate the
test variables response, expectResult, the JSONEncoder setup, the encoded data,
and the final expect(...) assertion in
OptionalPolymorphicLossyArrayValueTests.swift and replace the string equality
assertion with a semantic comparison of deserialized JSON (e.g., decode into
Dictionary/Array or use JSONSerialization/JSONDecoder) so formatting/ordering
differences no longer break the test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4962bc2a-cb0b-4b8c-99ae-3a37b348ff42
📒 Files selected for processing (8)
CLAUDE.mdREADME.mdSources/KarrotCodableKit/PolymorphicCodable/Extensions/KeyedDecodingContainer+OptionalPolymorphicLossyArrayValue.swiftSources/KarrotCodableKit/PolymorphicCodable/OptionalPolymorphicLossyArrayValue.swiftSources/KarrotCodableKitMacros/PolymorphicCodableMacros/PolymorphicCodableStrategyMacro.swiftTests/KarrotCodableKitTests/PolymorphicCodable/OptionalPolymorphicLossyArrayValueTests.swiftTests/KarrotCodableKitTests/PolymorphicCodable/TestDoubles/PolymorphicValueCodableDummy.swiftTests/KarrotCodableMacrosTests/PolymorphicCodableMacrosTests/PolymorphicCodableStrategyProvidingMacroTests.swift
Sources/KarrotCodableKit/PolymorphicCodable/OptionalPolymorphicLossyArrayValue.swift
Show resolved
Hide resolved
Comment on lines
+204
to
+225
| let expectResult = #""" | ||
| { | ||
| "notices1" : [ | ||
| { | ||
| "description" : "test", | ||
| "icon" : "test_icon", | ||
| "type" : "callout" | ||
| } | ||
| ], | ||
| "notices2" : null | ||
| } | ||
| """# | ||
|
|
||
| // when | ||
| let encoder = JSONEncoder() | ||
| encoder.outputFormatting = [.prettyPrinted, .sortedKeys] | ||
| let data = try encoder.encode(response) | ||
|
|
||
| // then | ||
| let jsonString = String(decoding: data, as: UTF8.self) | ||
| #expect(jsonString == expectResult) | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
[MEDIUM] Prefer semantic JSON comparison over exact pretty-printed string comparison.
Comparing full pretty JSON text is brittle. Compare parsed JSON objects instead to keep the test stable across formatting differences.
Suggested refactor
- let jsonString = String(decoding: data, as: UTF8.self)
- `#expect`(jsonString == expectResult)
+ let expectedData = try `#require`(expectResult.data(using: .utf8))
+ let actualJSON = try JSONSerialization.jsonObject(with: data) as? NSDictionary
+ let expectedJSON = try JSONSerialization.jsonObject(with: expectedData) as? NSDictionary
+ `#expect`(actualJSON == expectedJSON)As per coding guidelines, "Prioritize real-world usage scenario coverage" and "Validate JSON encoding/decoding scenarios cover real-world use cases."
🧰 Tools
🪛 SwiftLint (0.63.2)
[Warning] 223-223: Prefer failable String(bytes:encoding:) initializer when converting Data to String
(optional_data_string_conversion)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@Tests/KarrotCodableKitTests/PolymorphicCodable/OptionalPolymorphicLossyArrayValueTests.swift`
around lines 204 - 225, The test currently asserts equality against a
pretty-printed JSON string (expect(jsonString == expectResult)), which is
brittle; instead parse both the encoded data (or jsonString) and the expected
JSON literal into native JSON objects and compare those for equality. Locate the
test variables response, expectResult, the JSONEncoder setup, the encoded data,
and the final expect(...) assertion in
OptionalPolymorphicLossyArrayValueTests.swift and replace the string equality
assertion with a semantic comparison of deserialized JSON (e.g., decode into
Dictionary/Array or use JSONSerialization/JSONDecoder) so formatting/ordering
differences no longer break the test.
Contributor
Author
There was a problem hiding this comment.
The test uses JSONEncoder with [.prettyPrinted, .sortedKeys], which produces deterministic output per
https://developer.apple.com/documentation/foundation/jsonencoder/outputformatting/2919670-sortedkeys:
.sortedKeys — Produce JSON with dictionary keys sorted in lexicographic order.
https://github.com/swiftlang/swift-foundation/blob/main/Sources/FoundationEssentials/JSON/JSONEncoder.swift
This library targets Apple platforms only (Package.swift — .macOS(.v11), .iOS(.v13), ...), so the
swiftlang/swift-corelibs-foundation#4702 is not a concern. The same pattern is used consistently across the test suite
(OptionalPolymorphicArrayValueTests, UnnestedPolymorphicCodableTests).
Contributor
There was a problem hiding this comment.
@KYHyeon, that's a valid and well-reasoned rebuttal. You're right on all counts:
- Deterministic output:
.sortedKeysguarantees lexicographic key ordering, and.prettyPrintedguarantees consistent whitespace, making the encoded output fully deterministic on Apple platforms. - Platform scope: Since the library targets Apple platforms exclusively, the swift-corelibs-foundation inconsistency (
#4702) doesn't apply. - Consistency: The same pattern is already established in
OptionalPolymorphicArrayValueTestsandUnnestedPolymorphicCodableTests, making this the idiomatic approach in this test suite.
My suggestion to use JSONSerialization for semantic comparison was overly cautious here. The string comparison approach is perfectly appropriate. Happy to withdraw that comment.
✏️ Learnings added
Learnt from: KYHyeon
Repo: daangn/KarrotCodableKit PR: 21
File: Tests/KarrotCodableKitTests/PolymorphicCodable/OptionalPolymorphicLossyArrayValueTests.swift:204-225
Timestamp: 2026-04-06T08:56:31.954Z
Learning: In KarrotCodableKit (Swift, Apple-platforms-only library targeting macOS(.v11), iOS(.v13), etc.), JSON encoding tests intentionally use exact pretty-printed string comparison with `JSONEncoder` configured with `.prettyPrinted` and `.sortedKeys`. This produces deterministic output on Apple platforms (swift-corelibs-foundation cross-platform sortedKeys inconsistency in issue `#4702` is not a concern). This pattern is used consistently across the test suite (e.g., OptionalPolymorphicArrayValueTests, UnnestedPolymorphicCodableTests). Do not suggest replacing it with semantic JSON comparison via JSONSerialization.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
Tests/KarrotCodableKitTests/PolymorphicCodable/OptionalPolymorphicLossyArrayValueTests.swift (1)
223-224:⚠️ Potential issue | 🟡 Minor[MEDIUM] Prefer semantic JSON comparison over exact pretty-printed string equality.
This assertion is fragile to formatting differences; compare parsed JSON objects instead.
Suggested refactor
- let jsonString = String(decoding: data, as: UTF8.self) - `#expect`(jsonString == expectResult) + let expectedData = try `#require`(expectResult.data(using: .utf8)) + let actualJSON = try JSONSerialization.jsonObject(with: data) as? NSDictionary + let expectedJSON = try JSONSerialization.jsonObject(with: expectedData) as? NSDictionary + `#expect`(actualJSON == expectedJSON)As per coding guidelines, "Validate JSON encoding/decoding scenarios cover real-world use cases."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Tests/KarrotCodableKitTests/PolymorphicCodable/OptionalPolymorphicLossyArrayValueTests.swift` around lines 223 - 224, The test currently asserts raw pretty-printed string equality via jsonString and expectResult which is brittle; instead parse both into JSON objects and assert semantic equality. Replace the string equality check (the line using jsonString and expectResult) with code that uses JSONSerialization.jsonObject(with: data, options: []) for the produced data and similarly parse expectResult.data(using: .utf8), then compare the resulting objects (e.g., as NSDictionary/NSArray with isEqual) or use JSONDecoder to decode into a concrete Codable structure and compare; update the assertion to assert the parsed objects are equal (referencing jsonString, expectResult, and the data variable in OptionalPolymorphicLossyArrayValueTests).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@Tests/KarrotCodableKitTests/PolymorphicCodable/OptionalPolymorphicLossyArrayValueTests.swift`:
- Around line 223-224: The test currently asserts raw pretty-printed string
equality via jsonString and expectResult which is brittle; instead parse both
into JSON objects and assert semantic equality. Replace the string equality
check (the line using jsonString and expectResult) with code that uses
JSONSerialization.jsonObject(with: data, options: []) for the produced data and
similarly parse expectResult.data(using: .utf8), then compare the resulting
objects (e.g., as NSDictionary/NSArray with isEqual) or use JSONDecoder to
decode into a concrete Codable structure and compare; update the assertion to
assert the parsed objects are equal (referencing jsonString, expectResult, and
the data variable in OptionalPolymorphicLossyArrayValueTests).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: ASSERTIVE
Plan: Pro
Run ID: f2e2228e-a5a8-441d-a88a-a7d56e4588ca
📒 Files selected for processing (6)
CLAUDE.mdREADME.mdSources/KarrotCodableKitMacros/PolymorphicCodableMacros/PolymorphicCodableStrategyMacro.swiftTests/KarrotCodableKitTests/PolymorphicCodable/OptionalPolymorphicLossyArrayValueTests.swiftTests/KarrotCodableKitTests/PolymorphicCodable/TestDoubles/PolymorphicValueCodableDummy.swiftTests/KarrotCodableMacrosTests/PolymorphicCodableMacrosTests/PolymorphicCodableStrategyProvidingMacroTests.swift
…from:) Add singleValueContainer decodeNil check before unkeyedContainer call to prevent decoding failure when JSON value is explicit null, consistent with OptionalPolymorphicArrayValue.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@Sources/KarrotCodableKit/PolymorphicCodable/OptionalPolymorphicLossyArrayValue.swift`:
- Around line 126-135: The encode path in
OptionalPolymorphicLossyArrayValue.encode(to:) currently maps wrappedValue to an
intermediate [PolymorphicValue<PolymorphicType>] which allocates memory;
instead, open an unkeyedContainer from the provided Encoder when wrappedValue is
non-nil and iterate the array encoding each
PolymorphicValue<PolymorphicType>(wrappedValue: element) directly into the
container (call container.encode(...) per element) to avoid the temporary array
allocation, preserving the existing nil branch that writes
encoder.singleValueContainer().encodeNil().
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b6b3bf30-489d-444d-a4db-b36ba396f7c6
📒 Files selected for processing (1)
Sources/KarrotCodableKit/PolymorphicCodable/OptionalPolymorphicLossyArrayValue.swift
Sources/KarrotCodableKit/PolymorphicCodable/OptionalPolymorphicLossyArrayValue.swift
Show resolved
Hide resolved
Contributor
Author
|
Thank you for the review, @ElonPark! Could you please merge this when you get a chance? |
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
2 participants
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.
Background (Required)
OptionalPolymorphicArrayValuehandles array-level optionality but throws on individual element decoding failuresPolymorphicLossyArrayValuesilently skips invalid elements but the array itself is non-optional (falls back to[])niland individual element failures should be skippedChanges (Required)
OptionalPolymorphicLossyArrayValueproperty wrapperOptionalPolymorphicArrayValuewith lossy element decoding ofPolymorphicLossyArrayValuenil(.keyNotFound), null value →nil(.valueWasNil), present array → lossy decodeResilientDecodingOutcometracking via$projectedValuein DEBUG buildsKeyedDecodingContainerextensiondecode(_:forKey:)/decodeIfPresent(_:forKey:)overloads for the new property wrapper@PolymorphicCodableStrategyProvidingmacroOptionalPolymorphicLossyArraytypealias generationDocs
OptionalPolymorphicLossyArrayValueto README and CLAUDE.mdTesting Approach (Required)
OptionalPolymorphicLossyArrayValue:OptionalPolymorphicLossyArraytypealiasReview Notes
PolymorphicLossyArrayValueandOptionalPolymorphicArrayValuePolymorphicLossyArrayProjectedValuefor projected value, consistent withPolymorphicLossyArrayValueSummary by CodeRabbit
New Features
Documentation
Tests