Fix: Improve error handling for non-optional property wrappers #14
Conversation
- Added given/when/then comments to approximately 33 test files - Updated about 150-200 test functions for better readability - Followed BDD (Behavior-Driven Development) test structure - Ensured consistency across all test files in the project - All tests passing successfully in both debug and release modes
…aultCodable properties When decoding non-optional properties wrapped with DefaultCodable, missing keys and nil values are now properly reported as recoverable errors rather than silently falling back to default values. This provides better error tracking while maintaining backward compatibility through the default value fallback mechanism.
…al property LossyArray now properly reports an error when decoding encounters a nil value for non-optional properties, providing better error tracking while still falling back to an empty array.
…yDictionary LossyDictionary now properly reports errors when encountering missing keys or nil values for non-optional properties, providing better error tracking while still falling back to an empty dictionary.
…rrayValue DefaultEmptyPolymorphicArrayValue now properly reports errors when encountering missing keys or nil values for non-optional properties, providing better error tracking while maintaining the default empty array fallback behavior.
PolymorphicLossyArrayValue now properly reports errors when encountering missing keys or nil values for non-optional properties, providing better error tracking while maintaining the lossy array behavior.
📝 WalkthroughWalkthroughDecoding error reporting was consolidated to DEBUG builds across DefaultCodable, Lossy/Lossless containers, Polymorphic codable paths, and several value types: DEBUG builds now construct contextual DecodingError and report via nested decoders before recovering to defaults; non-DEBUG builds fall back silently. A Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant KDC as KeyedDecodingContainer
participant Dec as Decoder
participant Strat as Strategy/Type
participant Result as Wrapped Value
Caller->>KDC: decode(non-optional property, forKey:)
alt Key present & non-nil
KDC->>Strat: decode value
Strat-->>KDC: value
KDC-->>Caller: Result(wrapped: value, outcome: .decodedSuccessfully)
else Key missing (DEBUG)
KDC->>Dec: build DecodingError.keyNotFound + context
KDC->>Dec: reportError(error)
KDC-->>Caller: Result(wrapped: default, outcome: .recoveredFrom(error, wasReported: true))
else Key missing (non-DEBUG)
KDC-->>Caller: Result(wrapped: default, outcome: .recoveredFrom(error, wasReported: false))
else Value nil (DEBUG)
KDC->>Dec: build DecodingError.valueNotFound + context
KDC->>Dec: reportError(error)
KDC-->>Caller: Result(wrapped: default, outcome: .recoveredFrom(error, wasReported: true))
else Value nil (non-DEBUG)
KDC-->>Caller: Result(wrapped: default, outcome: .recoveredFrom(error, wasReported: false))
end
sequenceDiagram
autonumber
actor Caller
participant KDC as KeyedDecodingContainer
participant Dec as Decoder
participant RR as RawRepresentable.Type
Caller->>KDC: decode(RawRepresentable, forKey:)
KDC->>RR: decode RawValue
alt RawValue maps to case
RR-->>KDC: enum case
KDC-->>Caller: wrapped case (.decodedSuccessfully)
else Unknown raw value (DEBUG)
KDC->>Dec: createUnknownRawValueError (/dataCorrupted if frozen)
KDC->>Dec: reportError(error)
KDC-->>Caller: default (.recoveredFrom(error, wasReported: true))
else Unknown raw value (non-DEBUG)
KDC-->>Caller: default (no report, wasReported: false)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested labels
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 72
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (34)
Tests/KarrotCodableKitTests/BetterCodable/LossyValue/LossyOptionalTests.swift (1)
12-12: Align test class name with file and fix a tiny doc typo
- Rename the class to match the file’s subject for discoverability.
- Grammar: “it often the case” → “it is often the case”.
[LOW]
-final class DefaultNilTests: XCTestCase { +final class LossyOptionalTests: XCTestCase {- /// optional types, it often the case that we end up with an error instead of + /// optional types, it is often the case that we end up with an error instead ofAlso applies to: 13-15
Tests/KarrotCodableMacrosTests/PolymorphicCodableMacrosTests/PolymorphicDeodableMacroTests.swift (3)
106-112: Tighten diagnostic anchoring to the offending argumentIf the macro reports the error at the string literal for
identifier: "", consider pinningcolumnto that literal’s start for stronger checks. Otherwise, keep it at column 1 for stability across formatting.
[MEDIUM]
27-72: Broaden macro-expansion coverage for PolymorphicDecodableConsider adding tests for:
- Non-literal identifier (e.g., a variable or interpolation) → expect a dedicated diagnostic
- Missing
identifier:argument → expect “required argument” diagnostic- Additional
codingKeyStylecases (kebab/camel/pascal) with mixed-property inputs- Duplicate attribute usage on the same type → expect diagnostic
- Access control permutations (internal/public type & members) to ensure generated API modifiers are correct
[MEDIUM]Happy to draft these tests.
2-2: Typo: file name “Deodable” → “Decodable”Please rename the file to
PolymorphicDecodableMacroTests.swiftto match the class name and avoid search/grep misses.
[LOW]Tests/KarrotCodableKitTests/BetterCodable/RawRepresentableTests.swift (1)
16-33: Add missing/null key tests for non-optional DefaultCodable [HIGH]
· RawRepresentableTests.swift only exercises the “unknown raw value → default” path; add tests assertingJSONDecoder().decodethrows when the key is absent and when it’snull.
· In DEBUG builds, leverageerrorReporter.flushReportedErrors()/ResilientDecodingOutcome.keyNotFound(see Defaults/DefaultCodableResilientTests.swift) to verify no fallback occurs and errors are reported correctly.Tests/KarrotCodableKitTests/BetterCodable/LosslessValue/LosslessValueTests.swift (1)
82-97: [LOW] — Same Data initializer nit, repeated in multiple blocks.- let jsonData = #"{ "bool": 1, "string": "42", "int": 7, "double": 7.1 }"#.data(using: .utf8)! + let jsonData = Data(#"{ "bool": 1, "string": "42", "int": 7, "double": 7.1 }"#.utf8)Tests/KarrotCodableKitTests/AnyCodable/AnyDecodableTests.swift (2)
13-30: [LOW] — Prefer non-optional Data initializer.- """.data(using: .utf8)! + """.data(using: .utf8)! // consider: Data(#"...json..."#.utf8)Even better:
- let json = """ + let json = Data(""" { ... - """.data(using: .utf8)! + """.utf8)
35-42: [MEDIUM] — Avoid force-casts in tests to fail gracefully.Use typed extraction helpers or conditional casts with explicit failures to keep assertion failures readable.
- XCTAssertEqual(dictionary["boolean"]?.value as! Bool, true) + XCTAssertEqual(dictionary["boolean"]?.value as? Bool, true)Or:
if let b = dictionary["boolean"]?.value as? Bool { XCTAssertTrue(b) } else { XCTFail("Expected Bool for key 'boolean'") }Tests/KarrotCodableMacrosTests/PolymorphicEnumCodableMacroTests/PolymorphicEnumCodableMacroTests.swift (1)
382-387: Fix diagnostic expectation: unclosed backtick in message. [HIGH]The expected message is missing a closing backtick, which can make the test brittle and hard to read.
- message: "Missing fallback case: should be defined as `case undefinedCallout", + message: "Missing fallback case: should be defined as `case undefinedCallout`",Tests/KarrotCodableMacrosTests/CustomCodableMacros/CustomEncodableMacroTests.swift (2)
51-52: Optional: add // then marker here too. [LOW]Keeps phase annotations consistent across the suite.
- macros: testMacros, + // then + macros: testMacros,
300-310: Remove duplicate DiagnosticSpec
The test asserts the same diagnostic twice; drop the second entry to prevent masking regressions.
File:Tests/KarrotCodableMacrosTests/CustomCodableMacros/CustomEncodableMacroTests.swift(lines ~300–310)diagnostics: [ DiagnosticSpec( message: "`@CustomCodable`, `@CustomEncodable`, `@CustomDecodable` cannot be applied to enum", line: 1, column: 1 ), - DiagnosticSpec( - message: "`@CustomCodable`, `@CustomEncodable`, `@CustomDecodable` cannot be applied to enum", - line: 1, - column: 1 - ), ],Tests/KarrotCodableMacrosTests/CustomCodableMacros/CustomDecodableMacroTests.swift (2)
51-52: Optional: add // then marker for phase completeness. [LOW]- macros: testMacros, + // then + macros: testMacros,
300-310: Remove duplicate DiagnosticSpec entry
Tests/KarrotCodableMacrosTests/CustomCodableMacros/CustomDecodableMacroTests.swift – the same DiagnosticSpec appears twice in thediagnosticsarray; remove the extra occurrence.Tests/KarrotCodableMacrosTests/UnnestedPolymorphicCodableMacroTests/UnnestedPolymorphicDecodableMacroTests.swift (4)
2-3: Header nits: filename and module name. [LOW]
- The header shows “UnnestedPolymorphicDecodableMacroTests 2.swift” and “KarrotDecodableKit”. Consider correcting to match the actual filename and module (KarrotCodableKit).
-// UnnestedPolymorphicDecodableMacroTests 2.swift -// KarrotDecodableKit +// UnnestedPolymorphicDecodableMacroTests.swift +// KarrotCodableKit
284-294: Duplicate diagnostics for enum application error. [HIGH]Two identical DiagnosticSpec entries are asserted. Keep one unless the macro truly emits duplicates.
diagnostics: [ DiagnosticSpec( message: "`@UnnestedPolymorphicDecodable` cannot be applied to enum types. Use `@PolymorphicEnumDecodable` instead.", line: 1, column: 1 ), - DiagnosticSpec( - message: "`@UnnestedPolymorphicDecodable` cannot be applied to enum types. Use `@PolymorphicEnumDecodable` instead.", - line: 1, - column: 1 - ), ],
323-334: Duplicate diagnostics for empty identifier. [HIGH]De-duplicate the identical DiagnosticSpec.
diagnostics: [ DiagnosticSpec( message: "Invalid polymorphic identifier: expected a non-empty string.", line: 1, column: 1 ), - DiagnosticSpec( - message: "Invalid polymorphic identifier: expected a non-empty string.", - line: 1, - column: 1 - ), ],
429-439: Duplicate diagnostics for empty nested key. [HIGH]Remove the repeated DiagnosticSpec.
diagnostics: [ DiagnosticSpec( message: "Invalid nested key: expected a non-empty string.", line: 1, column: 1 ), - DiagnosticSpec( - message: "Invalid nested key: expected a non-empty string.", - line: 1, - column: 1 - ), ],Sources/KarrotCodableKit/BetterCodable/LossyValue/LossyArray.swift (1)
71-79: [LOW] Avoid unnecessary SingleValueContainer creation per elementDecoding via T(from:) avoids creating a SingleValueDecodingContainer for each element and works for both primitive and keyed T.
- let value = try elementDecoder.singleValueContainer().decode(T.self) + let value = try T(from: elementDecoder)Tests/KarrotCodableMacrosTests/PolymorphicEnumCodableMacroTests/PolymorphicEnumDecodableMacroTests.swift (5)
78-88: LGTM on default-parameter path coverage. [MEDIUM]Covers the no-argument attribute well. Consider adding a case with nested enums to ensure macro lookup works in nested scopes.
Also applies to: 89-121
164-174: Invalid identifier value coverage is solid. [MEDIUM]Nice check for empty key. Add a test for a non-literal expression (e.g., identifierCodingKey: someVar) to verify diagnostic on non-string-literal arguments.
Also applies to: 175-191
273-284: Fallback-case happy path looks good. [HIGH]Expansion routes unknown types to the fallback; readable switch ordering. Consider an additional test where the fallback case appears first to ensure order doesn’t matter.
Also applies to: 285-320
328-339: Fix diagnostic string: missing closing backtick (and punctuation). [HIGH]The expected message is missing a closing backtick, which will make the test brittle against the actual diagnostic.
Apply:
- message: "Missing fallback case: should be defined as `case undefinedCallout", + message: "Missing fallback case: should be defined as `case undefinedCallout`.",Also applies to: 347-355
363-374: Empty fallbackCaseName validation looks correct. [MEDIUM]Covers the empty string case well. Consider a test for invalid Swift identifiers (e.g., contains whitespace or symbols).
Also applies to: 381-390
Tests/KarrotCodableKitTests/AnyCodable/AnyCodableTests.swift (1)
22-51: Nice BDD phase markers; suggest minor test hygiene. [LOW]
- Keep the Given/When/Then—reads better.
- Optional: use non-optional Data initializers to silence lint and remove “!”.
Apply representative changes:
- let json = """ + let json = Data(""" { "boolean": true, "integer": 42, "double": 3.141592653589793, "string": "string", "array": [1, 2, 3], "nested": { "a": "alpha", "b": "bravo", "c": "charlie" }, "null": null } - """.data(using: .utf8)! + """.utf8)- let expected = """ + let expected = Data(""" { "boolean": true, "integer": 42, "double": 3.141592653589793, "string": "string", "stringInterpolation": "string 1234", "array": [1, 2, 3], "nested": { "a": "alpha", "b": "bravo", "c": "charlie" }, "someCodable": { "string":"String", "int":100, "bool": true, "has_underscore":"another string" }, "null": null } - """.data(using: .utf8)! + """.utf8)Also applies to: 54-84, 87-142
Sources/KarrotCodableKit/BetterCodable/LossyValue/LossyDictionary.swift (3)
285-296: Do not throw from superDecoder(forKey:) on missing keys; use try? and differentiate null vs missing. [HIGH]
- superDecoder(forKey:) throws when the key is absent—current code leaks the throw and breaks “recover with default”.
- Also emit valueNotFound for present-but-null, not keyNotFound.
Apply:
- let context = DecodingError.Context( - codingPath: codingPath + [key], - debugDescription: "Key not found but property is non-optional" - ) - let error = DecodingError.keyNotFound(key, context) - let decoder = try superDecoder(forKey: key) - decoder.reportError(error) - return LossyDictionary(wrappedValue: [:], outcome: .recoveredFrom(error, wasReported: true)) + if contains(key) && ((try? decodeNil(forKey: key)) == true) { + let context = DecodingError.Context( + codingPath: codingPath + [key], + debugDescription: "Value was nil but property is non-optional" + ) + let error = DecodingError.valueNotFound([DictKey: DictValue].self, context) + let decoder = try? superDecoder(forKey: key) + decoder?.reportError(error) + return LossyDictionary(wrappedValue: [:], outcome: .recoveredFrom(error, wasReported: true)) + } else { + let context = DecodingError.Context( + codingPath: codingPath + [key], + debugDescription: "Key not found but property is non-optional" + ) + let error = DecodingError.keyNotFound(key, context) + // superDecoder(forKey:) throws if the key is missing; use try? to avoid propagating. + let decoder = try? superDecoder(forKey: key) + decoder?.reportError(error) + return LossyDictionary(wrappedValue: [:], outcome: .recoveredFrom(error, wasReported: true)) + }
55-61: Nit: generic parameter typo “DecodablValue”. [LOW]Rename for clarity.
- private struct LossyDecodableValue<DecodablValue: Decodable>: Decodable { - let value: DecodablValue + private struct LossyDecodableValue<DecodableValue: Decodable>: Decodable { + let value: DecodableValue @@ - self.value = try container.decode(DecodablValue.self) + self.value = try container.decode(DecodableValue.self)
150-157: Error reporting path looks consistent; minor robustness tweaks optional. [MEDIUM]Optional: if AnyDecodableValue decoding can fail for non-object tokens, wrap it in a dedicated “sink” decoder that accepts any token to avoid extra noise.
Also applies to: 176-183, 185-196, 231-239
Tests/KarrotCodableKitTests/BetterCodable/Defaults/DefaultFalseTests.swift (1)
103-107: Typo in assertion messageUse “fall into” (two words).
Priority: [LOW]
- "Should fall in to the else block and return default value" + "Should fall into the else block and return default value"Tests/KarrotCodableKitTests/BetterCodable/Defaults/DefaultTrueTests.swift (1)
88-92: Typo in assertion messageUse “fall into”.
Priority: [LOW]
- "Should fall in to the else block and return default value" + "Should fall into the else block and return default value"Tests/KarrotCodableKitTests/BetterCodable/LossyValue/LossyDictionaryResilientTests.swift (1)
127-132: DEBUG null handling: good; assert error kind for stronger guarantees.
The expectation flip to!= nilis correct for non-optional dictionaries. To lock behavior, also assert the error isvalueNotFoundforobjectDict = null.
Priority: [MEDIUM]#expect(fixture.$stringDict.error != nil) #expect(fixture.$intDict.error != nil) - #expect(fixture.$objectDict.error != nil) // null value should error for non-optional property + #expect(fixture.$objectDict.error != nil) // null value should error for non-optional property + if let err = fixture.$objectDict.error as? DecodingError { + if case .valueNotFound = err { + // expected + } else { + Issue.record("Expected valueNotFound for null objectDict") + } + }Tests/KarrotCodableKitTests/BetterCodable/LossyValue/LossyDictionaryTests.swift (3)
19-41: Avoid force unwraps of JSON data in tests.
Use XCTUnwrap/guard to fail gracefully instead of crashing on bad UTF-8.
Priority: [LOW]- let jsonData = """ + let json = """ { "stringToInt": { "one": 1, "two": 2, "three": null }, "intToString": { "1": "one", "2": "two", "3": null } } - """.data(using: .utf8)! + """ + guard let jsonData = json.data(using: .utf8) else { + XCTFail("UTF-8 encoding failed") + return + }
72-95: Misnamed test: it’s a dictionary, not an array.
Rename to prevent confusion in reports.
Priority: [LOW]- func testEncodingDecodedLosslessArrayRetainsContents() throws { + func testEncodingDecodedLosslessDictionaryRetainsContents() throws {
97-119: Key strategies don’t affect dictionary keys here—remove to avoid confusion.
keyEncodingStrategy/keyDecodingStrategyapply to CodingKeys, not dictionary keys; they’re unnecessary for this assertion.
Priority: [LOW]let decoder = JSONDecoder() - decoder.keyDecodingStrategy = .convertFromSnakeCase let encoder = JSONEncoder() - encoder.keyEncodingStrategy = .convertToSnakeCaseSources/KarrotCodableKit/PolymorphicCodable/Extensions/KeyedDecodingContainer+PolymorphicLossyArrayValue.swift (1)
86-101: Align non-DEBUG null outcome indecodeIfPresentforPolymorphicLossyArrayValue
Change the#elsebranch inKeyedDecodingContainer+PolymorphicLossyArrayValue.swiftto:- return PolymorphicLossyArrayValue(wrappedValue: []) + return PolymorphicLossyArrayValue(wrappedValue: [], outcome: .valueWasNil)to mirror
DefaultEmptyPolymorphicArrayValueand ensure downstream telemetry remains consistent.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (41)
Sources/KarrotCodableKit/BetterCodable/Defaults/DefaultCodable.swift(7 hunks)Sources/KarrotCodableKit/BetterCodable/LossyValue/LossyArray.swift(1 hunks)Sources/KarrotCodableKit/BetterCodable/LossyValue/LossyDictionary.swift(2 hunks)Sources/KarrotCodableKit/PolymorphicCodable/Extensions/KeyedDecodingContainer+DefaultEmptyPolymorphicArrayValue.swift(3 hunks)Sources/KarrotCodableKit/PolymorphicCodable/Extensions/KeyedDecodingContainer+PolymorphicLossyArrayValue.swift(4 hunks)Tests/KarrotCodableKitTests/AnyCodable/AnyCodableTests.swift(6 hunks)Tests/KarrotCodableKitTests/AnyCodable/AnyDecodableTests.swift(2 hunks)Tests/KarrotCodableKitTests/AnyCodable/AnyEncodableTests.swift(5 hunks)Tests/KarrotCodableKitTests/BetterCodable/DataValue/DataValueTests.swift(1 hunks)Tests/KarrotCodableKitTests/BetterCodable/Defaults/DefaulEmptyDictionaryTests.swift(1 hunks)Tests/KarrotCodableKitTests/BetterCodable/Defaults/DefaultCodableResilientTests.swift(4 hunks)Tests/KarrotCodableKitTests/BetterCodable/Defaults/DefaultEmptyArrayTests.swift(1 hunks)Tests/KarrotCodableKitTests/BetterCodable/Defaults/DefaultEmptyStringTests.swift(1 hunks)Tests/KarrotCodableKitTests/BetterCodable/Defaults/DefaultFalseTests.swift(1 hunks)Tests/KarrotCodableKitTests/BetterCodable/Defaults/DefaultTrueTests.swift(2 hunks)Tests/KarrotCodableKitTests/BetterCodable/Defaults/DefaultZeroDoubleTests.swift(1 hunks)Tests/KarrotCodableKitTests/BetterCodable/Defaults/DefaultZeroFloatTests.swift(1 hunks)Tests/KarrotCodableKitTests/BetterCodable/Defaults/DefaultZeroIntTests.swift(1 hunks)Tests/KarrotCodableKitTests/BetterCodable/LosslessValue/LosslessArrayTests.swift(1 hunks)Tests/KarrotCodableKitTests/BetterCodable/LosslessValue/LosslessCustomValueTests.swift(1 hunks)Tests/KarrotCodableKitTests/BetterCodable/LosslessValue/LosslessValueTests.swift(5 hunks)Tests/KarrotCodableKitTests/BetterCodable/LossyValue/LossyArrayResilientTests.swift(1 hunks)Tests/KarrotCodableKitTests/BetterCodable/LossyValue/LossyArrayTests.swift(1 hunks)Tests/KarrotCodableKitTests/BetterCodable/LossyValue/LossyDictionaryResilientTests.swift(2 hunks)Tests/KarrotCodableKitTests/BetterCodable/LossyValue/LossyDictionaryTests.swift(5 hunks)Tests/KarrotCodableKitTests/BetterCodable/LossyValue/LossyOptionalTests.swift(1 hunks)Tests/KarrotCodableKitTests/BetterCodable/RawRepresentableTests.swift(2 hunks)Tests/KarrotCodableKitTests/PolymorphicCodable/ArrayValue/DefaultEmptyPolymorphicArrayValueResilientTests.swift(2 hunks)Tests/KarrotCodableKitTests/PolymorphicCodable/ArrayValue/PolymorphicLossyArrayValueResilientTests.swift(1 hunks)Tests/KarrotCodableMacrosTests/CustomCodableMacros/CustomCodableMacroTests.swift(4 hunks)Tests/KarrotCodableMacrosTests/CustomCodableMacros/CustomDecodableMacroTests.swift(4 hunks)Tests/KarrotCodableMacrosTests/CustomCodableMacros/CustomEncodableMacroTests.swift(4 hunks)Tests/KarrotCodableMacrosTests/PolymorphicCodableMacrosTests/PolymorphicCodableMacroTests.swift(4 hunks)Tests/KarrotCodableMacrosTests/PolymorphicCodableMacrosTests/PolymorphicCodableStrategyProvidingMacroTests.swift(8 hunks)Tests/KarrotCodableMacrosTests/PolymorphicCodableMacrosTests/PolymorphicDeodableMacroTests.swift(4 hunks)Tests/KarrotCodableMacrosTests/PolymorphicCodableMacrosTests/PolymorphicEncodableMacroTests.swift(4 hunks)Tests/KarrotCodableMacrosTests/PolymorphicEnumCodableMacroTests/PolymorphicEnumCodableMacroTests.swift(18 hunks)Tests/KarrotCodableMacrosTests/PolymorphicEnumCodableMacroTests/PolymorphicEnumDecodableMacroTests.swift(18 hunks)Tests/KarrotCodableMacrosTests/PolymorphicEnumCodableMacroTests/PolymorphicEnumEncodableMacroTests.swift(12 hunks)Tests/KarrotCodableMacrosTests/UnnestedPolymorphicCodableMacroTests/UnnestedPolymorphicCodableMacroTests.swift(16 hunks)Tests/KarrotCodableMacrosTests/UnnestedPolymorphicCodableMacroTests/UnnestedPolymorphicDecodableMacroTests.swift(16 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
Tests/KarrotCodableMacrosTests/**
⚙️ CodeRabbit configuration file
Tests/KarrotCodableMacrosTests/**: You are a senior Swift/iOS engineer reviewing macro expansion tests for KarrotCodableKit's Swift macro implementations.1. Macro Expansion Testing [HIGH]
- Verify SwiftSyntaxMacrosTestSupport usage is comprehensive
- Check that all macro expansion scenarios are covered
- Assess edge case testing for invalid macro arguments
- Validate diagnostic message testing matches actual validation logic
2. Test Code Quality [HIGH]
- Review test case organization and naming clarity
- Check that test data represents realistic usage scenarios
- Verify test assertions are specific and meaningful
- Assess test maintainability and readability
3. Coverage Assessment [MEDIUM]
- Ensure all macro variants (Codable, Decodable, Encodable) are tested
- Check polymorphic macro test coverage for different identifier strategies
- Verify error conditions and validation failures are tested
- Assess macro registration and plugin functionality testing
Review Focus
- Prioritize test correctness and comprehensive coverage
- Focus on macro-specific testing patterns and SwiftSyntax integration
- Mark comments with priority: [HIGH], [MEDIUM], or [LOW]
Files:
Tests/KarrotCodableMacrosTests/CustomCodableMacros/CustomDecodableMacroTests.swiftTests/KarrotCodableMacrosTests/PolymorphicEnumCodableMacroTests/PolymorphicEnumEncodableMacroTests.swiftTests/KarrotCodableMacrosTests/CustomCodableMacros/CustomCodableMacroTests.swiftTests/KarrotCodableMacrosTests/PolymorphicEnumCodableMacroTests/PolymorphicEnumDecodableMacroTests.swiftTests/KarrotCodableMacrosTests/PolymorphicCodableMacrosTests/PolymorphicCodableStrategyProvidingMacroTests.swiftTests/KarrotCodableMacrosTests/UnnestedPolymorphicCodableMacroTests/UnnestedPolymorphicDecodableMacroTests.swiftTests/KarrotCodableMacrosTests/CustomCodableMacros/CustomEncodableMacroTests.swiftTests/KarrotCodableMacrosTests/PolymorphicCodableMacrosTests/PolymorphicEncodableMacroTests.swiftTests/KarrotCodableMacrosTests/PolymorphicEnumCodableMacroTests/PolymorphicEnumCodableMacroTests.swiftTests/KarrotCodableMacrosTests/PolymorphicCodableMacrosTests/PolymorphicDeodableMacroTests.swiftTests/KarrotCodableMacrosTests/UnnestedPolymorphicCodableMacroTests/UnnestedPolymorphicCodableMacroTests.swiftTests/KarrotCodableMacrosTests/PolymorphicCodableMacrosTests/PolymorphicCodableMacroTests.swift
Tests/KarrotCodableKitTests/**
⚙️ CodeRabbit configuration file
Tests/KarrotCodableKitTests/**: You are a senior Swift/iOS engineer reviewing runtime functionality tests for KarrotCodableKit's Codable extensions.1. Runtime Behavior Testing [HIGH]
- Verify JSON encoding/decoding scenarios cover real-world use cases
- Check polymorphic type resolution testing across different strategies
- Assess property wrapper testing (@DATEvalue, @DefaultFalse, etc.)
- Validate error handling and edge case coverage
2. Test Data Quality [HIGH]
- Review test JSON structures for realistic complexity
- Check that TestDoubles provide comprehensive mock scenarios
- Verify test data covers various data types and edge cases
- Assess polymorphic test data represents actual usage patterns
3. Integration Testing [MEDIUM]
- Check integration between macro-generated code and runtime functionality
- Verify end-to-end scenarios combining multiple KarrotCodableKit features
- Assess performance testing for large data structures
- Review memory usage testing for complex polymorphic scenarios
Review Focus
- Prioritize real-world usage scenario coverage
- Focus on Codable correctness and performance implications
- Mark comments with priority: [HIGH], [MEDIUM], or [LOW]
Files:
Tests/KarrotCodableKitTests/AnyCodable/AnyDecodableTests.swiftTests/KarrotCodableKitTests/PolymorphicCodable/ArrayValue/DefaultEmptyPolymorphicArrayValueResilientTests.swiftTests/KarrotCodableKitTests/BetterCodable/LossyValue/LossyOptionalTests.swiftTests/KarrotCodableKitTests/BetterCodable/Defaults/DefaultZeroFloatTests.swiftTests/KarrotCodableKitTests/BetterCodable/Defaults/DefaultZeroDoubleTests.swiftTests/KarrotCodableKitTests/BetterCodable/RawRepresentableTests.swiftTests/KarrotCodableKitTests/PolymorphicCodable/ArrayValue/PolymorphicLossyArrayValueResilientTests.swiftTests/KarrotCodableKitTests/BetterCodable/LosslessValue/LosslessArrayTests.swiftTests/KarrotCodableKitTests/BetterCodable/DataValue/DataValueTests.swiftTests/KarrotCodableKitTests/BetterCodable/Defaults/DefaultEmptyStringTests.swiftTests/KarrotCodableKitTests/BetterCodable/LossyValue/LossyArrayResilientTests.swiftTests/KarrotCodableKitTests/BetterCodable/LosslessValue/LosslessCustomValueTests.swiftTests/KarrotCodableKitTests/AnyCodable/AnyEncodableTests.swiftTests/KarrotCodableKitTests/BetterCodable/Defaults/DefaultZeroIntTests.swiftTests/KarrotCodableKitTests/BetterCodable/LossyValue/LossyDictionaryResilientTests.swiftTests/KarrotCodableKitTests/AnyCodable/AnyCodableTests.swiftTests/KarrotCodableKitTests/BetterCodable/Defaults/DefaultEmptyArrayTests.swiftTests/KarrotCodableKitTests/BetterCodable/Defaults/DefaultTrueTests.swiftTests/KarrotCodableKitTests/BetterCodable/LosslessValue/LosslessValueTests.swiftTests/KarrotCodableKitTests/BetterCodable/Defaults/DefaultCodableResilientTests.swiftTests/KarrotCodableKitTests/BetterCodable/LossyValue/LossyArrayTests.swiftTests/KarrotCodableKitTests/BetterCodable/Defaults/DefaulEmptyDictionaryTests.swiftTests/KarrotCodableKitTests/BetterCodable/LossyValue/LossyDictionaryTests.swiftTests/KarrotCodableKitTests/BetterCodable/Defaults/DefaultFalseTests.swift
Sources/KarrotCodableKit/**
⚙️ CodeRabbit configuration file
Sources/KarrotCodableKit/**: You are a senior Swift/iOS engineer reviewing runtime library code for KarrotCodableKit, a comprehensive Codable extension framework.1. Codable Performance [HIGH]
- Check encoding/decoding efficiency and memory usage
- Verify proper handling of large JSON structures
- Assess polymorphic type resolution performance
- Review property wrapper overhead and optimization
2. Type Safety & Polymorphism [HIGH]
- Validate PolymorphicCodable identifier-based type resolution
- Check AnyCodable type erasure implementation for edge cases
- Verify UnnestedPolymorphic macro integration with runtime components
- Assess error handling in polymorphic decoding scenarios
3. API Design [HIGH]
- Evaluate public interface consistency across modules
- Check property wrapper ergonomics (@DefaultFalse, @DATEvalue, etc.)
- Verify protocol design follows Swift API guidelines
- Assess extensibility for new Codable patterns
4. BetterCodable Integration [MEDIUM]
- Review property wrapper implementations for common patterns
- Check date strategy implementations (ISO8601, RFC3339, etc.)
- Verify default value and lossy conversion handling
- Assess data encoding strategies (Base64, etc.)
5. Error Handling [MEDIUM]
- Verify comprehensive DecodingError and EncodingError usage
- Check PolymorphicCodableError provides sufficient context
- Assess graceful fallback handling in polymorphic scenarios
Review Focus
- Prioritize runtime correctness and performance over style
- Focus on real-world JSON processing scenarios
- Mark comments with priority: [HIGH], [MEDIUM], or [LOW]
Files:
Sources/KarrotCodableKit/BetterCodable/LossyValue/LossyArray.swiftSources/KarrotCodableKit/BetterCodable/LossyValue/LossyDictionary.swiftSources/KarrotCodableKit/BetterCodable/Defaults/DefaultCodable.swiftSources/KarrotCodableKit/PolymorphicCodable/Extensions/KeyedDecodingContainer+DefaultEmptyPolymorphicArrayValue.swiftSources/KarrotCodableKit/PolymorphicCodable/Extensions/KeyedDecodingContainer+PolymorphicLossyArrayValue.swift
🧬 Code graph analysis (13)
Sources/KarrotCodableKit/BetterCodable/LossyValue/LossyArray.swift (2)
Sources/KarrotCodableKit/Resilient/ErrorReporting.swift (1)
reportError(154-166)Sources/KarrotCodableKit/Resilient/ResilientDecodingOutcome.swift (1)
recoveredFrom(46-46)
Sources/KarrotCodableKit/BetterCodable/LossyValue/LossyDictionary.swift (2)
Sources/KarrotCodableKit/Resilient/ErrorReporting.swift (1)
reportError(154-166)Sources/KarrotCodableKit/Resilient/ResilientDecodingOutcome.swift (1)
recoveredFrom(46-46)
Tests/KarrotCodableKitTests/BetterCodable/LosslessValue/LosslessArrayTests.swift (2)
Sources/KarrotCodableKit/BetterCodable/LosslessValue/LosslessArray.swift (1)
encode(81-83)Sources/KarrotCodableKit/BetterCodable/LosslessValue/LosslessValue.swift (1)
encode(78-87)
Tests/KarrotCodableKitTests/BetterCodable/DataValue/DataValueTests.swift (2)
Sources/KarrotCodableKit/BetterCodable/DataValue/Base64Strategy.swift (2)
decode(14-20)encode(22-24)Sources/KarrotCodableKit/BetterCodable/DataValue/DataValue.swift (1)
encode(61-63)
Tests/KarrotCodableKitTests/AnyCodable/AnyEncodableTests.swift (1)
Sources/KarrotCodableKit/AnyCodable/AnyEncodable.swift (2)
encode(57-116)encode(119-150)
Tests/KarrotCodableKitTests/BetterCodable/Defaults/DefaultTrueTests.swift (2)
Sources/KarrotCodableKit/BetterCodable/Defaults/DefaultCodable.swift (4)
decode(122-172)decode(180-251)decode(258-323)encode(94-97)Tests/KarrotCodableKitTests/BetterCodable/Defaults/DefaultFalseTests.swift (4)
testDecodingKeyNotPresentDefaultsToFalse(29-38)testEncodingDecodedFailableArrayDefaultsToFalse(40-52)testEncodingDecodedFulfillableBoolRetainsValue(54-65)testDecodingMisalignedBoolIntValueDecodesCorrectBoolValue(67-79)
Sources/KarrotCodableKit/BetterCodable/Defaults/DefaultCodable.swift (5)
Sources/KarrotCodableKit/PolymorphicCodable/Extensions/KeyedDecodingContainer+PolymorphicLossyArrayValue.swift (1)
decode(12-72)Sources/KarrotCodableKit/PolymorphicCodable/Extensions/KeyedDecodingContainer+DefaultEmptyPolymorphicArrayValue.swift (1)
decode(12-51)Sources/KarrotCodableKit/BetterCodable/LossyValue/LossyDictionary.swift (1)
decode(275-297)Sources/KarrotCodableKit/Resilient/ErrorReporting.swift (1)
reportError(154-166)Sources/KarrotCodableKit/Resilient/ResilientDecodingOutcome.swift (1)
recoveredFrom(46-46)
Sources/KarrotCodableKit/PolymorphicCodable/Extensions/KeyedDecodingContainer+DefaultEmptyPolymorphicArrayValue.swift (2)
Sources/KarrotCodableKit/Resilient/ErrorReporting.swift (1)
reportError(154-166)Sources/KarrotCodableKit/Resilient/ResilientDecodingOutcome.swift (1)
recoveredFrom(46-46)
Tests/KarrotCodableKitTests/BetterCodable/Defaults/DefaultCodableResilientTests.swift (2)
Sources/KarrotCodableKit/BetterCodable/Defaults/DefaultCodable.swift (3)
decode(122-172)decode(180-251)decode(258-323)Sources/KarrotCodableKit/Resilient/ErrorReporting.swift (1)
decode(56-71)
Tests/KarrotCodableKitTests/BetterCodable/LossyValue/LossyArrayTests.swift (1)
Sources/KarrotCodableKit/BetterCodable/LossyValue/LossyArray.swift (1)
encode(109-111)
Tests/KarrotCodableKitTests/BetterCodable/Defaults/DefaulEmptyDictionaryTests.swift (2)
Sources/KarrotCodableKit/BetterCodable/LossyValue/LossyDictionary.swift (2)
decode(275-297)encode(259-261)Sources/KarrotCodableKit/BetterCodable/Defaults/DefaultCodable.swift (4)
decode(122-172)decode(180-251)decode(258-323)encode(94-97)
Tests/KarrotCodableKitTests/BetterCodable/Defaults/DefaultFalseTests.swift (2)
Sources/KarrotCodableKit/BetterCodable/Defaults/DefaultCodable.swift (4)
decode(122-172)decode(180-251)decode(258-323)encode(94-97)Tests/KarrotCodableKitTests/BetterCodable/Defaults/DefaultTrueTests.swift (5)
testDecodingKeyNotPresentDefaultsToFalse(28-37)testEncodingDecodedFailableArrayDefaultsToFalse(39-51)testEncodingDecodedFulfillableBoolRetainsValue(53-64)testDecodingMisalignedBoolIntValueDecodesCorrectBoolValue(66-78)testDecodingInvalidValueDecodesToDefaultValue(80-93)
Sources/KarrotCodableKit/PolymorphicCodable/Extensions/KeyedDecodingContainer+PolymorphicLossyArrayValue.swift (2)
Sources/KarrotCodableKit/Resilient/ErrorReporting.swift (1)
reportError(154-166)Sources/KarrotCodableKit/Resilient/ResilientDecodingOutcome.swift (1)
recoveredFrom(46-46)
🪛 SwiftLint (0.57.0)
Tests/KarrotCodableKitTests/BetterCodable/LossyValue/LossyOptionalTests.swift
[Warning] 21-21: Prefer non-optional Data(_:) initializer when converting String to Data
(non_optional_string_data_conversion)
[Warning] 35-35: Prefer non-optional Data(_:) initializer when converting String to Data
(non_optional_string_data_conversion)
[Warning] 51-51: Prefer non-optional Data(_:) initializer when converting String to Data
(non_optional_string_data_conversion)
[Warning] 68-68: Prefer non-optional Data(_:) initializer when converting String to Data
(non_optional_string_data_conversion)
[Warning] 82-82: Prefer non-optional Data(_:) initializer when converting String to Data
(non_optional_string_data_conversion)
Tests/KarrotCodableKitTests/BetterCodable/Defaults/DefaultZeroFloatTests.swift
[Warning] 20-20: Prefer non-optional Data(_:) initializer when converting String to Data
(non_optional_string_data_conversion)
[Warning] 31-31: Prefer non-optional Data(_:) initializer when converting String to Data
(non_optional_string_data_conversion)
[Warning] 42-42: Prefer non-optional Data(_:) initializer when converting String to Data
(non_optional_string_data_conversion)
Tests/KarrotCodableKitTests/BetterCodable/Defaults/DefaultZeroDoubleTests.swift
[Warning] 20-20: Prefer non-optional Data(_:) initializer when converting String to Data
(non_optional_string_data_conversion)
[Warning] 31-31: Prefer non-optional Data(_:) initializer when converting String to Data
(non_optional_string_data_conversion)
[Warning] 42-42: Prefer non-optional Data(_:) initializer when converting String to Data
(non_optional_string_data_conversion)
Tests/KarrotCodableKitTests/BetterCodable/LosslessValue/LosslessArrayTests.swift
[Warning] 23-23: Prefer non-optional Data(_:) initializer when converting String to Data
(non_optional_string_data_conversion)
[Warning] 34-34: Prefer non-optional Data(_:) initializer when converting String to Data
(non_optional_string_data_conversion)
[Warning] 45-45: Prefer non-optional Data(_:) initializer when converting String to Data
(non_optional_string_data_conversion)
[Warning] 56-56: Prefer non-optional Data(_:) initializer when converting String to Data
(non_optional_string_data_conversion)
[Warning] 74-74: Prefer non-optional Data(_:) initializer when converting String to Data
(non_optional_string_data_conversion)
Tests/KarrotCodableKitTests/BetterCodable/DataValue/DataValueTests.swift
[Warning] 18-18: Prefer non-optional Data(_:) initializer when converting String to Data
(non_optional_string_data_conversion)
[Warning] 38-38: Prefer non-optional Data(_:) initializer when converting String to Data
(non_optional_string_data_conversion)
[Warning] 49-49: Prefer non-optional Data(_:) initializer when converting String to Data
(non_optional_string_data_conversion)
Tests/KarrotCodableKitTests/BetterCodable/Defaults/DefaultEmptyStringTests.swift
[Warning] 20-20: Prefer non-optional Data(_:) initializer when converting String to Data
(non_optional_string_data_conversion)
[Warning] 31-31: Prefer non-optional Data(_:) initializer when converting String to Data
(non_optional_string_data_conversion)
[Warning] 42-42: Prefer non-optional Data(_:) initializer when converting String to Data
(non_optional_string_data_conversion)
Tests/KarrotCodableKitTests/BetterCodable/LosslessValue/LosslessCustomValueTests.swift
[Warning] 35-35: Prefer non-optional Data(_:) initializer when converting String to Data
(non_optional_string_data_conversion)
[Warning] 49-49: Prefer non-optional Data(_:) initializer when converting String to Data
(non_optional_string_data_conversion)
Tests/KarrotCodableKitTests/AnyCodable/AnyEncodableTests.swift
[Error] 48-48: Force casts should be avoided
(force_cast)
[Error] 97-97: Force casts should be avoided
(force_cast)
[Error] 149-149: Force casts should be avoided
(force_cast)
Tests/KarrotCodableKitTests/BetterCodable/Defaults/DefaultZeroIntTests.swift
[Warning] 19-19: Prefer non-optional Data(_:) initializer when converting String to Data
(non_optional_string_data_conversion)
[Warning] 30-30: Prefer non-optional Data(_:) initializer when converting String to Data
(non_optional_string_data_conversion)
[Warning] 41-41: Prefer non-optional Data(_:) initializer when converting String to Data
(non_optional_string_data_conversion)
Tests/KarrotCodableKitTests/BetterCodable/Defaults/DefaultEmptyArrayTests.swift
[Warning] 25-25: Prefer non-optional Data(_:) initializer when converting String to Data
(non_optional_string_data_conversion)
[Warning] 37-37: Prefer non-optional Data(_:) initializer when converting String to Data
(non_optional_string_data_conversion)
[Warning] 49-49: Prefer non-optional Data(_:) initializer when converting String to Data
(non_optional_string_data_conversion)
[Warning] 65-65: Prefer non-optional Data(_:) initializer when converting String to Data
(non_optional_string_data_conversion)
Tests/KarrotCodableKitTests/BetterCodable/Defaults/DefaultTrueTests.swift
[Warning] 19-19: Prefer non-optional Data(_:) initializer when converting String to Data
(non_optional_string_data_conversion)
[Warning] 30-30: Prefer non-optional Data(_:) initializer when converting String to Data
(non_optional_string_data_conversion)
[Warning] 41-41: Prefer non-optional Data(_:) initializer when converting String to Data
(non_optional_string_data_conversion)
[Warning] 55-55: Prefer non-optional Data(_:) initializer when converting String to Data
(non_optional_string_data_conversion)
[Warning] 68-68: Prefer non-optional Data(_:) initializer when converting String to Data
(non_optional_string_data_conversion)
[Warning] 69-69: Prefer non-optional Data(_:) initializer when converting String to Data
(non_optional_string_data_conversion)
[Warning] 82-82: Prefer non-optional Data(_:) initializer when converting String to Data
(non_optional_string_data_conversion)
[Warning] 97-97: Prefer non-optional Data(_:) initializer when converting String to Data
(non_optional_string_data_conversion)
[Warning] 98-98: Prefer non-optional Data(_:) initializer when converting String to Data
(non_optional_string_data_conversion)
Tests/KarrotCodableKitTests/BetterCodable/LossyValue/LossyArrayTests.swift
[Warning] 25-25: Prefer non-optional Data(_:) initializer when converting String to Data
(non_optional_string_data_conversion)
[Warning] 37-37: Prefer non-optional Data(_:) initializer when converting String to Data
(non_optional_string_data_conversion)
[Warning] 49-49: Prefer non-optional Data(_:) initializer when converting String to Data
(non_optional_string_data_conversion)
[Warning] 65-65: Prefer non-optional Data(_:) initializer when converting String to Data
(non_optional_string_data_conversion)
[Warning] 83-83: Prefer non-optional Data(_:) initializer when converting String to Data
(non_optional_string_data_conversion)
Tests/KarrotCodableKitTests/BetterCodable/Defaults/DefaulEmptyDictionaryTests.swift
[Warning] 19-19: Prefer non-optional Data(_:) initializer when converting String to Data
(non_optional_string_data_conversion)
[Warning] 30-30: Prefer non-optional Data(_:) initializer when converting String to Data
(non_optional_string_data_conversion)
[Warning] 41-41: Prefer non-optional Data(_:) initializer when converting String to Data
(non_optional_string_data_conversion)
[Warning] 55-55: Prefer non-optional Data(_:) initializer when converting String to Data
(non_optional_string_data_conversion)
Tests/KarrotCodableKitTests/BetterCodable/Defaults/DefaultFalseTests.swift
[Warning] 20-20: Prefer non-optional Data(_:) initializer when converting String to Data
(non_optional_string_data_conversion)
[Warning] 31-31: Prefer non-optional Data(_:) initializer when converting String to Data
(non_optional_string_data_conversion)
[Warning] 42-42: Prefer non-optional Data(_:) initializer when converting String to Data
(non_optional_string_data_conversion)
[Warning] 56-56: Prefer non-optional Data(_:) initializer when converting String to Data
(non_optional_string_data_conversion)
[Warning] 69-69: Prefer non-optional Data(_:) initializer when converting String to Data
(non_optional_string_data_conversion)
[Warning] 70-70: Prefer non-optional Data(_:) initializer when converting String to Data
(non_optional_string_data_conversion)
[Warning] 83-83: Prefer non-optional Data(_:) initializer when converting String to Data
(non_optional_string_data_conversion)
[Warning] 84-84: Prefer non-optional Data(_:) initializer when converting String to Data
(non_optional_string_data_conversion)
[Warning] 97-97: Prefer non-optional Data(_:) initializer when converting String to Data
(non_optional_string_data_conversion)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build and Test (16.4, release)
- GitHub Check: Check Macro Compatibility
- GitHub Check: Build and Test (16.4, debug)
🔇 Additional comments (25)
Tests/KarrotCodableMacrosTests/PolymorphicCodableMacrosTests/PolymorphicDeodableMacroTests.swift (4)
29-31: Nice addition: // given improves readabilityThe arrange step is now explicit; please apply this consistently across the suite for uniformity.
[LOW]
76-78: Good: // given clarifies input setupKeeps the invalid-argument scenario self-documenting.
[LOW]
21-25: Macro registration looks correct
testMacrosmaps the attribute name to the macro type as expected for SwiftSyntaxMacrosTestSupport.
[LOW]
107-114: Diagnostic message is stable and matches implementation; tests are correct.Tests/KarrotCodableKitTests/BetterCodable/RawRepresentableTests.swift (1)
37-37: No-op formatting change [LOW]Blank line only; nothing to review.
Tests/KarrotCodableKitTests/BetterCodable/Defaults/DefaultEmptyStringTests.swift (1)
18-27: Update test to assert decoding error for nil DefaultEmptyString
The test for non-optional@DefaultEmptyString var string: Stringcurrently expectsfixture.string == ""when the JSON value isnull, but per the PR’s objective non-optional wrappers should now throw onvalueWasNilor missing keys. Replace:- let fixture = try JSONDecoder().decode(Fixture.self, from: jsonData) - XCTAssertEqual(fixture.string, "") + XCTAssertThrowsError(try JSONDecoder().decode(Fixture.self, from: jsonData))Tests/KarrotCodableKitTests/BetterCodable/Defaults/DefaultEmptyArrayTests.swift (1)
35-45: [HIGH] — Same issue for missing keys.Non-optional
values/nonPrimitiveValuesshould not silently default. Align assertions with erroring behavior.- let fixture = try JSONDecoder().decode(Fixture.self, from: jsonData) - XCTAssertEqual(fixture.values, []) - XCTAssertEqual(fixture.nonPrimitiveValues, []) + XCTAssertThrowsError(try JSONDecoder().decode(Fixture.self, from: jsonData))Likely an incorrect or invalid review comment.
Tests/KarrotCodableMacrosTests/CustomCodableMacros/CustomCodableMacroTests.swift (2)
195-228: [LOW] — LGTM on BDD annotations; macro assertions remain strong.No functional change; the expected CodingKeys exclude computed properties, which is correct.
236-273: [LOW] — LGTM; annotations improve readability without altering behavior.Tests/KarrotCodableKitTests/BetterCodable/LosslessValue/LosslessArrayTests.swift (1)
22-29: Readable Given/When/Then annotations — good polish.Nice clarity improvements; no behavior change. Priority: [LOW]
Also applies to: 33-41, 44-52, 55-70, 73-85
Tests/KarrotCodableKitTests/BetterCodable/LosslessValue/LosslessCustomValueTests.swift (1)
34-45: Given/When/Then comments improve intent readability.Keeps tests easy to scan; behavior unchanged. Priority: [LOW]
Also applies to: 48-53
Tests/KarrotCodableMacrosTests/PolymorphicCodableMacrosTests/PolymorphicCodableMacroTests.swift (1)
29-29: Macro test phase markers look good.Small readability win; expectations unchanged. Priority: [LOW]
Also applies to: 43-43, 76-76, 88-88
Tests/KarrotCodableMacrosTests/PolymorphicCodableMacrosTests/PolymorphicCodableStrategyProvidingMacroTests.swift (1)
29-29: Consistent Given/When markers across macro tests.Nice consistency; no functional impact. Priority: [LOW]
Also applies to: 46-46, 96-96, 111-111, 160-160, 174-174, 199-199, 214-214
Tests/KarrotCodableMacrosTests/PolymorphicEnumCodableMacroTests/PolymorphicEnumEncodableMacroTests.swift (1)
27-27: Readable phase comments; macro expectations remain precise.All expansions/diagnostics intact. Priority: [LOW]
Also applies to: 37-37, 69-69, 79-79, 111-111, 121-121, 146-146, 156-156, 181-181, 191-191
Sources/KarrotCodableKit/BetterCodable/LossyValue/LossyArray.swift (2)
97-104: [MEDIUM] Good: whole-array failure is reported and recovereddecoder.reportError(error) on container acquisition failure with DEBUG-only outcome is consistent with the new semantics. No changes requested.
31-36: Verify KeyedDecodingContainer.decode symmetry for LossyArray
NoKeyedDecodingContainer.decode(_:forKey:)extension forLossyArraywas found; please manually confirm that it reportsDecodingError.keyNotFoundin both DEBUG and RELEASE, and thatrecoveredFrom(..., wasReported: true)is only used under#if DEBUG.Tests/KarrotCodableKitTests/BetterCodable/LossyValue/LossyArrayResilientTests.swift (1)
141-146: [HIGH] Test update aligns with non-optional semanticsAsserting fixture.$objects.error != nil for null enforces the new “treat nil as error” rule for non-optional LossyArray.
Tests/KarrotCodableMacrosTests/UnnestedPolymorphicCodableMacroTests/UnnestedPolymorphicCodableMacroTests.swift (1)
27-27: [LOW] BDD comments improve readability without affecting behaviorCosmetic only; macro expansion inputs/outputs and diagnostics remain unchanged.
Also applies to: 48-48, 122-122, 135-135, 187-187, 199-199, 242-242, 256-256, 311-311, 324-324, 352-352, 364-364, 391-391, 405-405, 469-469, 481-481
Tests/KarrotCodableMacrosTests/PolymorphicEnumCodableMacroTests/PolymorphicEnumDecodableMacroTests.swift (4)
27-37: Good BDD-style test scaffolding; expansion stays readable. [LOW]Clear Given/When structure improves scanability; assertions and expandedSource remain unchanged.
Also applies to: 38-70
129-139: Error diagnostic for non-enum targets verified. [HIGH]Message and location look correct for attaching the attribute to a struct.
Also applies to: 140-156
199-209: Associated-value count validation looks correct. [HIGH]Message aligns with rule “exactly one associated value”.
Also applies to: 210-226
234-244: Missing associated value validation looks correct. [HIGH]Good coverage for “no payload” case.
Also applies to: 245-261
Sources/KarrotCodableKit/BetterCodable/LossyValue/LossyDictionary.swift (1)
202-209: Correct: nil dictionary now yields valueNotFound and reports error. [HIGH]Matches the PR objective for non-optional wrappers; recovery to empty is appropriate.
Sources/KarrotCodableKit/BetterCodable/Defaults/DefaultCodable.swift (1)
79-89: Remove DEBUG gating suggestion—error reporting is already unconditional
A search across all decoders showsdecoder.reportError(error)is invoked outside of any#if DEBUGguards, with only downstream outcome handling gated for debug builds. There’s no inconsistency to address.Likely an incorrect or invalid review comment.
Tests/KarrotCodableKitTests/BetterCodable/LossyValue/LossyDictionaryTests.swift (1)
44-70: Given/When/Then scaffolding improves readability—nice.
No functional changes; structure is clearer.
Priority: [LOW]
Sources/KarrotCodableKit/BetterCodable/Defaults/DefaultCodable.swift
Outdated
Show resolved
Hide resolved
Sources/KarrotCodableKit/BetterCodable/Defaults/DefaultCodable.swift
Outdated
Show resolved
Hide resolved
| let context = DecodingError.Context( | ||
| codingPath: decoder.codingPath, | ||
| debugDescription: "Value was nil but property is non-optional" | ||
| ) | ||
| let error = DecodingError.valueNotFound([T].self, context) | ||
| decoder.reportError(error) | ||
| self.init(wrappedValue: [], outcome: .recoveredFrom(error, wasReported: true)) | ||
| #else | ||
| self.init(wrappedValue: []) | ||
| #endif |
There was a problem hiding this comment.
🛠️ Refactor suggestion
[HIGH] Don’t gate production error reporting behind DEBUG for nil arrays
Reporting via decoder.reportError is cheap and opt-in (only active when a reporter is installed). Keep recovery semantics gated to DEBUG if you want, but always report the error so production can observe it.
Apply:
- #if DEBUG
- let context = DecodingError.Context(
- codingPath: decoder.codingPath,
- debugDescription: "Value was nil but property is non-optional"
- )
- let error = DecodingError.valueNotFound([T].self, context)
- decoder.reportError(error)
- self.init(wrappedValue: [], outcome: .recoveredFrom(error, wasReported: true))
- #else
- self.init(wrappedValue: [])
- #endif
+ let context = DecodingError.Context(
+ codingPath: decoder.codingPath,
+ debugDescription: "Value was nil but property is non-optional"
+ )
+ let error = DecodingError.valueNotFound([T].self, context)
+ decoder.reportError(error)
+ #if DEBUG
+ self.init(wrappedValue: [], outcome: .recoveredFrom(error, wasReported: true))
+ #else
+ self.init(wrappedValue: [])
+ #endif📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let context = DecodingError.Context( | |
| codingPath: decoder.codingPath, | |
| debugDescription: "Value was nil but property is non-optional" | |
| ) | |
| let error = DecodingError.valueNotFound([T].self, context) | |
| decoder.reportError(error) | |
| self.init(wrappedValue: [], outcome: .recoveredFrom(error, wasReported: true)) | |
| #else | |
| self.init(wrappedValue: []) | |
| #endif | |
| // Always report the missing-value error, even in production | |
| let context = DecodingError.Context( | |
| codingPath: decoder.codingPath, | |
| debugDescription: "Value was nil but property is non-optional" | |
| ) | |
| let error = DecodingError.valueNotFound([T].self, context) | |
| decoder.reportError(error) | |
| #if DEBUG | |
| // In debug builds, record that we recovered from the error | |
| self.init(wrappedValue: [], outcome: .recoveredFrom(error, wasReported: true)) | |
| #else | |
| // In production, still recover but without tracking outcome | |
| self.init(wrappedValue: []) | |
| #endif |
🤖 Prompt for AI Agents
In Sources/KarrotCodableKit/BetterCodable/LossyValue/LossyArray.swift around
lines 47–56, decoder.reportError(error) is currently inside the DEBUG block;
move the error creation and call to decoder.reportError out of the #if DEBUG so
the error is always reported, then keep the existing conditional for recovery
semantics — i.e., construct the DecodingError.Context and
DecodingError.valueNotFound, call decoder.reportError(error) unconditionally,
then inside #if DEBUG call self.init(wrappedValue: [], outcome:
.recoveredFrom(error, wasReported: true)) and in the #else call
self.init(wrappedValue: []).
| let encodedJSONObject = try JSONSerialization.jsonObject(with: json, options: []) as! NSDictionary | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
[MEDIUM] Replace force casts with XCTUnwrap to satisfy SwiftLint and avoid crashes
Use XCTUnwrap to safely downcast JSONSerialization results.
- let encodedJSONObject = try JSONSerialization.jsonObject(with: json, options: []) as! NSDictionary
+ let encodedJSONObject = try XCTUnwrap(JSONSerialization.jsonObject(with: json, options: []) as? NSDictionary)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let encodedJSONObject = try JSONSerialization.jsonObject(with: json, options: []) as! NSDictionary | |
| let encodedJSONObject = try XCTUnwrap(JSONSerialization.jsonObject(with: json, options: []) as? NSDictionary) |
🧰 Tools
🪛 SwiftLint (0.57.0)
[Error] 48-48: Force casts should be avoided
(force_cast)
🤖 Prompt for AI Agents
In Tests/KarrotCodableKitTests/AnyCodable/AnyEncodableTests.swift around lines
48-49, the test force-casts the JSONSerialization result using "as!
NSDictionary" which can crash and violates SwiftLint; replace the force-cast
with a safe unwrap using XCTUnwrap: call JSONSerialization.jsonObject(with:
json, options: []) as? NSDictionary, pass that optional into XCTUnwrap to get a
non-optional NSDictionary for assertions, and update any subsequent uses to rely
on the unwrapped value.
| // when | ||
| expandedSource: """ | ||
| struct DismissibleCallout: Notice { |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Rename // when → // then for the expectation section
This block asserts the expected expansion and diagnostics; label it as the “then” step.
[LOW]
- // when
+ // then📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // when | |
| expandedSource: """ | |
| struct DismissibleCallout: Notice { | |
| // then | |
| expandedSource: """ | |
| struct DismissibleCallout: Notice { |
🤖 Prompt for AI Agents
In
Tests/KarrotCodableMacrosTests/PolymorphicCodableMacrosTests/PolymorphicDeodableMacroTests.swift
around lines 90 to 92, the comment preceding the expected expansion block is
labeled "// when" but it should be "// then" because this section asserts the
expected output; update the comment text from "// when" to "// then" so the test
follows the given-when-then ordering.
|
|
||
| func testPolymorphicEncodableMacro() throws { | ||
| #if canImport(KarrotCodableKitMacros) | ||
| // given |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Clear Given/When annotations added. [LOW]
Looks good. Consider inserting // then before macros: to complete the BDD pattern.
Example:
""",
+ // then
macros: testMacros,
indentationWidth: .spaces(2)Also applies to: 43-43, 76-76, 90-90
🤖 Prompt for AI Agents
In
Tests/KarrotCodableMacrosTests/PolymorphicCodableMacrosTests/PolymorphicEncodableMacroTests.swift
around line 29 (and similarly at lines 43, 76, 90), the test comments currently
have "// given" and "// when" but are missing the "// then" step; add a "//
then" comment immediately before the macros invocation (or where
assertions/verification occur) to complete the Given/When/Then BDD pattern so
each block reads "// given", "// when", "// then".
|
|
||
| func testPolymorphicEnumCodableMacro() throws { | ||
| #if canImport(KarrotCodableKitMacros) | ||
| // given |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Nice BDD-style markers added. [LOW]
Adding // given and // when improves readability of macro expansion tests. Consider also adding a // then marker right before the expectations (expandedSource/diagnostics) or the macros: parameter for consistent Given/When/Then flow.
Example diff for the first test:
expandedSource: """
...
""",
+ // then
macros: testMacros,
indentationWidth: .spaces(2)
)Also applies to: 37-37, 89-89, 99-99, 151-151, 161-161, 186-186, 196-196, 221-221, 231-231, 256-256, 266-266, 295-295, 306-306, 363-363, 373-373, 398-398, 408-408
🤖 Prompt for AI Agents
In
Tests/KarrotCodableMacrosTests/PolymorphicEnumCodableMacroTests/PolymorphicEnumCodableMacroTests.swift
around lines 27, 37, 89, 99, 151, 161, 186, 196, 221, 231, 256, 266, 295, 306,
363, 373, 398, and 408, add a BDD-style "// then" marker immediately before each
test's expectations (the expandedSource/diagnostics assertions or the macros:
parameter) so each test follows a consistent Given/When/Then flow; insert the
comment right before the assertion block or macros parameter to clearly separate
the setup/act sections from the assertions.
| #endif | ||
| } | ||
|
|
||
| func testPolymorphicCodableStrategyProvidingMacroWithDefaultParameters() throws { |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Rename test for clarity and consistency. [MEDIUM]
The method name references “PolymorphicCodableStrategyProviding” but this suite targets PolymorphicEnumCodable. Rename to match the subject.
- func testPolymorphicCodableStrategyProvidingMacroWithDefaultParameters() throws {
+ func testPolymorphicEnumCodableMacroWithDefaultParameters() throws {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func testPolymorphicCodableStrategyProvidingMacroWithDefaultParameters() throws { | |
| func testPolymorphicEnumCodableMacroWithDefaultParameters() throws { |
🤖 Prompt for AI Agents
In
Tests/KarrotCodableMacrosTests/PolymorphicEnumCodableMacroTests/PolymorphicEnumCodableMacroTests.swift
around line 87, the test method name
testPolymorphicCodableStrategyProvidingMacroWithDefaultParameters incorrectly
references "PolymorphicCodableStrategyProviding"; change the method name to
reference PolymorphicEnumCodable for clarity and consistency (for example rename
to testPolymorphicEnumCodableMacroWithDefaultParameters), and update any
references/usages of the old name (calls or test discovery annotations) to the
new name.
|
|
||
| func testUnnestedPolymorphicDecodableMacroExpansion() throws { | ||
| #if canImport(KarrotCodableKitMacros) | ||
| // given |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Good addition of // given and // when across cases. [LOW]
This makes each macro scenario easy to scan. Add a // then before macros: to complete the triad.
Example:
""",
+ // then
macros: testMacros,
indentationWidth: .spaces(2)Also applies to: 47-47, 106-106, 119-119, 160-160, 172-172, 207-207, 220-220, 265-265, 276-276, 305-305, 316-316, 345-345, 357-357, 411-411, 422-422
🤖 Prompt for AI Agents
In
Tests/KarrotCodableMacrosTests/UnnestedPolymorphicCodableMacroTests/UnnestedPolymorphicDecodableMacroTests.swift
around line 26 (and also apply the same change at lines 47, 106, 119, 160, 172,
207, 220, 265, 276, 305, 316, 345, 357, 411, 422), add a "// then" comment
immediately before the macro invocation in each test case so the triad of "//
given", "// when", "// then" is complete; ensure the placement is directly above
the line that performs the macro assertion or check and use the same comment
style as the existing "// given" and "// when" lines.
Sources/KarrotCodableKit/BetterCodable/Defaults/DefaultCodable.swift
Outdated
Show resolved
Hide resolved
| public func decode<T>( | ||
| _ type: DefaultEmptyPolymorphicArrayValue<T>.Type, | ||
| _: DefaultEmptyPolymorphicArrayValue<T>.Type, | ||
| forKey key: Key | ||
| ) throws -> DefaultEmptyPolymorphicArrayValue<T> where T: PolymorphicCodableStrategy { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Public API label change is source-breaking; keep a deprecated overload for transition. [HIGH]
Dropping the external label from decode(type:forKey:) to decode(_:forKey:) follows Swift API guidelines but will break existing call sites. Add a forwarding overload marked deprecated to minimize churn.
extension KeyedDecodingContainer {
- public func decode<T>(
- _: DefaultEmptyPolymorphicArrayValue<T>.Type,
- forKey key: Key
- ) throws -> DefaultEmptyPolymorphicArrayValue<T> where T: PolymorphicCodableStrategy {
+ @available(*, deprecated, message: "Use decode(_:, forKey:) instead")
+ public func decode<T>(
+ _ type: DefaultEmptyPolymorphicArrayValue<T>.Type,
+ forKey key: Key
+ ) throws -> DefaultEmptyPolymorphicArrayValue<T> where T: PolymorphicCodableStrategy {
+ try decode(Optional(type)!, forKey: key) // forward to the unlabeled overload
+ }
+
+ public func decode<T>(
+ _: DefaultEmptyPolymorphicArrayValue<T>.Type,
+ forKey key: Key
+ ) throws -> DefaultEmptyPolymorphicArrayValue<T> where T: PolymorphicCodableStrategy {Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
Sources/KarrotCodableKit/PolymorphicCodable/Extensions/KeyedDecodingContainer+DefaultEmptyPolymorphicArrayValue.swift
around lines 12–15, you removed the external parameter label from
decode(type:forKey:) which is source-breaking; add back a deprecated forwarding
overload with the original signature decode(type:
DefaultEmptyPolymorphicArrayValue<T>.Type, forKey key: Key) marked @available(*,
deprecated, message: "Use decode(_:forKey:)") that simply calls the new
decode(_:forKey:) implementation and returns its result; ensure the overload has
the same generic constraints (where T: PolymorphicCodableStrategy) and preserves
thrown behavior so existing call sites continue to work while guiding users to
the new API.
| #if DEBUG | ||
| let context = DecodingError.Context( | ||
| codingPath: codingPath + [key], | ||
| debugDescription: "Key not found but property is non-optional" | ||
| ) | ||
| let error = DecodingError.keyNotFound(key, context) | ||
| let decoder = try superDecoder(forKey: key) | ||
| decoder.reportError(error) | ||
| return DefaultEmptyPolymorphicArrayValue(wrappedValue: [], outcome: .recoveredFrom(error, wasReported: true)) | ||
| #else | ||
| return DefaultEmptyPolymorphicArrayValue(wrappedValue: [], outcome: .keyNotFound) | ||
| #endif |
There was a problem hiding this comment.
Avoid superDecoder(forKey:) when contains(key) == false — it can throw and bypass recovery. [HIGH]
In the key-missing path you call try superDecoder(forKey:). Some decoders throw on missing keys, which would surface an unexpected error instead of returning the recovered value. Guard this with try? and report only if obtainable.
- let decoder = try superDecoder(forKey: key)
- decoder.reportError(error)
+ if let reportingDecoder = try? superDecoder(forKey: key) {
+ reportingDecoder.reportError(error)
+ }
return DefaultEmptyPolymorphicArrayValue(wrappedValue: [], outcome: .recoveredFrom(error, wasReported: true))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #if DEBUG | |
| let context = DecodingError.Context( | |
| codingPath: codingPath + [key], | |
| debugDescription: "Key not found but property is non-optional" | |
| ) | |
| let error = DecodingError.keyNotFound(key, context) | |
| let decoder = try superDecoder(forKey: key) | |
| decoder.reportError(error) | |
| return DefaultEmptyPolymorphicArrayValue(wrappedValue: [], outcome: .recoveredFrom(error, wasReported: true)) | |
| #else | |
| return DefaultEmptyPolymorphicArrayValue(wrappedValue: [], outcome: .keyNotFound) | |
| #endif | |
| #if DEBUG | |
| let context = DecodingError.Context( | |
| codingPath: codingPath + [key], | |
| debugDescription: "Key not found but property is non-optional" | |
| ) | |
| let error = DecodingError.keyNotFound(key, context) | |
| - let decoder = try superDecoder(forKey: key) | |
| if let reportingDecoder = try? superDecoder(forKey: key) { | |
| reportingDecoder.reportError(error) | |
| } | |
| return DefaultEmptyPolymorphicArrayValue(wrappedValue: [], outcome: .recoveredFrom(error, wasReported: true)) | |
| #else | |
| return DefaultEmptyPolymorphicArrayValue(wrappedValue: [], outcome: .keyNotFound) | |
| #endif |
🤖 Prompt for AI Agents
In
Sources/KarrotCodableKit/PolymorphicCodable/Extensions/KeyedDecodingContainer+DefaultEmptyPolymorphicArrayValue.swift
around lines 18 to 29, avoid calling try superDecoder(forKey:) when the key is
missing; change it to use optional try (try?) so the call cannot throw and only
call decoder.reportError(error) if the superDecoder was successfully obtained.
Concretely, wrap the superDecoder call in a try? and guard/if-let the result
before reporting the DecodingError, then return the same
DefaultEmptyPolymorphicArrayValue outcome regardless of whether a decoder was
obtained.
| #if DEBUG | ||
| let context = DecodingError.Context( | ||
| codingPath: codingPath + [key], | ||
| debugDescription: "Value was nil but property is non-optional" | ||
| ) | ||
| let error = DecodingError.valueNotFound([Any].self, context) | ||
| let decoder = try superDecoder(forKey: key) | ||
| decoder.reportError(error) | ||
| return DefaultEmptyPolymorphicArrayValue(wrappedValue: [], outcome: .recoveredFrom(error, wasReported: true)) | ||
| #else | ||
| return DefaultEmptyPolymorphicArrayValue(wrappedValue: [], outcome: .valueWasNil) | ||
| #endif |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Nil-path reporting is good; tighten the expected-type context and DRY up reporting. [MEDIUM]
- Consider using a more concrete expected type in
valueNotFound(e.g., the strategy’s element array if available) to improve diagnostics. - Extract a tiny helper to avoid repeating the
superDecoder + reportErrorpattern.
Example helper (add within this extension’s file scope):
@inline(__always)
fileprivate func _report(_ error: Error, in container: KeyedDecodingContainer<Key>, for key: Key) {
if let d = try? container.superDecoder(forKey: key) { d.reportError(error) }
}Then replace the two sites with _report(error, in: self, for: key).
🤖 Prompt for AI Agents
In
Sources/KarrotCodableKit/PolymorphicCodable/Extensions/KeyedDecodingContainer+DefaultEmptyPolymorphicArrayValue.swift
around lines 34–45, tighten the DecodingError.valueNotFound context by using a
more concrete expected type (the strategy’s element array type when available)
instead of [Any]. Then DRY up the superDecoder+reportError pattern by adding a
fileprivate helper (e.g., an @inline(__always) function in this file scope that
accepts Error, the container, and the key and calls superDecoder(forKey:) and
reportError if superDecoder succeeds) and replace the existing direct
superDecoder+reportError calls with that helper.
| _: DefaultEmptyPolymorphicArrayValue<T>.Type, | ||
| forKey key: Self.Key | ||
| ) throws -> DefaultEmptyPolymorphicArrayValue<T>? where T: PolymorphicCodableStrategy { |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
decodeIfPresent label change mirrors the above — add a deprecated overload too. [MEDIUM]
Keep a forwarding decodeIfPresent(type:forKey:) overload to reduce source breakage.
- public func decodeIfPresent<T>(
- _: DefaultEmptyPolymorphicArrayValue<T>.Type,
- forKey key: Self.Key
- ) throws -> DefaultEmptyPolymorphicArrayValue<T>? where T: PolymorphicCodableStrategy {
+ @available(*, deprecated, message: "Use decodeIfPresent(_:, forKey:) instead")
+ public func decodeIfPresent<T>(
+ _ type: DefaultEmptyPolymorphicArrayValue<T>.Type,
+ forKey key: Self.Key
+ ) throws -> DefaultEmptyPolymorphicArrayValue<T>? where T: PolymorphicCodableStrategy {
+ try decodeIfPresent(Optional(type)!, forKey: key)
+ }
+
+ public func decodeIfPresent<T>(
+ _: DefaultEmptyPolymorphicArrayValue<T>.Type,
+ forKey key: Self.Key
+ ) throws -> DefaultEmptyPolymorphicArrayValue<T>? where T: PolymorphicCodableStrategy {Committable suggestion skipped: line range outside the PR's diff.
| func testDecodinSuccessDefaultZeroInt() throws { | ||
| // given | ||
| let jsonData = #"{ "intValue": 999 }"#.data(using: .utf8)! | ||
|
|
||
| // when | ||
| let fixture = try JSONDecoder().decode(Fixture.self, from: jsonData) | ||
|
|
||
| // then | ||
| XCTAssertEqual(fixture.intValue, 999) | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
[LOW] [Naming + hygiene] — Fix test name typo and prefer Data(_:)
- func testDecodinSuccessDefaultZeroInt() throws {
+ func testDecodingSuccessDefaultZeroInt() throws {
@@
- let jsonData = #"{ "intValue": 999 }"#.data(using: .utf8)!
+ let jsonData = Data(#"{ "intValue": 999 }"#.utf8)🧰 Tools
🪛 SwiftLint (0.57.0)
[Warning] 41-41: Prefer non-optional Data(_:) initializer when converting String to Data
(non_optional_string_data_conversion)
🤖 Prompt for AI Agents
In Tests/KarrotCodableKitTests/BetterCodable/Defaults/DefaultZeroIntTests.swift
around lines 39 to 49, rename the test from testDecodinSuccessDefaultZeroInt to
testDecodingSuccessDefaultZeroInt to fix the typo, and change the JSON data
creation from using .data(using: .utf8)! to the Data initializer with a UTF8
view (e.g. Data(#"{ "intValue": 999 }"#.utf8)) to avoid the forced
string-to-Data conversion; keep the rest of the test logic unchanged.
| #if DEBUG | ||
| #expect(fixture.$stringDict.outcome == .keyNotFound) | ||
| #expect(fixture.$intDict.outcome == .keyNotFound) | ||
| #expect(fixture.$objectDict.outcome == .keyNotFound) | ||
|
|
||
| // No error when key is missing | ||
| #expect(fixture.$stringDict.error == nil) | ||
| #expect(fixture.$intDict.error == nil) | ||
| #expect(fixture.$objectDict.error == nil) | ||
| // Missing key should error for non-optional properties | ||
| #expect(fixture.$stringDict.error != nil) | ||
| #expect(fixture.$intDict.error != nil) | ||
| #expect(fixture.$objectDict.error != nil) | ||
| #endif |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Missing keys: also validate error type (keyNotFound).
Since these are non-optional, asserting the concrete kind avoids regressions.
Priority: [HIGH]
// Missing key should error for non-optional properties
- #expect(fixture.$stringDict.error != nil)
- #expect(fixture.$intDict.error != nil)
- #expect(fixture.$objectDict.error != nil)
+ #expect(fixture.$stringDict.error != nil)
+ #expect(fixture.$intDict.error != nil)
+ #expect(fixture.$objectDict.error != nil)
+ if let err = fixture.$stringDict.error as? DecodingError {
+ if case .keyNotFound = err {
+ // expected
+ } else {
+ Issue.record("Expected keyNotFound for missing stringDict")
+ }
+ }
+ // (Optional) replicate the kind check for intDict/objectDict if you want parity.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #if DEBUG | |
| #expect(fixture.$stringDict.outcome == .keyNotFound) | |
| #expect(fixture.$intDict.outcome == .keyNotFound) | |
| #expect(fixture.$objectDict.outcome == .keyNotFound) | |
| // No error when key is missing | |
| #expect(fixture.$stringDict.error == nil) | |
| #expect(fixture.$intDict.error == nil) | |
| #expect(fixture.$objectDict.error == nil) | |
| // Missing key should error for non-optional properties | |
| #expect(fixture.$stringDict.error != nil) | |
| #expect(fixture.$intDict.error != nil) | |
| #expect(fixture.$objectDict.error != nil) | |
| #endif | |
| #if DEBUG | |
| // Missing key should error for non-optional properties | |
| #expect(fixture.$stringDict.error != nil) | |
| #expect(fixture.$intDict.error != nil) | |
| #expect(fixture.$objectDict.error != nil) | |
| // Also validate that the error is specifically `.keyNotFound` | |
| if let err = fixture.$stringDict.error as? DecodingError { | |
| if case .keyNotFound = err { | |
| // expected | |
| } else { | |
| Issue.record("Expected keyNotFound for missing stringDict") | |
| } | |
| } | |
| // (Optional) replicate the kind check for intDict/objectDict if you want parity. | |
| #endif |
🤖 Prompt for AI Agents
In
Tests/KarrotCodableKitTests/BetterCodable/LossyValue/LossyDictionaryResilientTests.swift
around lines 148 to 153, the tests only assert that errors exist for missing
non-optional keys; update them to assert the error is specifically a
DecodingError.keyNotFound. Replace or supplement the current #expect checks so
each fixture.$stringDict.error, fixture.$intDict.error, and
fixture.$objectDict.error is unwrapped and validated to be a
DecodingError.keyNotFound (e.g., by pattern-matching or casting the error to
DecodingError and asserting .keyNotFound), keeping the existing failure behavior
if the error is different or nil.
| // missing key should error for non-optional property | ||
| #expect(result.$notices.error != nil) | ||
| #endif |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
[HIGH] [Behavior parity] — Also assert recoveredFrom outcome for missing key
Align with invalidType (Lines 161–166): assert the outcome is recoveredFrom when the key is missing for a non-optional property.
#if DEBUG
// missing key should error for non-optional property
#expect(result.$notices.error != nil)
+if case .recoveredFrom = result.$notices.outcome {
+ // Expected
+} else {
+ Issue.record("Expected recoveredFrom outcome")
+}
#endif📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // missing key should error for non-optional property | |
| #expect(result.$notices.error != nil) | |
| #endif | |
| #if DEBUG | |
| // missing key should error for non-optional property | |
| #expect(result.$notices.error != nil) | |
| if case .recoveredFrom = result.$notices.outcome { | |
| // Expected | |
| } else { | |
| Issue.record("Expected recoveredFrom outcome") | |
| } | |
| #endif |
🤖 Prompt for AI Agents
In
Tests/KarrotCodableKitTests/PolymorphicCodable/ArrayValue/DefaultEmptyPolymorphicArrayValueResilientTests.swift
around lines 119–121, the test for a missing key currently only checks that an
error exists; mirror the invalidType block (lines 161–166) by also asserting
that the decode outcome is recoveredFrom — add the same recoveredFrom assertion
used there (e.g. assert(result.$notices.recoveredFrom == true)) so the test
verifies behavior parity for missing non-optional keys.
| // null value should error for non-optional property | ||
| #expect(result.$notices.error != nil) | ||
| #endif |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
[HIGH] [Behavior parity] — Also assert recoveredFrom outcome for null value
Same reasoning as above; null for non-optional should error and recover.
#if DEBUG
// null value should error for non-optional property
#expect(result.$notices.error != nil)
+if case .recoveredFrom = result.$notices.outcome {
+ // Expected
+} else {
+ Issue.record("Expected recoveredFrom outcome")
+}
#endif📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // null value should error for non-optional property | |
| #expect(result.$notices.error != nil) | |
| #endif | |
| #if DEBUG | |
| // null value should error for non-optional property | |
| #expect(result.$notices.error != nil) | |
| if case .recoveredFrom = result.$notices.outcome { | |
| // Expected | |
| } else { | |
| Issue.record("Expected recoveredFrom outcome") | |
| } | |
| #endif |
🤖 Prompt for AI Agents
In
Tests/KarrotCodableKitTests/PolymorphicCodable/ArrayValue/DefaultEmptyPolymorphicArrayValueResilientTests.swift
around lines 140-142, the test currently only asserts that a null value for a
non-optional property produces an error; also assert that the decode
recoveredFrom flag is true for this null case. Add an assertion immediately
after the existing error check that verifies result.recoveredFrom (or the
equivalent property on the decode result) is true to confirm the value was
recovered despite the error.
| // missing key should error for non-optional property | ||
| #expect(result.$notices.error != nil) | ||
| #expect(result.$notices.results.isEmpty) |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
[MEDIUM] [Behavior parity] — Also assert recoveredFrom outcome for missing key
Good change to require an error for a non-optional property. For consistency with your invalidType test (Lines 159–166), also assert the outcome is recoveredFrom.
#if DEBUG
// missing key should error for non-optional property
#expect(result.$notices.error != nil)
+if case .recoveredFrom = result.$notices.outcome {
+ // Expected
+} else {
+ Issue.record("Expected recoveredFrom outcome")
+}
#expect(result.$notices.results.isEmpty)
#endif📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // missing key should error for non-optional property | |
| #expect(result.$notices.error != nil) | |
| #expect(result.$notices.results.isEmpty) | |
| #if DEBUG | |
| // missing key should error for non-optional property | |
| #expect(result.$notices.error != nil) | |
| if case .recoveredFrom = result.$notices.outcome { | |
| // Expected | |
| } else { | |
| Issue.record("Expected recoveredFrom outcome") | |
| } | |
| #expect(result.$notices.results.isEmpty) | |
| #endif |
🤖 Prompt for AI Agents
In
Tests/KarrotCodableKitTests/PolymorphicCodable/ArrayValue/PolymorphicLossyArrayValueResilientTests.swift
around lines 137 to 139, you currently assert an error for a missing
non-optional property but do not assert the recoveredFrom outcome; add the same
recoveredFrom assertion used in the invalidType test (lines 159–166) so the test
verifies that the error was recoveredFrom (i.e., assert the result's outcome
equals .recoveredFrom or the equivalent recovered flag on result.$notices).
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (13)
Sources/KarrotCodableKit/PolymorphicCodable/DefaultEmptyPolymorphicArrayValue.swift (3)
14-19: Docs vs implementation are inconsistent (throws vs recover; print vs reportError).The comments say “throws” and “logs via print,” but the code always recovers to [] and reports via
decoder.reportError. Please update to reflect actual behavior.Priority: [MEDIUM]
-/// - **Crucially, if *any* element within the array fails to decode according to the `PolymorphicType` strategy, the entire decoding process for the wrapper fails, and an error is thrown.** This wrapper does **not** skip invalid elements. -/// - If the initial step of obtaining the unkeyed container fails (e.g., the key is missing in the parent JSON object, or the corresponding value is `null` or not an array), it catches the error, assigns `[]` to `wrappedValue`, and logs the error using `print`. +/// - If any element fails to decode according to the `PolymorphicType` strategy, this value is considered invalid. The wrapper recovers by producing `[]` and, when a `ResilientDecodingErrorReporter` is configured, reports the error via the decoder. +/// - If obtaining the unkeyed container fails (e.g., missing key, `null`, or non-array), it recovers to `[]` and reports via `decoder.reportError` when available.
58-65: Preallocate array capacity from container.count to reduce reallocations.Priority: [MEDIUM]
var container = try decoder.unkeyedContainer() var elements = [PolymorphicType.ExpectedType]() + if let count = container.count { + elements.reserveCapacity(count) + } while !container.isAtEnd {
83-88: Avoid intermediate array on encode; stream into UnkeyedEncodingContainer.Priority: [MEDIUM]
- public func encode(to encoder: Encoder) throws { - let polymorphicValues = wrappedValue.map { - PolymorphicValue<PolymorphicType>(wrappedValue: $0) - } - try polymorphicValues.encode(to: encoder) - } + public func encode(to encoder: Encoder) throws { + var container = encoder.unkeyedContainer() + for value in wrappedValue { + try container.encode(PolymorphicValue<PolymorphicType>(wrappedValue: value)) + } + }Sources/KarrotCodableKit/PolymorphicCodable/Extensions/KeyedDecodingContainer+OptionalPolymorphicArrayValue.swift (2)
39-45: Preallocate array capacity for large payloads [MEDIUM]UnkeyedDecodingContainer exposes count; reserving capacity avoids repeated reallocations on large arrays.
var container = try nestedUnkeyedContainer(forKey: key) var elements = [T.ExpectedType]() + if let c = container.count { + elements.reserveCapacity(c) + }
1-59: Wrap unguarded reportError in DEBUG guard
OptionalPolymorphicArrayValue.swift (lines 97–101): thedecoder.reportError(error)call in the catch block isn’t wrapped in#if DEBUG/#endif; add those guards to match the rest of the module and prevent release-build overhead.Sources/KarrotCodableKit/PolymorphicCodable/Extensions/KeyedDecodingContainer+LossyOptionalPolymorphicValue.swift (1)
16-21: Bug: missing return statements cause compile error.The function must return a value on both branches.
Apply this diff:
- if let value = try decodeIfPresent(type, forKey: key) { - value - } else { - LossyOptionalPolymorphicValue(wrappedValue: nil, outcome: .keyNotFound) - } + if let value = try decodeIfPresent(type, forKey: key) { + return value + } else { + return LossyOptionalPolymorphicValue(wrappedValue: nil, outcome: .keyNotFound) + }Priority: [HIGH]
Sources/KarrotCodableKit/BetterCodable/LosslessValue/LosslessArray.swift (1)
41-42: [HIGH] Catch path may not advance the unkeyed container → risk of infinite loopDecoding
AnyDecodableValueas an empty struct won’t reliably consume non-object elements (e.g., numbers/strings), so after a failure the index may not advance andwhile !container.isAtEndcan loop forever.Fix by making the “skip one value” type always succeed for any JSON shape:
- private struct AnyDecodableValue: Decodable {} + private struct AnyDecodableValue: Decodable { + private struct AnyCodingKey: CodingKey { + var stringValue: String + init?(stringValue: String) { self.stringValue = stringValue } + var intValue: Int? + init?(intValue: Int) { self.intValue = intValue } + } + init(from decoder: Decoder) throws { + // If it’s a single value (string/number/bool/null), just accept it. + if (try? decoder.singleValueContainer()) != nil { return } + // If it’s an array, accepting the unkeyed container is enough to consume. + if (try? decoder.unkeyedContainer()) != nil { return } + // Otherwise, consume an object (keys ignored). + _ = try decoder.container(keyedBy: AnyCodingKey.self) + } + }Also applies to: 59-63
Sources/KarrotCodableKit/BetterCodable/LossyValue/LossyArray.swift (1)
91-93: [MEDIUM] Consider reporting the aggregated ArrayDecodingError as wellYou report individual element errors; also reporting the aggregated context improves diagnostics. Since this branch is DEBUG-only, it won’t affect release performance.
- let error = ResilientDecodingOutcome.ArrayDecodingError(results: results) - self.init(wrappedValue: elements, outcome: .recoveredFrom(error, wasReported: false)) + let error = ResilientDecodingOutcome.ArrayDecodingError(results: results) + decoder.reportError(error) + self.init(wrappedValue: elements, outcome: .recoveredFrom(error, wasReported: true))Sources/KarrotCodableKit/BetterCodable/LossyValue/LossyDictionary.swift (1)
92-106: Avoid double-decoding just to recover original string keys; iterate container keys once
extractKeysdecodes the whole dictionary a second time into[String: AnyDecodableValue], which is unnecessary and expensive for large payloads. For dictionaries,JSONDecoder.KeyDecodingStrategydoes not transform dynamic map keys, socontainer.allKeys[*].stringValuealready preserves the original keys. Streamline to a single pass and dropextractKeysandExtractedKey.Priority: [HIGH] (perf)
@@ - var state = DecodingState() - let container = try decoder.container(keyedBy: DictionaryCodingKey.self) - let keys = try extractKeys(from: decoder, container: container) - - for extractedKey in keys { - decodeSingleKeyValue( - container: container, - key: extractedKey.codingKey, - originalKey: extractedKey.originalKey, - state: &state - ) - } + var state = DecodingState() + let container = try decoder.container(keyedBy: DictionaryCodingKey.self) + for key in container.allKeys { + decodeSingleKeyValue( + container: container, + key: key, + originalKey: key.stringValue, + state: &state + ) + } @@ - private static func extractKeys( - from decoder: Decoder, - container: KeyedDecodingContainer<DictionaryCodingKey> - ) throws -> [ExtractedKey] { - // Decode a dictionary ignoring the values to decode the original keys - // without using the `JSONDecoder.KeyDecodingStrategy`. - let keys = try decoder.singleValueContainer().decode([String: AnyDecodableValue].self).keys - - return zip( - container.allKeys.sorted(by: { $0.stringValue < $1.stringValue }), - keys.sorted() - ) - .map { ExtractedKey(codingKey: $0, originalKey: $1) } - } + // removed: extractKeys (no longer needed)And remove the now-unused helper type:
- private struct ExtractedKey { - let codingKey: DictionaryCodingKey - let originalKey: String - }Also applies to: 135-143, 240-253, 64-67
Sources/KarrotCodableKit/BetterCodable/LosslessValue/LosslessValue.swift (4)
80-89: Prefer lossless String init and propagate encoder.codingPath in error.
This avoids relying onCustomStringConvertibleand improves diagnostics.
[LOW]- let string = String(describing: wrappedValue) + let string = String(wrappedValue) @@ - guard let original = type.init(string) else { + guard let original = type.init(string) else { let description = "Unable to encode '\(wrappedValue)' back to source type '\(type)'" - throw EncodingError.invalidValue(string, .init(codingPath: [], debugDescription: description)) + throw EncodingError.invalidValue(string, .init(codingPath: encoder.codingPath, debugDescription: description)) }
106-127: Consider including Int32/UInt32 in the default probing order.
Improves type preservation symmetry with other fixed-width integers.
[MEDIUM]decode(Int8.self), decode(Int16.self), + decode(Int32.self), decode(Int64.self), @@ decode(UInt8.self), decode(UInt16.self), + decode(UInt32.self), decode(UInt64.self),
58-66: Minor perf: reduce repeated decode attempts.
Each probe re-parses the same single value. If this shows up hot, consider first decoding asStringand attemptingStrategy.Value.init(_:), then only probe numerics when that fails. Keeps behavior while cutting parser churn.
[LOW]
74-77: Use.recoveredFromoutcome for fallback decoding. When decoding falls back via an alternate type, setoutcome = .recoveredFrom(error, wasReported: /* Bool */)instead of
outcome = .decodedSuccessfullyto accurately preserve telemetry.
♻️ Duplicate comments (5)
Sources/KarrotCodableKit/BetterCodable/LossyValue/LossyArray.swift (1)
47-53: [HIGH] Don’t gate production error reporting for nil arrays (repeat of prior feedback)Always call
decoder.reportError(error); keep recovery outcome DEBUG-gated if desired. This preserves observability in release builds. Mirrors the earlier comment on this file.- #if DEBUG - let context = DecodingError.Context( - codingPath: decoder.codingPath, - debugDescription: "Value was nil but property is non-optional" - ) - let error = DecodingError.valueNotFound([T].self, context) - decoder.reportError(error) - self.init(wrappedValue: [], outcome: .recoveredFrom(error, wasReported: true)) - #else - self.init(wrappedValue: []) - #endif + let context = DecodingError.Context( + codingPath: decoder.codingPath, + debugDescription: "Value was nil but property is non-optional" + ) + let error = DecodingError.valueNotFound([T].self, context) + decoder.reportError(error) + #if DEBUG + self.init(wrappedValue: [], outcome: .recoveredFrom(error, wasReported: true)) + #else + self.init(wrappedValue: []) + #endifSources/KarrotCodableKit/BetterCodable/Defaults/DefaultCodable.swift (4)
160-171: Remove unreachable “else: Key not found” branch after prior guardsAfter
contains(key)and explicitdecodeNil(forKey:)handling,decodeIfPresent(DefaultCodable<P>.self, forKey:)will not returnnilfor present, non-nil keys (it either decodes or throws). This else-branch is dead and duplicates the first not-found path.Priority: [MEDIUM]
- if let value = try decodeIfPresent(DefaultCodable<P>.self, forKey: key) { - return value - } else { - #if DEBUG - let context = DecodingError.Context( - codingPath: codingPath + [key], - debugDescription: "Key not found but property is non-optional" - ) - let error = DecodingError.keyNotFound(key, context) - ((try? superDecoder(forKey: key)) ?? (try? superDecoder()))? - .reportError(error) - return DefaultCodable(wrappedValue: P.defaultValue, outcome: .recoveredFrom(error, wasReported: true)) - #else - return DefaultCodable(wrappedValue: P.defaultValue) - #endif - } + if let value = try decodeIfPresent(DefaultCodable<P>.self, forKey: key) { + return value + }
126-134: Make error reporting best-effort: fall back tosuperDecoder()when keyed variant failsAcross all keyed paths, prefer
(try? superDecoder(forKey:)) ?? (try? superDecoder())so a missing/invalid key doesn’t suppress reporting. Apply uniformly to keep behavior consistent.Priority: [MEDIUM]
- let decoder = try? superDecoder(forKey: key) - decoder?.reportError(error) + ((try? superDecoder(forKey: key)) ?? (try? superDecoder()))? + .reportError(error)Also applies to: 147-149, 165-167, 184-191, 205-207, 223-226, 243-248, 269-271, 282-285, 307-310, 318-319
267-271: Unify debugDescription for missing keysUse the same wording as other branches for searchability and consistency.
Priority: [LOW]
- let context = DecodingError.Context(codingPath: codingPath + [key], debugDescription: "Key not found") + let context = DecodingError.Context( + codingPath: codingPath + [key], + debugDescription: "Key not found but property is non-optional" + )
279-285: Fix wrongvalueNotFoundtype and match message
valueNotFoundshould reference the missing value’s type, not the strategy type. Also align the message with other branches.Priority: [HIGH]
- let context = DecodingError.Context(codingPath: codingPath + [key], debugDescription: "Value was nil") - let error = DecodingError.valueNotFound(P.self, context) + let context = DecodingError.Context( + codingPath: codingPath + [key], + debugDescription: "Value was nil but property is non-optional" + ) + let error = DecodingError.valueNotFound(P.DefaultValue.self, context)
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (15)
Sources/KarrotCodableKit/BetterCodable/DataValue/DataValue.swift(1 hunks)Sources/KarrotCodableKit/BetterCodable/DateValue/DateValue.swift(1 hunks)Sources/KarrotCodableKit/BetterCodable/DateValue/OptionalDateValue.swift(1 hunks)Sources/KarrotCodableKit/BetterCodable/Defaults/DefaultCodable.swift(11 hunks)Sources/KarrotCodableKit/BetterCodable/Extensions/Result+Extension.swift(0 hunks)Sources/KarrotCodableKit/BetterCodable/LosslessValue/LosslessArray.swift(1 hunks)Sources/KarrotCodableKit/BetterCodable/LosslessValue/LosslessValue.swift(2 hunks)Sources/KarrotCodableKit/BetterCodable/LosslessValue/OptionalLosslessValue.swift(1 hunks)Sources/KarrotCodableKit/BetterCodable/LossyValue/LossyArray.swift(3 hunks)Sources/KarrotCodableKit/BetterCodable/LossyValue/LossyDictionary.swift(5 hunks)Sources/KarrotCodableKit/PolymorphicCodable/DefaultEmptyPolymorphicArrayValue.swift(1 hunks)Sources/KarrotCodableKit/PolymorphicCodable/Extensions/KeyedDecodingContainer+LossyOptionalPolymorphicValue.swift(1 hunks)Sources/KarrotCodableKit/PolymorphicCodable/Extensions/KeyedDecodingContainer+OptionalPolymorphicArrayValue.swift(1 hunks)Sources/KarrotCodableKit/PolymorphicCodable/LossyOptionalPolymorphicValue.swift(1 hunks)Sources/KarrotCodableKit/PolymorphicCodable/PolymorphicValue.swift(1 hunks)
💤 Files with no reviewable changes (1)
- Sources/KarrotCodableKit/BetterCodable/Extensions/Result+Extension.swift
🧰 Additional context used
📓 Path-based instructions (1)
Sources/KarrotCodableKit/**
⚙️ CodeRabbit configuration file
Sources/KarrotCodableKit/**: You are a senior Swift/iOS engineer reviewing runtime library code for KarrotCodableKit, a comprehensive Codable extension framework.1. Codable Performance [HIGH]
- Check encoding/decoding efficiency and memory usage
- Verify proper handling of large JSON structures
- Assess polymorphic type resolution performance
- Review property wrapper overhead and optimization
2. Type Safety & Polymorphism [HIGH]
- Validate PolymorphicCodable identifier-based type resolution
- Check AnyCodable type erasure implementation for edge cases
- Verify UnnestedPolymorphic macro integration with runtime components
- Assess error handling in polymorphic decoding scenarios
3. API Design [HIGH]
- Evaluate public interface consistency across modules
- Check property wrapper ergonomics (@DefaultFalse, @DATEvalue, etc.)
- Verify protocol design follows Swift API guidelines
- Assess extensibility for new Codable patterns
4. BetterCodable Integration [MEDIUM]
- Review property wrapper implementations for common patterns
- Check date strategy implementations (ISO8601, RFC3339, etc.)
- Verify default value and lossy conversion handling
- Assess data encoding strategies (Base64, etc.)
5. Error Handling [MEDIUM]
- Verify comprehensive DecodingError and EncodingError usage
- Check PolymorphicCodableError provides sufficient context
- Assess graceful fallback handling in polymorphic scenarios
Review Focus
- Prioritize runtime correctness and performance over style
- Focus on real-world JSON processing scenarios
- Mark comments with priority: [HIGH], [MEDIUM], or [LOW]
Files:
Sources/KarrotCodableKit/BetterCodable/DateValue/OptionalDateValue.swiftSources/KarrotCodableKit/BetterCodable/LosslessValue/OptionalLosslessValue.swiftSources/KarrotCodableKit/PolymorphicCodable/PolymorphicValue.swiftSources/KarrotCodableKit/BetterCodable/LosslessValue/LosslessValue.swiftSources/KarrotCodableKit/PolymorphicCodable/Extensions/KeyedDecodingContainer+LossyOptionalPolymorphicValue.swiftSources/KarrotCodableKit/BetterCodable/DataValue/DataValue.swiftSources/KarrotCodableKit/BetterCodable/DateValue/DateValue.swiftSources/KarrotCodableKit/PolymorphicCodable/DefaultEmptyPolymorphicArrayValue.swiftSources/KarrotCodableKit/PolymorphicCodable/Extensions/KeyedDecodingContainer+OptionalPolymorphicArrayValue.swiftSources/KarrotCodableKit/BetterCodable/LosslessValue/LosslessArray.swiftSources/KarrotCodableKit/BetterCodable/LossyValue/LossyArray.swiftSources/KarrotCodableKit/BetterCodable/LossyValue/LossyDictionary.swiftSources/KarrotCodableKit/PolymorphicCodable/LossyOptionalPolymorphicValue.swiftSources/KarrotCodableKit/BetterCodable/Defaults/DefaultCodable.swift
🧬 Code graph analysis (14)
Sources/KarrotCodableKit/BetterCodable/DateValue/OptionalDateValue.swift (1)
Sources/KarrotCodableKit/Resilient/ErrorReporting.swift (1)
reportError(154-166)
Sources/KarrotCodableKit/BetterCodable/LosslessValue/OptionalLosslessValue.swift (1)
Sources/KarrotCodableKit/Resilient/ErrorReporting.swift (1)
reportError(154-166)
Sources/KarrotCodableKit/PolymorphicCodable/PolymorphicValue.swift (1)
Sources/KarrotCodableKit/Resilient/ErrorReporting.swift (1)
reportError(154-166)
Sources/KarrotCodableKit/BetterCodable/LosslessValue/LosslessValue.swift (1)
Sources/KarrotCodableKit/Resilient/ErrorReporting.swift (1)
reportError(154-166)
Sources/KarrotCodableKit/PolymorphicCodable/Extensions/KeyedDecodingContainer+LossyOptionalPolymorphicValue.swift (2)
Sources/KarrotCodableKit/Resilient/ErrorReporting.swift (1)
reportError(154-166)Sources/KarrotCodableKit/Resilient/ResilientDecodingOutcome.swift (1)
recoveredFrom(46-46)
Sources/KarrotCodableKit/BetterCodable/DataValue/DataValue.swift (1)
Sources/KarrotCodableKit/Resilient/ErrorReporting.swift (1)
reportError(154-166)
Sources/KarrotCodableKit/BetterCodable/DateValue/DateValue.swift (1)
Sources/KarrotCodableKit/Resilient/ErrorReporting.swift (1)
reportError(154-166)
Sources/KarrotCodableKit/PolymorphicCodable/DefaultEmptyPolymorphicArrayValue.swift (2)
Sources/KarrotCodableKit/Resilient/ErrorReporting.swift (1)
reportError(154-166)Sources/KarrotCodableKit/Resilient/ResilientDecodingOutcome.swift (1)
recoveredFrom(46-46)
Sources/KarrotCodableKit/PolymorphicCodable/Extensions/KeyedDecodingContainer+OptionalPolymorphicArrayValue.swift (1)
Sources/KarrotCodableKit/Resilient/ErrorReporting.swift (1)
reportError(154-166)
Sources/KarrotCodableKit/BetterCodable/LosslessValue/LosslessArray.swift (1)
Sources/KarrotCodableKit/Resilient/ErrorReporting.swift (1)
reportError(154-166)
Sources/KarrotCodableKit/BetterCodable/LossyValue/LossyArray.swift (2)
Sources/KarrotCodableKit/Resilient/ErrorReporting.swift (1)
reportError(154-166)Sources/KarrotCodableKit/Resilient/ResilientDecodingOutcome.swift (1)
recoveredFrom(46-46)
Sources/KarrotCodableKit/BetterCodable/LossyValue/LossyDictionary.swift (2)
Sources/KarrotCodableKit/Resilient/ErrorReporting.swift (1)
reportError(154-166)Sources/KarrotCodableKit/Resilient/ResilientDecodingOutcome.swift (1)
recoveredFrom(46-46)
Sources/KarrotCodableKit/PolymorphicCodable/LossyOptionalPolymorphicValue.swift (2)
Sources/KarrotCodableKit/Resilient/ErrorReporting.swift (1)
reportError(154-166)Sources/KarrotCodableKit/Resilient/ResilientDecodingOutcome.swift (1)
recoveredFrom(46-46)
Sources/KarrotCodableKit/BetterCodable/Defaults/DefaultCodable.swift (3)
Sources/KarrotCodableKit/Resilient/ErrorReporting.swift (1)
reportError(154-166)Sources/KarrotCodableKit/BetterCodable/LossyValue/LossyDictionary.swift (1)
decode(273-295)Sources/KarrotCodableKit/Resilient/ResilientDecodingOutcome.swift (1)
recoveredFrom(46-46)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Check Macro Compatibility
- GitHub Check: Build and Test (16.4, debug)
- GitHub Check: Build and Test (16.4, release)
🔇 Additional comments (14)
Sources/KarrotCodableKit/PolymorphicCodable/DefaultEmptyPolymorphicArrayValue.swift (1)
47-52: Confirm DEBUG-only availability of$projectedValue.If teams rely on
$to inspect outcomes in production incidents, this will be unavailable. Consider exposing it in Release behind a dedicated feature flag instead of#if DEBUG.Priority: [LOW]
Sources/KarrotCodableKit/PolymorphicCodable/Extensions/KeyedDecodingContainer+OptionalPolymorphicArrayValue.swift (2)
37-56: Optional wrapper semantics: confirm throw vs recover [HIGH]For OptionalPolymorphicArrayValue, do we intend to propagate element/structure errors (current code throws) or recover to wrappedValue: nil with an outcome (e.g., .recoveredFrom(error))? The PR notes “Optional property behavior remains unchanged”; please confirm that “throw” is the established behavior for this wrapper. If recovery is desired, return a nil wrappedValue with an explicit outcome and only report under DEBUG.
Suggested checks:
- Ensure tests cover: missing key → .keyNotFound, explicit null → .valueWasNil, empty array → .decodedSuccessfully, malformed element → current intended behavior (throw vs recover).
- Verify call sites relying on optionality won’t unexpectedly hard-fail when a single element is bad.
16-21: Outcome coverage is consistent across paths [LOW]Key-not-found, value-nil, and success paths now all produce explicit outcomes. This improves downstream diagnostics. Nicely done.
Also applies to: 32-36, 48-49
Sources/KarrotCodableKit/BetterCodable/DataValue/DataValue.swift (1)
54-57: DEBUG-only error reporting is appropriate and consistent.Rethrow maintains behavior; gating avoids release overhead/noise. Looks good.
Priority: [LOW]
Sources/KarrotCodableKit/BetterCodable/DateValue/OptionalDateValue.swift (2)
56-59: Good: report only in DEBUG for inner decode failure.Behavior for optionals (nil becomes
.valueWasNil) remains intact; other errors still surface. LGTM.Priority: [LOW]
65-68: Good: DEBUG-gated outer error reporting.Release builds won’t attempt reporting; error still propagates. LGTM.
Priority: [LOW]
Sources/KarrotCodableKit/BetterCodable/DateValue/DateValue.swift (1)
54-57: LGTM: consistent DEBUG-only error reporting.Non-optional wrapper still throws on nil/missing as intended by the PR objective.
Priority: [LOW]
Sources/KarrotCodableKit/PolymorphicCodable/PolymorphicValue.swift (1)
59-62: LGTM: DEBUG-only reporting on polymorphic decode failures.Preserves throw semantics; avoids release-time reporter lookup.
Priority: [LOW]
Sources/KarrotCodableKit/BetterCodable/LossyValue/LossyDictionary.swift (1)
202-209: DEBUG error reporting paths look solidGood contextual
DecodingError.valueNotFound/catch reporting and correctwasReportedflagging. Matches the resilient pattern used elsewhere.Priority: [LOW]
Also applies to: 232-233
Sources/KarrotCodableKit/BetterCodable/Defaults/DefaultCodable.swift (2)
84-86: Top-level DEBUG recovery is consistentReporting the decode failure and returning
.recoveredFrom(_, wasReported: true)aligns with the resilient design.Priority: [LOW]
121-122: Docstring nit: fix “fallback” phrasingGood catch on the wording; reads clearly now.
Priority: [LOW]
Sources/KarrotCodableKit/BetterCodable/LosslessValue/OptionalLosslessValue.swift (1)
81-83: DEBUG-gated error reporting is appropriate hereKeeps release paths clean while still surfacing actionable failures in diagnostics builds.
Priority: [LOW]
Sources/KarrotCodableKit/BetterCodable/LosslessValue/LosslessValue.swift (2)
2-2: Header filename update looks good.
No functional impact.
[LOW]
68-71: DEBUG-gated error reporting is appropriate.
This matches the PR goal: development visibility without release noise; error is still thrown in both builds.
[MEDIUM]
| decoder.reportError(error) | ||
| self.init(wrappedValue: Default.defaultValue, outcome: .recoveredFrom(error, wasReported: true)) | ||
| #else |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider making DEBUG-only reporting togglable at runtime
A compile-time gate limits observability in TestFlight/production. Providing a runtime flag (e.g., via userInfo or an environment switch) would let clients opt into reporting in non-DEBUG builds without changing binaries.
Priority: [LOW]
Also applies to: 126-134, 142-149, 184-191, 200-207, 267-271, 279-285
🤖 Prompt for AI Agents
In Sources/KarrotCodableKit/BetterCodable/Defaults/DefaultCodable.swift around
lines 84-86 (and similarly at 126-134, 142-149, 184-191, 200-207, 267-271,
279-285), the decoder.reportError(...) calls are wrapped in a compile-time DEBUG
check; change this to consult a runtime-configurable flag instead so reporting
can be enabled outside DEBUG builds. Add a static/runtime-accessible toggle
(e.g., a Decoder.userInfo key, an environment switch, or a shared configuration
property) that the init paths read before calling decoder.reportError(...),
defaulting to the current DEBUG behavior; update each guarded location to check
the runtime flag and only call reportError when it is enabled. Ensure the new
flag is documented and thread-safe and that existing behavior remains unchanged
when the flag is not set.
| #if DEBUG | ||
| decoder.reportError(error) | ||
| results.append(.failure(error)) | ||
| #endif |
There was a problem hiding this comment.
🛠️ Refactor suggestion
[HIGH] Don’t gate error reporting behind DEBUG for element failures
decoder.reportError is already opt-in (no-op without a reporter). Gate recovery semantics if you want, but always report to keep production observability.
- #if DEBUG
- decoder.reportError(error)
- results.append(.failure(error))
- #endif
+ decoder.reportError(error)
+ #if DEBUG
+ results.append(.failure(error))
+ #endif📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #if DEBUG | |
| decoder.reportError(error) | |
| results.append(.failure(error)) | |
| #endif | |
| decoder.reportError(error) | |
| #if DEBUG | |
| results.append(.failure(error)) | |
| #endif |
🤖 Prompt for AI Agents
In Sources/KarrotCodableKit/BetterCodable/LosslessValue/LosslessArray.swift
around lines 60–63, the call to decoder.reportError(error) (and currently
results.append(.failure(error))) is wrapped in a #if DEBUG block;
unconditionally call decoder.reportError(error) so production observability
always receives element decoding errors, and if you want to keep recovery
semantics gated, keep only results.append(.failure(error)) inside the DEBUG
conditional (or adjust recovery gating as intended) — remove the #if DEBUG
around the report call and ensure the reporter invocation is executed in release
builds.
| #if DEBUG | ||
| elementDecoder.reportError(error) | ||
| results.append(.failure(error)) | ||
| #endif |
There was a problem hiding this comment.
🛠️ Refactor suggestion
[HIGH] Per-element error reporting should not be DEBUG-gated
Report element errors unconditionally; keep results only in DEBUG.
- #if DEBUG
- elementDecoder.reportError(error)
- results.append(.failure(error))
- #endif
+ elementDecoder.reportError(error)
+ #if DEBUG
+ results.append(.failure(error))
+ #endif📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #if DEBUG | |
| elementDecoder.reportError(error) | |
| results.append(.failure(error)) | |
| #endif | |
| // ... earlier in the loop | |
| - #if DEBUG | |
| - elementDecoder.reportError(error) | |
| - results.append(.failure(error)) | |
| elementDecoder.reportError(error) | |
| #if DEBUG | |
| results.append(.failure(error)) | |
| #endif | |
| // ... rest of loop |
🤖 Prompt for AI Agents
In Sources/KarrotCodableKit/BetterCodable/LossyValue/LossyArray.swift around
lines 80–83, move the elementDecoder.reportError(error) call out of the #if
DEBUG block so element errors are reported unconditionally, and keep
results.append(.failure(error)) inside the #if DEBUG block (ensure the results
array is only referenced/declared under DEBUG to avoid build errors); in short:
always call reportError(error), but only append .failure(error) to results when
DEBUG is enabled.
| decoder.reportError(error) | ||
| self.init(wrappedValue: [], outcome: .recoveredFrom(error, wasReported: true)) | ||
| #else |
There was a problem hiding this comment.
🛠️ Refactor suggestion
[HIGH] Outer array decoding errors should be reported in production too
Keep observability: always call decoder.reportError(error).
- #if DEBUG
- decoder.reportError(error)
- self.init(wrappedValue: [], outcome: .recoveredFrom(error, wasReported: true))
- #else
- self.init(wrappedValue: [])
- #endif
+ decoder.reportError(error)
+ #if DEBUG
+ self.init(wrappedValue: [], outcome: .recoveredFrom(error, wasReported: true))
+ #else
+ self.init(wrappedValue: [])
+ #endif🤖 Prompt for AI Agents
In Sources/KarrotCodableKit/BetterCodable/LossyValue/LossyArray.swift around
lines 99 to 101, the call to decoder.reportError(error) is currently guarded
(only executed in DEBUG), so outer array decoding errors are not reported in
production; ensure decoder.reportError(error) is always invoked when recovering
from an error by moving the decoder.reportError(error) call outside the
conditional (or duplicating it in both branches) before initializing with the
recovered empty array so the recovery still occurs but the error is always
reported.
| _ = try? container.decode(AnyDecodableValue.self, forKey: key) | ||
| let decoder = try? container.superDecoder(forKey: key) | ||
| decoder?.reportError(error) | ||
| #if DEBUG | ||
| decoder?.reportError(error) | ||
| state.results[castKey] = .failure(error) | ||
| #endif |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Gate the extra decoding attempt behind DEBUG to avoid release overhead
The _ = try? container.decode(AnyDecodableValue.self, forKey: key) is only for diagnostics. It does real work in release too. Guard it with #if DEBUG.
Priority: [LOW]
- _ = try? container.decode(AnyDecodableValue.self, forKey: key)
+ #if DEBUG
+ _ = try? container.decode(AnyDecodableValue.self, forKey: key)
+ #endif📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| _ = try? container.decode(AnyDecodableValue.self, forKey: key) | |
| let decoder = try? container.superDecoder(forKey: key) | |
| decoder?.reportError(error) | |
| #if DEBUG | |
| decoder?.reportError(error) | |
| state.results[castKey] = .failure(error) | |
| #endif | |
| #if DEBUG | |
| _ = try? container.decode(AnyDecodableValue.self, forKey: key) | |
| #endif | |
| let decoder = try? container.superDecoder(forKey: key) | |
| #if DEBUG | |
| decoder?.reportError(error) | |
| state.results[castKey] = .failure(error) | |
| #endif |
🤖 Prompt for AI Agents
In Sources/KarrotCodableKit/BetterCodable/LossyValue/LossyDictionary.swift
around lines 151 to 156, the diagnostic decoding call `_ = try?
container.decode(AnyDecodableValue.self, forKey: key)` is executed in release
builds causing unnecessary work; move that decode inside the existing `#if
DEBUG` block so it only runs in debug builds, i.e. wrap the decode with `#if
DEBUG`/`#endif` (or place it immediately before `decoder?.reportError(error)`
and `state.results[castKey] = .failure(error)`) so no extra decoding happens in
release.
| _ = try? container.decode(AnyDecodableValue.self, forKey: key) | ||
| #if DEBUG | ||
| let decoder = try? container.superDecoder(forKey: key) | ||
| decoder?.reportError(error) | ||
| #if DEBUG | ||
| state.results[castKey] = .failure(error) | ||
| #endif |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Mirror the superDecoder fallback in the Int-key path
Same best-effort reporting concern as the String-key path.
Priority: [MEDIUM]
- let decoder = try? container.superDecoder(forKey: key)
- decoder?.reportError(error)
+ ((try? container.superDecoder(forKey: key)) ?? (try? container.superDecoder()))?
+ .reportError(error)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| _ = try? container.decode(AnyDecodableValue.self, forKey: key) | |
| #if DEBUG | |
| let decoder = try? container.superDecoder(forKey: key) | |
| decoder?.reportError(error) | |
| #if DEBUG | |
| state.results[castKey] = .failure(error) | |
| #endif | |
| _ = try? container.decode(AnyDecodableValue.self, forKey: key) | |
| #if DEBUG | |
| ((try? container.superDecoder(forKey: key)) ?? (try? container.superDecoder()))? | |
| .reportError(error) | |
| state.results[castKey] = .failure(error) | |
| #endif |
🤖 Prompt for AI Agents
In Sources/KarrotCodableKit/BetterCodable/LossyValue/LossyDictionary.swift
around lines 176 to 181, the Int-key decoding path currently swallows decoding
errors without the same best-effort reporting used for String keys; replicate
the superDecoder fallback there: when a decode attempt throws, obtain a
superDecoder(forKey: key) in DEBUG, call decoder?.reportError(error), and set
state.results[castKey] = .failure(error) so the Int-key path reports errors just
like the String-key path.
| #if DEBUG | ||
| decoder.reportError(error) | ||
| self.wrappedValue = [] | ||
| self.outcome = .recoveredFrom(error, wasReported: true) | ||
| #else | ||
| self.wrappedValue = [] | ||
| self.outcome = .recoveredFrom(error, wasReported: false) | ||
| #endif |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Don’t gate error reporting behind DEBUG; drive wasReported from reporter presence.
This creates an observability gap in Release. reportError is already a no-op without a reporter; use reporter presence to set wasReported and always execute the call.
Priority: [HIGH]
- #if DEBUG
- decoder.reportError(error)
- self.wrappedValue = []
- self.outcome = .recoveredFrom(error, wasReported: true)
- #else
- self.wrappedValue = []
- self.outcome = .recoveredFrom(error, wasReported: false)
- #endif
+ let hasReporter = decoder.userInfo[.resilientDecodingErrorReporter] != nil
+ if hasReporter { decoder.reportError(error) }
+ self.wrappedValue = []
+ self.outcome = .recoveredFrom(error, wasReported: hasReporter)🤖 Prompt for AI Agents
In
Sources/KarrotCodableKit/PolymorphicCodable/DefaultEmptyPolymorphicArrayValue.swift
around lines 70 to 77, error reporting is currently gated by #if DEBUG which
prevents reporting in Release; always call decoder.reportError(error) and
compute wasReported from the presence of the reporter (e.g. let wasReported =
decoder.reporter != nil or decoder.hasReporter) then set self.wrappedValue = []
and self.outcome = .recoveredFrom(error, wasReported: wasReported); remove the
DEBUG conditional and ensure reportError is invoked unconditionally while
wasReported reflects actual reporter presence.
| #if DEBUG | ||
| /// Report error to resilient decoding error reporter | ||
| let decoder = try? superDecoder(forKey: key) | ||
| decoder?.reportError(error) | ||
| return LossyOptionalPolymorphicValue(wrappedValue: nil, outcome: .recoveredFrom(error, wasReported: true)) | ||
| #else | ||
| return LossyOptionalPolymorphicValue(wrappedValue: nil, outcome: .recoveredFrom(error, wasReported: false)) | ||
| #endif |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Outcome flag can be wrong if reporting fails; also avoid double superDecoder if possible.
wasReported is hardcoded to true even when superDecoder(forKey:) fails, leading to inaccurate telemetry.
Apply this minimal fix:
- /// Report error to resilient decoding error reporter
- let decoder = try? superDecoder(forKey: key)
- decoder?.reportError(error)
- return LossyOptionalPolymorphicValue(wrappedValue: nil, outcome: .recoveredFrom(error, wasReported: true))
+ /// Report error to resilient decoding error reporter (only if we can obtain a decoder)
+ if let reportingDecoder = try? superDecoder(forKey: key) {
+ reportingDecoder.reportError(error)
+ return LossyOptionalPolymorphicValue(wrappedValue: nil, outcome: .recoveredFrom(error, wasReported: true))
+ } else {
+ return LossyOptionalPolymorphicValue(wrappedValue: nil, outcome: .recoveredFrom(error, wasReported: false))
+ }Optional follow-up (outside this hunk): lift let decoder = try superDecoder(forKey: key) above the do so you can reuse the same decoder in both success and error paths and avoid calling superDecoder twice.
Priority: [MEDIUM]
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #if DEBUG | |
| /// Report error to resilient decoding error reporter | |
| let decoder = try? superDecoder(forKey: key) | |
| decoder?.reportError(error) | |
| return LossyOptionalPolymorphicValue(wrappedValue: nil, outcome: .recoveredFrom(error, wasReported: true)) | |
| #else | |
| return LossyOptionalPolymorphicValue(wrappedValue: nil, outcome: .recoveredFrom(error, wasReported: false)) | |
| #endif | |
| #if DEBUG | |
| /// Report error to resilient decoding error reporter (only if we can obtain a decoder) | |
| if let reportingDecoder = try? superDecoder(forKey: key) { | |
| reportingDecoder.reportError(error) | |
| return LossyOptionalPolymorphicValue(wrappedValue: nil, outcome: .recoveredFrom(error, wasReported: true)) | |
| } else { | |
| return LossyOptionalPolymorphicValue(wrappedValue: nil, outcome: .recoveredFrom(error, wasReported: false)) | |
| } | |
| #else | |
| return LossyOptionalPolymorphicValue(wrappedValue: nil, outcome: .recoveredFrom(error, wasReported: false)) | |
| #endif |
🤖 Prompt for AI Agents
In
Sources/KarrotCodableKit/PolymorphicCodable/Extensions/KeyedDecodingContainer+LossyOptionalPolymorphicValue.swift
around lines 43–50, the error path currently hardcodes wasReported: true even
when superDecoder(forKey:) returned nil; change the code to compute a Boolean
based on whether the decoder existed and reportError was actually invoked (e.g.,
let decoder = try? superDecoder(forKey: key); decoder?.reportError(error); let
reported = decoder != nil; return ... wasReported: reported). Optionally
(outside this hunk) move let decoder = try superDecoder(forKey: key) above the
do so the same decoder instance is reused in success and error paths to avoid
calling superDecoder twice.
| #if DEBUG | ||
| // Report the error through superDecoder | ||
| let decoder = try superDecoder(forKey: key) | ||
| decoder.reportError(error) | ||
| #endif | ||
| throw error |
There was a problem hiding this comment.
Don't let superDecoder(forKey:) mask the original error [HIGH]
If superDecoder(forKey:) throws here, it will escape this catch and replace the original decoding error. Report defensively and always rethrow the original error.
Apply:
- #if DEBUG
- // Report the error through superDecoder
- let decoder = try superDecoder(forKey: key)
- decoder.reportError(error)
- #endif
+ #if DEBUG
+ // Report without risking throw from superDecoder that could mask `error`
+ if let decoder = try? superDecoder(forKey: key) {
+ decoder.reportError(error)
+ } else {
+ assertionFailure("Failed to acquire superDecoder for error reporting at key: \(key.stringValue)")
+ }
+ #endif📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #if DEBUG | |
| // Report the error through superDecoder | |
| let decoder = try superDecoder(forKey: key) | |
| decoder.reportError(error) | |
| #endif | |
| throw error | |
| #if DEBUG | |
| // Report without risking throw from superDecoder that could mask `error` | |
| if let decoder = try? superDecoder(forKey: key) { | |
| decoder.reportError(error) | |
| } else { | |
| assertionFailure("Failed to acquire superDecoder for error reporting at key: \(key.stringValue)") | |
| } | |
| #endif | |
| throw error |
🤖 Prompt for AI Agents
In
Sources/KarrotCodableKit/PolymorphicCodable/Extensions/KeyedDecodingContainer+OptionalPolymorphicArrayValue.swift
around lines 50–55, the current DEBUG block calls superDecoder(forKey:) directly
which can throw and mask the original decoding error; change it to defensively
obtain the super decoder (e.g., use try? or wrap in a do-catch) so any failure
to get the super decoder is ignored, call decoder.reportError(error) only if you
successfully obtained a decoder, and then always rethrow the original error (do
not allow superDecoder's failure to escape).
| #if DEBUG | ||
| // Report error to resilient decoding error reporter | ||
| decoder.reportError(error) | ||
|
|
||
| self.wrappedValue = nil | ||
| self.outcome = .recoveredFrom(error, wasReported: true) | ||
| #else | ||
| self.outcome = .recoveredFrom(error, wasReported: false) | ||
| #endif |
There was a problem hiding this comment.
🛠️ Refactor suggestion
[HIGH] wrappedValue is uninitialized in non-DEBUG builds; also keep reporting unconditional
In the catch, wrappedValue isn’t set on the non-DEBUG path, which is a definite initialization bug. Also, error reporting should not be DEBUG-gated.
- #if DEBUG
- // Report error to resilient decoding error reporter
- decoder.reportError(error)
- self.wrappedValue = nil
- self.outcome = .recoveredFrom(error, wasReported: true)
- #else
- self.outcome = .recoveredFrom(error, wasReported: false)
- #endif
+ // Always report so production can observe; recovery remains build-config gated.
+ decoder.reportError(error)
+ self.wrappedValue = nil
+ #if DEBUG
+ self.outcome = .recoveredFrom(error, wasReported: true)
+ #else
+ self.outcome = .recoveredFrom(error, wasReported: false)
+ #endif🤖 Prompt for AI Agents
In
Sources/KarrotCodableKit/PolymorphicCodable/LossyOptionalPolymorphicValue.swift
around lines 68–75, the non-DEBUG path fails to initialize wrappedValue and
error reporting is incorrectly gated by DEBUG; always call
decoder.reportError(error), set self.wrappedValue = nil unconditionally before
setting outcome, and set outcome = .recoveredFrom(error, wasReported: true)
(since the error is always reported).
…llbacks in non-DEBUG builds.
ElonPark
force-pushed
the
feature/elon/change-ResilientDecodingOutcome-logic
branch
from
a267019 to
a89d94c
Compare
There was a problem hiding this comment.
Actionable comments posted: 20
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (11)
Sources/KarrotCodableKit/BetterCodable/DateValue/OptionalDateValue.swift (1)
102-114: Also report errors in thedecodeIfPresentspecialization (only on failure).Today, failures in
T.decode(_:)bypassreportErrorin this path. You can obtain a decoder viasuperDecoder(forKey:)only on error to keep the happy path zero-cost.Priority: [MEDIUM]
public func decodeIfPresent<T>( _ type: OptionalDateValue<T>.Type, forKey key: Self.Key ) throws -> OptionalDateValue<T> where T.RawValue == String { let stringOptionalValue = try decodeIfPresent(String.self, forKey: key) guard let stringValue = stringOptionalValue else { return .init(wrappedValue: nil) } - let dateValue = try T.decode(stringValue) - return .init(wrappedValue: dateValue) + do { + let dateValue = try T.decode(stringValue) + return .init(wrappedValue: dateValue) + } catch { + #if DEBUG + if let subDecoder = try? superDecoder(forKey: key) { + subDecoder.reportError(error) + } + #endif + throw error + } }If helpful, I can add a focused test that decodes this path and asserts digest presence only in DEBUG.
Sources/KarrotCodableKit/BetterCodable/LossyValue/LossyArray.swift (1)
31-35: [MEDIUM] Consider exposing projectedValue in Release (optional)Operationally useful: allowing
$array.outcomein Release can help field diagnostics without reporters. If you prefer keeping it DEBUG-only, ignore.Sources/KarrotCodableKit/BetterCodable/Defaults/DefaultCodable.swift (4)
124-137: Non-optional DefaultCodable must throw on missing key (no silent defaulting)Current code returns a default. Per PR objective, this should throw; DEBUG may report before throwing.
Priority: [HIGH]
- if !contains(key) { - #if DEBUG - let context = DecodingError.Context( - codingPath: codingPath + [key], - debugDescription: "Key not found but property is non-optional" - ) - let error = DecodingError.keyNotFound(key, context) - let decoder = try? superDecoder(forKey: key) - decoder?.reportError(error) - return DefaultCodable(wrappedValue: P.defaultValue, outcome: .recoveredFrom(error, wasReported: decoder != nil)) - #else - return DefaultCodable(wrappedValue: P.defaultValue) - #endif - } + if !contains(key) { + let context = DecodingError.Context( + codingPath: codingPath + [key], + debugDescription: "Key not found but property is non-optional" + ) + let error = DecodingError.keyNotFound(key, context) + #if DEBUG + let reportingDecoder = ( (try? superDecoder(forKey: key)) ?? (try? superDecoder()) ) + reportingDecoder?.reportError(error) + #endif + throw error + }
140-153: Non-optional DefaultCodable must throw on nil valueReturn default → throw valueNotFound for the actual value type; DEBUG may report.
Priority: [HIGH]
- if (try? decodeNil(forKey: key)) == true { - #if DEBUG - let context = DecodingError.Context( - codingPath: codingPath + [key], - debugDescription: "Value was nil but property is non-optional" - ) - let error = DecodingError.valueNotFound(P.DefaultValue.self, context) - let decoder = try? superDecoder(forKey: key) - decoder?.reportError(error) - return DefaultCodable(wrappedValue: P.defaultValue, outcome: .recoveredFrom(error, wasReported: decoder != nil)) - #else - return DefaultCodable(wrappedValue: P.defaultValue) - #endif - } + if (try? decodeNil(forKey: key)) == true { + let context = DecodingError.Context( + codingPath: codingPath + [key], + debugDescription: "Value was nil but property is non-optional" + ) + let error = DecodingError.valueNotFound(P.DefaultValue.self, context) + #if DEBUG + let reportingDecoder = ( (try? superDecoder(forKey: key)) ?? (try? superDecoder()) ) + reportingDecoder?.reportError(error) + #endif + throw error + }
182-195: Bool strategy: throw on missing key (non-optional)Mirror generic path: report in DEBUG, then throw.
Priority: [HIGH]
- if !contains(key) { - #if DEBUG - let context = DecodingError.Context( - codingPath: codingPath + [key], - debugDescription: "Key not found but property is non-optional" - ) - let error = DecodingError.keyNotFound(key, context) - let decoder = try? superDecoder(forKey: key) - decoder?.reportError(error) - return DefaultCodable(wrappedValue: P.defaultValue, outcome: .recoveredFrom(error, wasReported: decoder != nil)) - #else - return DefaultCodable(wrappedValue: P.defaultValue) - #endif - } + if !contains(key) { + let context = DecodingError.Context( + codingPath: codingPath + [key], + debugDescription: "Key not found but property is non-optional" + ) + let error = DecodingError.keyNotFound(key, context) + #if DEBUG + let reportingDecoder = ( (try? superDecoder(forKey: key)) ?? (try? superDecoder()) ) + reportingDecoder?.reportError(error) + #endif + throw error + }
198-211: Bool strategy: throw on nil (non-optional)Return default → throw valueNotFound(Bool.self, …).
Priority: [HIGH]
- if (try? decodeNil(forKey: key)) == true { - #if DEBUG - let context = DecodingError.Context( - codingPath: codingPath + [key], - debugDescription: "Value was nil but property is non-optional" - ) - let error = DecodingError.valueNotFound(Bool.self, context) - let decoder = try? superDecoder(forKey: key) - decoder?.reportError(error) - return DefaultCodable(wrappedValue: P.defaultValue, outcome: .recoveredFrom(error, wasReported: decoder != nil)) - #else - return DefaultCodable(wrappedValue: P.defaultValue) - #endif - } + if (try? decodeNil(forKey: key)) == true { + let context = DecodingError.Context( + codingPath: codingPath + [key], + debugDescription: "Value was nil but property is non-optional" + ) + let error = DecodingError.valueNotFound(Bool.self, context) + #if DEBUG + let reportingDecoder = ( (try? superDecoder(forKey: key)) ?? (try? superDecoder()) ) + reportingDecoder?.reportError(error) + #endif + throw error + }Sources/KarrotCodableKit/BetterCodable/LosslessValue/OptionalLosslessValue.swift (1)
169-182: Catch and convert decodeIfPresent errors to nil outcome
decodeIfPresent(_:forKey:)throwsDecodingError.typeMismatchwhen the stored value can’t be converted to the requested type, causing this optional wrapper to fail fast instead of returningnil(swiftdoc.org). Wrap the call indo-catchto return.valueWasNil(or record and recover via.recoveredFrom(error)) rather than propagating errors. Example:- if let value = try decodeIfPresent(type, forKey: key) { - return value - } - - return OptionalLosslessValueCodable<T>(wrappedValue: nil, outcome: .valueWasNil, type: nil) + do { + if let value = try decodeIfPresent(type, forKey: key) { + return value + } + return OptionalLosslessValueCodable<T>(wrappedValue: nil, outcome: .valueWasNil, type: nil) + } catch { + #if DEBUG + let decoder = try? superDecoder(forKey: key) + decoder?.reportError(error) + return OptionalLosslessValueCodable<T>( + wrappedValue: nil, + outcome: .recoveredFrom(error, wasReported: decoder != nil), + type: nil + ) + #else + return OptionalLosslessValueCodable<T>(wrappedValue: nil, outcome: .valueWasNil, type: nil) + #endif + }Sources/KarrotCodableKit/BetterCodable/LosslessValue/LosslessArray.swift (1)
67-73: Mark wasReported accurately when failures occur. [MEDIUM]If errors were reported, reflect it in
outcome.#if DEBUG if elements.count == results.count { self.init(wrappedValue: elements, outcome: .decodedSuccessfully) } else { - let error = ResilientDecodingOutcome.ArrayDecodingError(results: results) - self.init(wrappedValue: elements, outcome: .recoveredFrom(error, wasReported: false)) + let error = ResilientDecodingOutcome.ArrayDecodingError(results: results) + let wasReported = results.contains { if case .failure = $0 { true } else { false } } + self.init(wrappedValue: elements, outcome: .recoveredFrom(error, wasReported: wasReported)) } #elseSources/KarrotCodableKit/PolymorphicCodable/DefaultEmptyPolymorphicArrayValue.swift (1)
11-28: Doc comment contradicts implementation (throws vs recover).The code always recovers to [] on any error, while docs say it throws on element failure and mentions using print. Align docs with behavior (and reporter use).
Priority: [HIGH]
-/// - **Crucially, if *any* element within the array fails to decode according to the `PolymorphicType` strategy, the entire decoding process for the wrapper fails, and an error is thrown.** This wrapper does **not** skip invalid elements. -/// - If the initial step of obtaining the unkeyed container fails (e.g., the key is missing in the parent JSON object, or the corresponding value is `null` or not an array), it catches the error, assigns `[]` to `wrappedValue`, and logs the error using `print`. +/// - If obtaining the unkeyed container fails (missing key, `null`, or not an array) or any element fails to decode for the `PolymorphicType` strategy, +/// this wrapper recovers by setting `wrappedValue` to `[]` and records the error via the decoder’s resilient error reporter when installed. +/// This wrapper does not skip invalid elements.Sources/KarrotCodableKit/PolymorphicCodable/Extensions/KeyedDecodingContainer+LossyOptionalPolymorphicValue.swift (2)
16-21: Missing returns cause compile error in decode(_:forKey:)Both branches need explicit return statements.
Priority: [HIGH]- if let value = try decodeIfPresent(type, forKey: key) { - value - } else { - LossyOptionalPolymorphicValue(wrappedValue: nil, outcome: .keyNotFound) - } + if let value = try decodeIfPresent(type, forKey: key) { + return value + } else { + return LossyOptionalPolymorphicValue(wrappedValue: nil, outcome: .keyNotFound) + }
37-50: Optional: avoid double superDecoder(forKey:) to reduce overheadCapture the nested decoder once and reuse it for both decode and error reporting. This avoids creating two nested decoders at the same key.
Priority: [MEDIUM]- do { - let decoder = try superDecoder(forKey: key) - let value = try T.decode(from: decoder) - return LossyOptionalPolymorphicValue(wrappedValue: value, outcome: .decodedSuccessfully) - } catch { - #if DEBUG - /// Report error to resilient decoding error reporter - let decoder = try? superDecoder(forKey: key) - decoder?.reportError(error) - return LossyOptionalPolymorphicValue(wrappedValue: nil, outcome: .recoveredFrom(error, wasReported: true)) - #else - return LossyOptionalPolymorphicValue(wrappedValue: nil, outcome: .recoveredFrom(error, wasReported: false)) - #endif - } + var reportingDecoder: Decoder? + do { + let decoder = try superDecoder(forKey: key) + reportingDecoder = decoder + let value = try T.decode(from: decoder) + return LossyOptionalPolymorphicValue(wrappedValue: value, outcome: .decodedSuccessfully) + } catch { + #if DEBUG + reportingDecoder?.reportError(error) + let wasReported = (reportingDecoder != nil) + return LossyOptionalPolymorphicValue(wrappedValue: nil, outcome: .recoveredFrom(error, wasReported: wasReported)) + #else + return LossyOptionalPolymorphicValue(wrappedValue: nil, outcome: .recoveredFrom(error, wasReported: false)) + #endif + }
♻️ Duplicate comments (21)
Sources/KarrotCodableKit/PolymorphicCodable/LossyOptionalPolymorphicValue.swift (1)
68-75: [HIGH] Definite initialization bug in non-DEBUG; and don’t gate reporting behind DEBUG
wrappedValueisn’t set on the non-DEBUG path (compile-time init error). Also, error reporting should not be DEBUG-gated so production can observe failures when a reporter is installed.- #if DEBUG - // Report error to resilient decoding error reporter - decoder.reportError(error) - self.wrappedValue = nil - self.outcome = .recoveredFrom(error, wasReported: true) - #else - self.outcome = .recoveredFrom(error, wasReported: false) - #endif + // Always report so production can observe; recover to nil + decoder.reportError(error) + self.wrappedValue = nil + self.outcome = .recoveredFrom(error, wasReported: true)Sources/KarrotCodableKit/BetterCodable/LossyValue/LossyArray.swift (3)
47-54: [HIGH] Always report nil-array decoding errors (observability)Move
decoder.reportError(error)out of the DEBUG block so production captures these issues when a reporter is set.- #if DEBUG - let context = DecodingError.Context( - codingPath: decoder.codingPath, - debugDescription: "Value was nil but property is non-optional" - ) - let error = DecodingError.valueNotFound([T].self, context) - decoder.reportError(error) - self.init(wrappedValue: [], outcome: .recoveredFrom(error, wasReported: true)) - #else - self.init(wrappedValue: []) - #endif + let context = DecodingError.Context( + codingPath: decoder.codingPath, + debugDescription: "Value was nil but property is non-optional" + ) + let error = DecodingError.valueNotFound([T].self, context) + decoder.reportError(error) + #if DEBUG + self.init(wrappedValue: [], outcome: .recoveredFrom(error, wasReported: true)) + #else + self.init(wrappedValue: []) + #endif
81-83: [HIGH] Per-element errors should be reported in Release tooReport each element error unconditionally; keep
results.append(.failure(...))under DEBUG.- #if DEBUG - elementDecoder.reportError(error) - results.append(.failure(error)) - #endif + elementDecoder.reportError(error) + #if DEBUG + results.append(.failure(error)) + #endif
99-101: [HIGH] Report outer array decoding failures in all buildsEnsure
decoder.reportError(error)is invoked outside DEBUG for observability.- #if DEBUG - decoder.reportError(error) - self.init(wrappedValue: [], outcome: .recoveredFrom(error, wasReported: true)) - #else - self.init(wrappedValue: []) - #endif + decoder.reportError(error) + #if DEBUG + self.init(wrappedValue: [], outcome: .recoveredFrom(error, wasReported: true)) + #else + self.init(wrappedValue: []) + #endifSources/KarrotCodableKit/BetterCodable/Defaults/DefaultCodable.swift (3)
155-171: Unreachable else-branch; removeAfter contains+nil checks, decodeIfPresent won’t return nil. Keep the fast path only.
Priority: [MEDIUM]
- if let value = try decodeIfPresent(DefaultCodable<P>.self, forKey: key) { - return value - } else { - #if DEBUG - let context = DecodingError.Context( - codingPath: codingPath + [key], - debugDescription: "Key not found but property is non-optional" - ) - let error = DecodingError.keyNotFound(key, context) - let decoder = try? superDecoder(forKey: key) - decoder?.reportError(error) - return DefaultCodable(wrappedValue: P.defaultValue, outcome: .recoveredFrom(error, wasReported: decoder != nil)) - #else - return DefaultCodable(wrappedValue: P.defaultValue) - #endif - } + if let value = try decodeIfPresent(DefaultCodable<P>.self, forKey: key) { + return value + }
267-275: RawRepresentable path: wrong messages/types and should throw (not default)
- Update messages to the non-optional wording.
- valueNotFound should use P.DefaultValue.self (not P.self).
- Throw after optional reporting.
Priority: [HIGH]
- #if DEBUG - let context = DecodingError.Context(codingPath: codingPath + [key], debugDescription: "Key not found") - let error = DecodingError.keyNotFound(key, context) - let decoder = try? superDecoder(forKey: key) - decoder?.reportError(error) - return DefaultCodable(wrappedValue: P.defaultValue, outcome: .recoveredFrom(error, wasReported: decoder != nil)) - #else - return DefaultCodable(wrappedValue: P.defaultValue) - #endif + let context = DecodingError.Context( + codingPath: codingPath + [key], + debugDescription: "Key not found but property is non-optional" + ) + let error = DecodingError.keyNotFound(key, context) + #if DEBUG + let reportingDecoder = ( (try? superDecoder(forKey: key)) ?? (try? superDecoder()) ) + reportingDecoder?.reportError(error) + #endif + throw error @@ - #if DEBUG - let context = DecodingError.Context(codingPath: codingPath + [key], debugDescription: "Value was nil") - let error = DecodingError.valueNotFound(P.self, context) - let decoder = try? superDecoder(forKey: key) - decoder?.reportError(error) - return DefaultCodable(wrappedValue: P.defaultValue, outcome: .recoveredFrom(error, wasReported: decoder != nil)) - #else - return DefaultCodable(wrappedValue: P.defaultValue) - #endif + let context = DecodingError.Context( + codingPath: codingPath + [key], + debugDescription: "Value was nil but property is non-optional" + ) + let error = DecodingError.valueNotFound(P.DefaultValue.self, context) + #if DEBUG + let reportingDecoder = ( (try? superDecoder(forKey: key)) ?? (try? superDecoder()) ) + reportingDecoder?.reportError(error) + #endif + throw errorAlso applies to: 279-287
131-133: Prefer safe nested-decoder acquisition: try forKey OR fallback to superDecoder()Avoid throwing from superDecoder(forKey:) on missing keys; use optional acquisition with fallback.
Priority: [MEDIUM]
- let decoder = try? superDecoder(forKey: key) + let decoder = ( (try? superDecoder(forKey: key)) ?? (try? superDecoder()) )Also applies to: 147-149, 165-167, 189-191, 205-207, 223-225, 244-248, 269-271, 283-285, 317-319
Tests/KarrotCodableKitTests/BetterCodable/Defaults/DefaultCodableResilientTests.swift (4)
81-85: Rename test to reflect DEBUG error reporting and spot-check error kind. [HIGH]Missing keys do report errors in DEBUG per PR goals. Rename the test/description and optionally assert
keyNotFoundon one field.- @Test("missing keys use default values without error") - func missingKeysUseDefaultValues() throws { + @Test("missing keys use default values and report errors in DEBUG") + func missingKeysUseDefaultValues_reportErrorsInDebug() throws { @@ #if DEBUG #expect(fixture.$intValue.error != nil) #expect(fixture.$stringValue.error != nil) #expect(fixture.$boolValue.error != nil) #expect(fixture.$arrayValue.error != nil) #expect(fixture.$dictValue.error != nil) + // (Optional) verify error kind for precision + if let err = fixture.$intValue.error as? DecodingError { + if case .keyNotFound = err { } else { Issue.record("Expected keyNotFound for intValue") } + } #endifAlso applies to: 65-66
146-150: Nulls: assert valueNotFound to harden expectations. [MEDIUM]Add at least one error-kind check to guard mapping drift.
#if DEBUG #expect(fixture.$intValue.error != nil) #expect(fixture.$stringValue.error != nil) #expect(fixture.$boolValue.error != nil) #expect(fixture.$arrayValue.error != nil) #expect(fixture.$dictValue.error != nil) + if let err = fixture.$stringValue.error as? DecodingError { + if case .valueNotFound = err { } else { Issue.record("Expected valueNotFound for stringValue=null") } + } #endif
343-347: Resilient decode (missing keys): assert digest contents, not just non-nil. [MEDIUM]Validate we recorded both missing fields.
#if DEBUG - #expect(errorDigest != nil) + #expect(errorDigest != nil) + #expect(errorDigest?.errors.count == 2) #expect(fixture.$normalEnum.error != nil) #expect(fixture.$frozenEnum.error != nil) #endifAlso applies to: 354-356
373-377: Resilient decode (nulls): assert digest size; spot-check error kind. [MEDIUM]Mirror missing-keys checks and verify a
valueNotFoundon one field.#if DEBUG - #expect(errorDigest != nil) + #expect(errorDigest != nil) + #expect(errorDigest?.errors.count == 2) + if let err = fixture.$normalEnum.error as? DecodingError { + if case .valueNotFound = err { } else { Issue.record("Expected valueNotFound for normalEnum=null") } + } #expect(fixture.$normalEnum.error != nil) #expect(fixture.$frozenEnum.error != nil) #endifAlso applies to: 384-386
Sources/KarrotCodableKit/BetterCodable/LosslessValue/LosslessArray.swift (1)
61-63: Do not DEBUG-gate error reporting for element failures. [HIGH]Always call
decoder.reportError(error); keep result-tracking under DEBUG if desired.- #if DEBUG - decoder.reportError(error) - results.append(.failure(error)) - #endif + decoder.reportError(error) + #if DEBUG + results.append(.failure(error)) + #endifSources/KarrotCodableKit/PolymorphicCodable/DefaultEmptyPolymorphicArrayValue.swift (1)
70-77: Don’t gate reporting on DEBUG; drive wasReported from reporter presence.Release builds lose observability; compute wasReported via reporter presence and report when available.
Priority: [HIGH]
- #if DEBUG - decoder.reportError(error) - self.wrappedValue = [] - self.outcome = .recoveredFrom(error, wasReported: true) - #else - self.wrappedValue = [] - self.outcome = .recoveredFrom(error, wasReported: false) - #endif + let hasReporter = decoder.userInfo[.resilientDecodingErrorReporter] != nil + if hasReporter { decoder.reportError(error) } + self.wrappedValue = [] + self.outcome = .recoveredFrom(error, wasReported: hasReporter)Note: Tests asserting nil digest in non-DEBUG would need to be updated if you adopt this change.
Sources/KarrotCodableKit/PolymorphicCodable/Extensions/KeyedDecodingContainer+LossyOptionalPolymorphicValue.swift (1)
43-50: Don’t hardcode wasReported: true; compute based on successful reporting and avoid masking errorsIf superDecoder(forKey:) fails, we currently still mark wasReported = true. Use try? and derive the flag from decoder acquisition.
Priority: [HIGH]- #if DEBUG - /// Report error to resilient decoding error reporter - let decoder = try? superDecoder(forKey: key) - decoder?.reportError(error) - return LossyOptionalPolymorphicValue(wrappedValue: nil, outcome: .recoveredFrom(error, wasReported: true)) - #else - return LossyOptionalPolymorphicValue(wrappedValue: nil, outcome: .recoveredFrom(error, wasReported: false)) - #endif + #if DEBUG + // Report only if we can acquire a nested decoder; reflect that in `wasReported` + let reportingDecoder = try? superDecoder(forKey: key) + reportingDecoder?.reportError(error) + let wasReported = (reportingDecoder != nil) + return LossyOptionalPolymorphicValue(wrappedValue: nil, outcome: .recoveredFrom(error, wasReported: wasReported)) + #else + return LossyOptionalPolymorphicValue(wrappedValue: nil, outcome: .recoveredFrom(error, wasReported: false)) + #endifTests/KarrotCodableKitTests/PolymorphicCodable/ArrayValue/DefaultEmptyPolymorphicArrayValueResilientTests.swift (2)
119-121: Also assert recoveredFrom outcome for missing key (behavior parity with invalidType block)Mirror the assertion used in invalidType() so tests verify outcome, not just error presence.
Priority: [HIGH]#if DEBUG // missing key should error for non-optional property #expect(result.$notices.error != nil) +if case .recoveredFrom = result.$notices.outcome { + // Expected +} else { + Issue.record("Expected recoveredFrom outcome") +} #endif
140-142: Also assert recoveredFrom outcome for null valueAlign with invalidType() to validate the outcome for the null case.
Priority: [HIGH]#if DEBUG // null value should error for non-optional property #expect(result.$notices.error != nil) +if case .recoveredFrom = result.$notices.outcome { + // Expected +} else { + Issue.record("Expected recoveredFrom outcome") +} #endifSources/KarrotCodableKit/PolymorphicCodable/Extensions/KeyedDecodingContainer+OptionalPolymorphicArrayValue.swift (1)
50-55: Don’t let superDecoder(forKey:) in catch mask the original errorAcquire the nested decoder defensively; always rethrow the original error.
Priority: [HIGH]- #if DEBUG - // Report the error through superDecoder - let decoder = try superDecoder(forKey: key) - decoder.reportError(error) - #endif + #if DEBUG + // Report without risking a throw here that would shadow `error` + if let decoder = try? superDecoder(forKey: key) { + decoder.reportError(error) + } else { + assertionFailure("Failed to acquire superDecoder for error reporting at key: \(key.stringValue)") + } + #endifTests/KarrotCodableKitTests/PolymorphicCodable/ArrayValue/PolymorphicLossyArrayValueResilientTests.swift (1)
137-139: [HIGH] Also assert the recoveredFrom outcome for missing key (parity with invalidType).You assert error != nil, but don’t verify the outcome. Mirror the check used in Lines 159–166 so we don’t regress on outcome semantics.
#if DEBUG // missing key should error for non-optional property #expect(result.$notices.error != nil) +if case .recoveredFrom = result.$notices.outcome { + // Expected +} else { + Issue.record("Expected recoveredFrom outcome") +} #expect(result.$notices.results.isEmpty) #endifTests/KarrotCodableKitTests/BetterCodable/LossyValue/LossyDictionaryResilientTests.swift (1)
153-156: [HIGH] Missing keys: assert concrete error kind (keyNotFound).Non-optional dictionaries should surface DecodingError.keyNotFound; assert the specific kind to prevent regressions.
#if DEBUG // Missing key should error for non-optional properties #expect(fixture.$stringDict.error != nil) #expect(fixture.$intDict.error != nil) #expect(fixture.$objectDict.error != nil) + +if let err = fixture.$stringDict.error as? DecodingError { + if case .keyNotFound = err { + // expected + } else { + Issue.record("Expected keyNotFound for missing stringDict") + } +} +if let err = fixture.$intDict.error as? DecodingError { + if case .keyNotFound = err { + // expected + } else { + Issue.record("Expected keyNotFound for missing intDict") + } +} +if let err = fixture.$objectDict.error as? DecodingError { + if case .keyNotFound = err { + // expected + } else { + Issue.record("Expected keyNotFound for missing objectDict") + } +} #endifSources/KarrotCodableKit/BetterCodable/LossyValue/LossyDictionary.swift (2)
151-157: [HIGH] Gate diagnostic decode behind DEBUG and make error-reporting decoder resilient.The extra
_ = try? container.decode(AnyDecodableValue.self, ...)does real work in Release. Also,superDecoder(forKey:)can fail; fall back tosuperDecoder()to avoid losing reports.- } catch { - _ = try? container.decode(AnyDecodableValue.self, forKey: key) - let decoder = try? container.superDecoder(forKey: key) - #if DEBUG - decoder?.reportError(error) - state.results[castKey] = .failure(error) - #endif - } + } catch { + #if DEBUG + _ = try? container.decode(AnyDecodableValue.self, forKey: key) + let reportingDecoder = ((try? container.superDecoder(forKey: key)) ?? (try? container.superDecoder())) + reportingDecoder?.reportError(error) + state.results[castKey] = .failure(error) + #endif + }
176-181: [HIGH] Mirror DEBUG-gating and resilient superDecoder fallback for Int-key path.Keep parity with the String-key path and avoid doing work in Release.
- } catch { - _ = try? container.decode(AnyDecodableValue.self, forKey: key) - #if DEBUG - let decoder = try? container.superDecoder(forKey: key) - decoder?.reportError(error) - state.results[castKey] = .failure(error) - #endif - } + } catch { + #if DEBUG + _ = try? container.decode(AnyDecodableValue.self, forKey: key) + let reportingDecoder = ((try? container.superDecoder(forKey: key)) ?? (try? container.superDecoder())) + reportingDecoder?.reportError(error) + state.results[castKey] = .failure(error) + #endif + }
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (28)
Sources/KarrotCodableKit/BetterCodable/DataValue/DataValue.swift(1 hunks)Sources/KarrotCodableKit/BetterCodable/DateValue/DateValue.swift(1 hunks)Sources/KarrotCodableKit/BetterCodable/DateValue/OptionalDateValue.swift(1 hunks)Sources/KarrotCodableKit/BetterCodable/Defaults/DefaultCodable.swift(11 hunks)Sources/KarrotCodableKit/BetterCodable/Extensions/Result+Extension.swift(0 hunks)Sources/KarrotCodableKit/BetterCodable/LosslessValue/LosslessArray.swift(1 hunks)Sources/KarrotCodableKit/BetterCodable/LosslessValue/LosslessValue.swift(2 hunks)Sources/KarrotCodableKit/BetterCodable/LosslessValue/OptionalLosslessValue.swift(1 hunks)Sources/KarrotCodableKit/BetterCodable/LossyValue/LossyArray.swift(3 hunks)Sources/KarrotCodableKit/BetterCodable/LossyValue/LossyDictionary.swift(5 hunks)Sources/KarrotCodableKit/PolymorphicCodable/DefaultEmptyPolymorphicArrayValue.swift(1 hunks)Sources/KarrotCodableKit/PolymorphicCodable/Extensions/KeyedDecodingContainer+LossyOptionalPolymorphicValue.swift(1 hunks)Sources/KarrotCodableKit/PolymorphicCodable/Extensions/KeyedDecodingContainer+OptionalPolymorphicArrayValue.swift(1 hunks)Sources/KarrotCodableKit/PolymorphicCodable/LossyOptionalPolymorphicValue.swift(1 hunks)Sources/KarrotCodableKit/PolymorphicCodable/PolymorphicValue.swift(1 hunks)Tests/KarrotCodableKitTests/BetterCodable/DataValue/DataValueResilientTests.swift(1 hunks)Tests/KarrotCodableKitTests/BetterCodable/DateValue/DateValueResilientTests.swift(1 hunks)Tests/KarrotCodableKitTests/BetterCodable/Defaults/DefaultCodableResilientTests.swift(5 hunks)Tests/KarrotCodableKitTests/BetterCodable/LosslessValue/LosslessArrayResilientTests.swift(2 hunks)Tests/KarrotCodableKitTests/BetterCodable/LosslessValue/LosslessValueResilientTests.swift(1 hunks)Tests/KarrotCodableKitTests/BetterCodable/LossyValue/LossyArrayResilientTests.swift(3 hunks)Tests/KarrotCodableKitTests/BetterCodable/LossyValue/LossyDictionaryResilientTests.swift(3 hunks)Tests/KarrotCodableKitTests/PolymorphicCodable/ArrayValue/DefaultEmptyPolymorphicArrayValueResilientTests.swift(4 hunks)Tests/KarrotCodableKitTests/PolymorphicCodable/ArrayValue/PolymorphicArrayValueResilientTests.swift(2 hunks)Tests/KarrotCodableKitTests/PolymorphicCodable/ArrayValue/PolymorphicLossyArrayValueResilientTests.swift(3 hunks)Tests/KarrotCodableKitTests/PolymorphicCodable/OptionalPolymorphicArrayValueResilientTests.swift(3 hunks)Tests/KarrotCodableKitTests/PolymorphicCodable/OptionalValue/LossyOptionalPolymorphicValueResilientTests.swift(2 hunks)Tests/KarrotCodableKitTests/PolymorphicCodable/Value/PolymorphicValueResilientTests.swift(2 hunks)
💤 Files with no reviewable changes (1)
- Sources/KarrotCodableKit/BetterCodable/Extensions/Result+Extension.swift
🧰 Additional context used
📓 Path-based instructions (2)
Tests/KarrotCodableKitTests/**
⚙️ CodeRabbit configuration file
Tests/KarrotCodableKitTests/**: You are a senior Swift/iOS engineer reviewing runtime functionality tests for KarrotCodableKit's Codable extensions.1. Runtime Behavior Testing [HIGH]
- Verify JSON encoding/decoding scenarios cover real-world use cases
- Check polymorphic type resolution testing across different strategies
- Assess property wrapper testing (@DATEvalue, @DefaultFalse, etc.)
- Validate error handling and edge case coverage
2. Test Data Quality [HIGH]
- Review test JSON structures for realistic complexity
- Check that TestDoubles provide comprehensive mock scenarios
- Verify test data covers various data types and edge cases
- Assess polymorphic test data represents actual usage patterns
3. Integration Testing [MEDIUM]
- Check integration between macro-generated code and runtime functionality
- Verify end-to-end scenarios combining multiple KarrotCodableKit features
- Assess performance testing for large data structures
- Review memory usage testing for complex polymorphic scenarios
Review Focus
- Prioritize real-world usage scenario coverage
- Focus on Codable correctness and performance implications
- Mark comments with priority: [HIGH], [MEDIUM], or [LOW]
Files:
Tests/KarrotCodableKitTests/BetterCodable/LosslessValue/LosslessValueResilientTests.swiftTests/KarrotCodableKitTests/BetterCodable/DateValue/DateValueResilientTests.swiftTests/KarrotCodableKitTests/PolymorphicCodable/ArrayValue/PolymorphicArrayValueResilientTests.swiftTests/KarrotCodableKitTests/BetterCodable/DataValue/DataValueResilientTests.swiftTests/KarrotCodableKitTests/PolymorphicCodable/OptionalValue/LossyOptionalPolymorphicValueResilientTests.swiftTests/KarrotCodableKitTests/PolymorphicCodable/OptionalPolymorphicArrayValueResilientTests.swiftTests/KarrotCodableKitTests/PolymorphicCodable/ArrayValue/PolymorphicLossyArrayValueResilientTests.swiftTests/KarrotCodableKitTests/PolymorphicCodable/Value/PolymorphicValueResilientTests.swiftTests/KarrotCodableKitTests/BetterCodable/LossyValue/LossyDictionaryResilientTests.swiftTests/KarrotCodableKitTests/BetterCodable/LosslessValue/LosslessArrayResilientTests.swiftTests/KarrotCodableKitTests/BetterCodable/LossyValue/LossyArrayResilientTests.swiftTests/KarrotCodableKitTests/BetterCodable/Defaults/DefaultCodableResilientTests.swiftTests/KarrotCodableKitTests/PolymorphicCodable/ArrayValue/DefaultEmptyPolymorphicArrayValueResilientTests.swift
Sources/KarrotCodableKit/**
⚙️ CodeRabbit configuration file
Sources/KarrotCodableKit/**: You are a senior Swift/iOS engineer reviewing runtime library code for KarrotCodableKit, a comprehensive Codable extension framework.1. Codable Performance [HIGH]
- Check encoding/decoding efficiency and memory usage
- Verify proper handling of large JSON structures
- Assess polymorphic type resolution performance
- Review property wrapper overhead and optimization
2. Type Safety & Polymorphism [HIGH]
- Validate PolymorphicCodable identifier-based type resolution
- Check AnyCodable type erasure implementation for edge cases
- Verify UnnestedPolymorphic macro integration with runtime components
- Assess error handling in polymorphic decoding scenarios
3. API Design [HIGH]
- Evaluate public interface consistency across modules
- Check property wrapper ergonomics (@DefaultFalse, @DATEvalue, etc.)
- Verify protocol design follows Swift API guidelines
- Assess extensibility for new Codable patterns
4. BetterCodable Integration [MEDIUM]
- Review property wrapper implementations for common patterns
- Check date strategy implementations (ISO8601, RFC3339, etc.)
- Verify default value and lossy conversion handling
- Assess data encoding strategies (Base64, etc.)
5. Error Handling [MEDIUM]
- Verify comprehensive DecodingError and EncodingError usage
- Check PolymorphicCodableError provides sufficient context
- Assess graceful fallback handling in polymorphic scenarios
Review Focus
- Prioritize runtime correctness and performance over style
- Focus on real-world JSON processing scenarios
- Mark comments with priority: [HIGH], [MEDIUM], or [LOW]
Files:
Sources/KarrotCodableKit/BetterCodable/DateValue/DateValue.swiftSources/KarrotCodableKit/BetterCodable/DataValue/DataValue.swiftSources/KarrotCodableKit/BetterCodable/LosslessValue/LosslessValue.swiftSources/KarrotCodableKit/BetterCodable/LosslessValue/OptionalLosslessValue.swiftSources/KarrotCodableKit/BetterCodable/LossyValue/LossyArray.swiftSources/KarrotCodableKit/PolymorphicCodable/Extensions/KeyedDecodingContainer+OptionalPolymorphicArrayValue.swiftSources/KarrotCodableKit/PolymorphicCodable/PolymorphicValue.swiftSources/KarrotCodableKit/BetterCodable/DateValue/OptionalDateValue.swiftSources/KarrotCodableKit/BetterCodable/LossyValue/LossyDictionary.swiftSources/KarrotCodableKit/BetterCodable/LosslessValue/LosslessArray.swiftSources/KarrotCodableKit/PolymorphicCodable/Extensions/KeyedDecodingContainer+LossyOptionalPolymorphicValue.swiftSources/KarrotCodableKit/PolymorphicCodable/DefaultEmptyPolymorphicArrayValue.swiftSources/KarrotCodableKit/BetterCodable/Defaults/DefaultCodable.swiftSources/KarrotCodableKit/PolymorphicCodable/LossyOptionalPolymorphicValue.swift
🧬 Code graph analysis (26)
Tests/KarrotCodableKitTests/BetterCodable/LosslessValue/LosslessValueResilientTests.swift (1)
Sources/KarrotCodableKit/Resilient/ErrorReporting.swift (1)
errors(109-117)
Tests/KarrotCodableKitTests/BetterCodable/DateValue/DateValueResilientTests.swift (1)
Sources/KarrotCodableKit/Resilient/ErrorReporting.swift (1)
errors(109-117)
Sources/KarrotCodableKit/BetterCodable/DateValue/DateValue.swift (1)
Sources/KarrotCodableKit/Resilient/ErrorReporting.swift (1)
reportError(154-166)
Sources/KarrotCodableKit/BetterCodable/DataValue/DataValue.swift (1)
Sources/KarrotCodableKit/Resilient/ErrorReporting.swift (1)
reportError(154-166)
Sources/KarrotCodableKit/BetterCodable/LosslessValue/LosslessValue.swift (1)
Sources/KarrotCodableKit/Resilient/ErrorReporting.swift (1)
reportError(154-166)
Tests/KarrotCodableKitTests/PolymorphicCodable/ArrayValue/PolymorphicArrayValueResilientTests.swift (1)
Sources/KarrotCodableKit/Resilient/ErrorReporting.swift (1)
errors(109-117)
Tests/KarrotCodableKitTests/BetterCodable/DataValue/DataValueResilientTests.swift (1)
Sources/KarrotCodableKit/Resilient/ErrorReporting.swift (1)
errors(109-117)
Sources/KarrotCodableKit/BetterCodable/LosslessValue/OptionalLosslessValue.swift (1)
Sources/KarrotCodableKit/Resilient/ErrorReporting.swift (1)
reportError(154-166)
Tests/KarrotCodableKitTests/PolymorphicCodable/OptionalValue/LossyOptionalPolymorphicValueResilientTests.swift (1)
Sources/KarrotCodableKit/Resilient/ErrorReporting.swift (1)
errors(109-117)
Sources/KarrotCodableKit/BetterCodable/LossyValue/LossyArray.swift (2)
Sources/KarrotCodableKit/Resilient/ErrorReporting.swift (1)
reportError(154-166)Sources/KarrotCodableKit/Resilient/ResilientDecodingOutcome.swift (1)
recoveredFrom(46-46)
Tests/KarrotCodableKitTests/PolymorphicCodable/OptionalPolymorphicArrayValueResilientTests.swift (1)
Sources/KarrotCodableKit/Resilient/ErrorReporting.swift (1)
flushReportedErrors(84-89)
Tests/KarrotCodableKitTests/PolymorphicCodable/ArrayValue/PolymorphicLossyArrayValueResilientTests.swift (1)
Sources/KarrotCodableKit/Resilient/ErrorReporting.swift (1)
errors(109-117)
Tests/KarrotCodableKitTests/PolymorphicCodable/Value/PolymorphicValueResilientTests.swift (1)
Sources/KarrotCodableKit/Resilient/ErrorReporting.swift (1)
flushReportedErrors(84-89)
Sources/KarrotCodableKit/PolymorphicCodable/Extensions/KeyedDecodingContainer+OptionalPolymorphicArrayValue.swift (1)
Sources/KarrotCodableKit/Resilient/ErrorReporting.swift (1)
reportError(154-166)
Sources/KarrotCodableKit/PolymorphicCodable/PolymorphicValue.swift (1)
Sources/KarrotCodableKit/Resilient/ErrorReporting.swift (1)
reportError(154-166)
Sources/KarrotCodableKit/BetterCodable/DateValue/OptionalDateValue.swift (1)
Sources/KarrotCodableKit/Resilient/ErrorReporting.swift (1)
reportError(154-166)
Tests/KarrotCodableKitTests/BetterCodable/LossyValue/LossyDictionaryResilientTests.swift (1)
Sources/KarrotCodableKit/Resilient/ErrorReporting.swift (1)
errors(109-117)
Sources/KarrotCodableKit/BetterCodable/LossyValue/LossyDictionary.swift (2)
Sources/KarrotCodableKit/Resilient/ErrorReporting.swift (1)
reportError(154-166)Sources/KarrotCodableKit/Resilient/ResilientDecodingOutcome.swift (1)
recoveredFrom(46-46)
Sources/KarrotCodableKit/BetterCodable/LosslessValue/LosslessArray.swift (1)
Sources/KarrotCodableKit/Resilient/ErrorReporting.swift (1)
reportError(154-166)
Sources/KarrotCodableKit/PolymorphicCodable/Extensions/KeyedDecodingContainer+LossyOptionalPolymorphicValue.swift (2)
Sources/KarrotCodableKit/Resilient/ErrorReporting.swift (1)
reportError(154-166)Sources/KarrotCodableKit/Resilient/ResilientDecodingOutcome.swift (1)
recoveredFrom(46-46)
Tests/KarrotCodableKitTests/BetterCodable/LossyValue/LossyArrayResilientTests.swift (1)
Sources/KarrotCodableKit/Resilient/ErrorReporting.swift (1)
errors(109-117)
Sources/KarrotCodableKit/PolymorphicCodable/DefaultEmptyPolymorphicArrayValue.swift (2)
Sources/KarrotCodableKit/Resilient/ErrorReporting.swift (1)
reportError(154-166)Sources/KarrotCodableKit/Resilient/ResilientDecodingOutcome.swift (1)
recoveredFrom(46-46)
Sources/KarrotCodableKit/BetterCodable/Defaults/DefaultCodable.swift (3)
Sources/KarrotCodableKit/Resilient/ErrorReporting.swift (1)
reportError(154-166)Sources/KarrotCodableKit/BetterCodable/LossyValue/LossyDictionary.swift (1)
decode(273-295)Sources/KarrotCodableKit/Resilient/ResilientDecodingOutcome.swift (1)
recoveredFrom(46-46)
Sources/KarrotCodableKit/PolymorphicCodable/LossyOptionalPolymorphicValue.swift (2)
Sources/KarrotCodableKit/Resilient/ErrorReporting.swift (1)
reportError(154-166)Sources/KarrotCodableKit/Resilient/ResilientDecodingOutcome.swift (1)
recoveredFrom(46-46)
Tests/KarrotCodableKitTests/BetterCodable/Defaults/DefaultCodableResilientTests.swift (2)
Sources/KarrotCodableKit/Resilient/ErrorReporting.swift (1)
decode(56-71)Sources/KarrotCodableKit/BetterCodable/Defaults/DefaultCodable.swift (3)
decode(122-172)decode(180-254)decode(261-324)
Tests/KarrotCodableKitTests/PolymorphicCodable/ArrayValue/DefaultEmptyPolymorphicArrayValueResilientTests.swift (1)
Sources/KarrotCodableKit/Resilient/ErrorReporting.swift (1)
errors(109-117)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build and Test (16.4, release)
- GitHub Check: Check Macro Compatibility
- GitHub Check: Build and Test (16.4, debug)
🔇 Additional comments (20)
Sources/KarrotCodableKit/BetterCodable/DataValue/DataValue.swift (1)
54-57: DEBUG-only reporting is consistent with resilient decoding policy.Catches rethrow while reporting only in DEBUG, matching the project-wide approach and keeping release hot paths lean.
Priority: [LOW]
Sources/KarrotCodableKit/BetterCodable/DateValue/OptionalDateValue.swift (2)
56-59: Inner decode error reporting is correctly DEBUG-gated.Rethrow preserves semantics; reporting remains dev-only.
Priority: [LOW]
65-68: Outer decode error reporting is correctly DEBUG-gated.Consistent with the rest of BetterCodable wrappers.
Priority: [LOW]
Sources/KarrotCodableKit/BetterCodable/DateValue/DateValue.swift (1)
54-57: DEBUG-only reporting looks good and keeps release builds lean.Behavior (throwing) is unchanged; only diagnostics differ by configuration.
Priority: [LOW]
Tests/KarrotCodableKitTests/BetterCodable/DateValue/DateValueResilientTests.swift (1)
122-127: Test correctly differentiates DEBUG vs non-DEBUG behavior.Asserts digest only in DEBUG and nil in release, matching production semantics.
Priority: [LOW]
Sources/KarrotCodableKit/PolymorphicCodable/PolymorphicValue.swift (1)
59-61: DEBUG-only error reporting for polymorphic decode is consistent and safe.Re-throw ensures callers still observe failures; diagnostics stay dev-only.
Priority: [LOW]
Tests/KarrotCodableKitTests/PolymorphicCodable/Value/PolymorphicValueResilientTests.swift (1)
136-142: Keep DEBUG gating in the test: error reporting is only compiled in DEBUG builds
Alldecoder.reportErrorcalls in the source are wrapped in#if DEBUG, so in Release builds no errors are reported anderrorDigestremains nil. The existing#if DEBUG/#elseassertions in the test correctly reflect this and should not be removed.Likely an incorrect or invalid review comment.
Tests/KarrotCodableKitTests/BetterCodable/LossyValue/LossyArrayResilientTests.swift (1)
92-99: DEBUG-gated assertions are correct; verify Release config tooGood split between DEBUG and non-DEBUG. Please run this suite in both configurations to avoid config-only flakiness (CI matrix recommended).
Priority: [MEDIUM]
Sources/KarrotCodableKit/BetterCodable/Defaults/DefaultCodable.swift (1)
83-85: LGTM: DEBUG-only reporting in single-value initTop-level decode reports and recovers only in DEBUG; aligns with resilience design.
Priority: [LOW]
Tests/KarrotCodableKitTests/BetterCodable/LosslessValue/LosslessArrayResilientTests.swift (1)
19-22: LGTM: slimmed NestedObject keeps test focused on conversion failures. [LOW]The reduced conformance is appropriate for this scenario.
Tests/KarrotCodableKitTests/PolymorphicCodable/ArrayValue/PolymorphicArrayValueResilientTests.swift (1)
134-134: Doc-comment style is fine.The switch to "/// given" is consistent and improves BDD readability.
Priority: [LOW]
Tests/KarrotCodableKitTests/PolymorphicCodable/OptionalValue/LossyOptionalPolymorphicValueResilientTests.swift (1)
138-138: Doc-comment style is fine.Keeps the suite’s Given/When/Then consistent.
Priority: [LOW]
Tests/KarrotCodableKitTests/PolymorphicCodable/OptionalPolymorphicArrayValueResilientTests.swift (2)
152-152: Doc-comment style is fine.Matches the rest of the suite.
Priority: [LOW]
187-187: Doc-comment style is fine.Consistent and clear.
Priority: [LOW]
Tests/KarrotCodableKitTests/PolymorphicCodable/ArrayValue/DefaultEmptyPolymorphicArrayValueResilientTests.swift (2)
172-172: No action neededDoc comment only; functional behavior unchanged.
Priority: [LOW]
201-207: DEBUG-gated reporter assertions look goodAsserting non-DEBUG returns nil digest matches the new DEBUG-only reporting semantics.
Priority: [MEDIUM]Tests/KarrotCodableKitTests/PolymorphicCodable/ArrayValue/PolymorphicLossyArrayValueResilientTests.swift (2)
171-171: [LOW] LGTM on comment style tweak.The “/// given” doc-style comment is fine and consistent.
200-206: [MEDIUM] Good DEBUG-gated error-digest assertions.Clear split between DEBUG vs non-DEBUG behavior; this aligns with the framework’s resilient reporting model.
Tests/KarrotCodableKitTests/BetterCodable/LossyValue/LossyDictionaryResilientTests.swift (1)
103-109: [MEDIUM] DEBUG-only digest checks look good.The gating and minimum-error-count expectation matches the reporter’s contract.
Sources/KarrotCodableKit/BetterCodable/LossyValue/LossyDictionary.swift (1)
202-209: [MEDIUM] LGTM: nil value surfaces valueNotFound and reports in DEBUG.Matches the PR objective for non-optional properties and aligns with test expectations.
| #if DEBUG | ||
| decoder.reportError(error) | ||
| #endif | ||
| throw error |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Prefer runtime gating over compile-time #if DEBUG for SwiftPM portability.
DEBUG isn’t guaranteed across all build systems/configs. Using _isDebugAssertConfiguration() avoids relying on a compile-time define and preserves identical behavior.
Priority: [LOW]
Apply locally (propagate pattern elsewhere as needed):
- #if DEBUG
- decoder.reportError(error)
- #endif
+ if _isDebugAssertConfiguration() {
+ decoder.reportError(error)
+ }To verify DEBUG usage consistency in this repo:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Occurrences of '#if DEBUG':"
rg -n --no-heading "#if DEBUG" -S -g 'Sources/**' -g 'Tests/**' | wc -l
echo "Package.swift defines DEBUG?"
rg -nP 'swiftSettings\s*:\s*\[.*\.define\s*\(\s*"DEBUG"\s*\)' Package.swift -C2 || echo "No explicit swiftSettings DEBUG define found."
echo "xcconfig SWIFT_ACTIVE_COMPILATION_CONDITIONS:"
fd -H '\.xcconfig$' | xargs -r rg -n 'SWIFT_ACTIVE_COMPILATION_CONDITIONS' -C2 || echo "No xcconfig defines found."Length of output: 598
Use runtime debug check instead of #if DEBUG
File: Sources/KarrotCodableKit/BetterCodable/DataValue/DataValue.swift (lines 54–57)
Replace compile-time #if DEBUG with a runtime check so the guard works under SwiftPM and other build systems:
- #if DEBUG
- decoder.reportError(error)
- #endif
+ if _isDebugAssertConfiguration() {
+ decoder.reportError(error)
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #if DEBUG | |
| decoder.reportError(error) | |
| #endif | |
| throw error | |
| if _isDebugAssertConfiguration() { | |
| decoder.reportError(error) | |
| } | |
| throw error |
🤖 Prompt for AI Agents
In Sources/KarrotCodableKit/BetterCodable/DataValue/DataValue.swift around lines
54 to 57, replace the compile-time `#if DEBUG` block with a runtime debug check
so the decoder.reportError(error) call executes when running a debug/assert
build under SwiftPM and other build systems; change the conditional to use the
runtime check (for example `if _isDebugAssertConfiguration() {
decoder.reportError(error) }`) so the call is executed only in debug/assert
configurations while keeping the throw error afterward.
| #if DEBUG | ||
| decoder.reportError(error) | ||
| #endif | ||
| throw error |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Optional: switch to _isDebugAssertConfiguration() for portability.
Prevents reliance on a non-standard DEBUG define across SPM/Xcode.
Priority: [LOW]
- #if DEBUG
- decoder.reportError(error)
- #endif
+ if _isDebugAssertConfiguration() {
+ decoder.reportError(error)
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #if DEBUG | |
| decoder.reportError(error) | |
| #endif | |
| throw error | |
| if _isDebugAssertConfiguration() { | |
| decoder.reportError(error) | |
| } | |
| throw error |
🤖 Prompt for AI Agents
In Sources/KarrotCodableKit/BetterCodable/DateValue/DateValue.swift around lines
54-57, the code uses the compile-time #if DEBUG flag to call
decoder.reportError(error); replace that conditional with a runtime check using
_isDebugAssertConfiguration() so the reportError call runs in debug/assert
builds across SPM and Xcode; specifically, change the #if DEBUG / #endif block
to an if _isDebugAssertConfiguration() { decoder.reportError(error) } and keep
throwing the error afterward.
| /// Decodes successfully if key is available if not fallback to the default value provided. | ||
| public func decode<P>(_: DefaultCodable<P>.Type, forKey key: Key) throws -> DefaultCodable<P> { | ||
| // Check if key exists |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Docstring contradicts new semantics
Comment says “fallback to the default value” but PR mandates errors for non-optional on missing/nil. Update wording.
Priority: [LOW]
- /// Decodes successfully if key is available if not fallback to the default value provided.
+ /// For non-optional properties: throws on missing key or nil. Otherwise decodes, allowing strategy-specific recovery for type mismatches.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// Decodes successfully if key is available if not fallback to the default value provided. | |
| public func decode<P>(_: DefaultCodable<P>.Type, forKey key: Key) throws -> DefaultCodable<P> { | |
| // Check if key exists | |
| /// For non-optional properties: throws on missing key or nil. Otherwise decodes, allowing strategy-specific recovery for type mismatches. | |
| public func decode<P>(_: DefaultCodable<P>.Type, forKey key: Key) throws -> DefaultCodable<P> { | |
| // Check if key exists |
🤖 Prompt for AI Agents
In Sources/KarrotCodableKit/BetterCodable/Defaults/DefaultCodable.swift around
lines 121-123, the docstring currently claims the decoder "fallback to the
default value" on missing keys, but the PR changes semantics to throw for
non-optional types when the key is missing or null; update the docstring to
accurately state that decoding will throw an error for non-optional types on
missing or null values (and only use the provided default for optional/allowed
cases per the new semantics), and ensure the wording clearly documents the error
behavior and when the default is applied.
| #if DEBUG | ||
| decoder.reportError(error) | ||
| #endif | ||
| throw error |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Un-gate decoder.reportError to keep production telemetry. [HIGH]
The reporter is opt-in; unconditional reporting is safe and valuable.
- #if DEBUG
- decoder.reportError(error)
- #endif
+ decoder.reportError(error)
throw error📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #if DEBUG | |
| decoder.reportError(error) | |
| #endif | |
| throw error | |
| decoder.reportError(error) | |
| throw error |
🤖 Prompt for AI Agents
In Sources/KarrotCodableKit/BetterCodable/LosslessValue/LosslessValue.swift
around lines 68 to 71, the call to decoder.reportError(error) is currently
wrapped in a #if DEBUG guard; remove that conditional and call
decoder.reportError(error) unconditionally before throwing the error so
production telemetry (opt-in reporter) receives the report as well.
| #if DEBUG | ||
| decoder.reportError(error) | ||
| #endif | ||
| throw error |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
🛠️ Refactor suggestion
Always report to the error reporter; don’t gate behind DEBUG. [HIGH]
decoder.reportError is a no-op without an attached reporter; keeping it unconditional preserves production observability.
- #if DEBUG
- decoder.reportError(error)
- #endif
+ decoder.reportError(error)
throw error📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #if DEBUG | |
| decoder.reportError(error) | |
| #endif | |
| throw error | |
| decoder.reportError(error) | |
| throw error |
🤖 Prompt for AI Agents
In
Sources/KarrotCodableKit/BetterCodable/LosslessValue/OptionalLosslessValue.swift
around lines 80 to 83, the call to decoder.reportError(error) is currently
wrapped in an #if DEBUG block; remove the conditional so
decoder.reportError(error) is invoked unconditionally before throw error,
ensuring errors are always reported in production (the reporter remains a no-op
when not attached).
| #if DEBUG | ||
| let digest = try #require(errorDigest) | ||
| let errors = digest.errors | ||
| #expect(errors.count >= 1) | ||
| #else | ||
| #expect(errorDigest == nil) | ||
| #endif |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Tighten assertions on reported error shape (DEBUG).
Also assert at least one DecodingError to avoid false positives.
Priority: [MEDIUM]
#if DEBUG
let digest = try #require(errorDigest)
let errors = digest.errors
#expect(errors.count >= 1)
+ #expect(errors.contains { $0 is DecodingError })
#else
#expect(errorDigest == nil)
#endif📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #if DEBUG | |
| let digest = try #require(errorDigest) | |
| let errors = digest.errors | |
| #expect(errors.count >= 1) | |
| #else | |
| #expect(errorDigest == nil) | |
| #endif | |
| #if DEBUG | |
| let digest = try #require(errorDigest) | |
| let errors = digest.errors | |
| #expect(errors.count >= 1) | |
| #expect(errors.contains { $0 is DecodingError }) | |
| #else | |
| #expect(errorDigest == nil) | |
| #endif |
🤖 Prompt for AI Agents
In
Tests/KarrotCodableKitTests/PolymorphicCodable/ArrayValue/PolymorphicArrayValueResilientTests.swift
around lines 158-164, the DEBUG branch currently only asserts that errors.count
>= 1; tighten this by asserting that at least one of the entries in
digest.errors is a DecodingError (or unwrap/inspect underlying error types to
confirm DecodingError is present) and optionally assert expected error
shape/message if available; update the expectation to iterate the errors
collection and fail if no DecodingError is found so false positives are avoided.
| /// Verify errors were reported | ||
| let errorDigest = errorReporter.flushReportedErrors() | ||
|
|
||
| #if DEBUG | ||
| #expect(errorDigest != nil) | ||
| #else | ||
| #expect(errorDigest == nil) | ||
| #endif | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Strengthen DEBUG assertions with error type check.
Ensure at least one DecodingError is captured.
Priority: [MEDIUM]
/// Verify errors were reported
let errorDigest = errorReporter.flushReportedErrors()
#if DEBUG
-#expect(errorDigest != nil)
+let errors = try #require(errorDigest).errors
+#expect(!errors.isEmpty)
+#expect(errors.contains { $0 is DecodingError })
#else
#expect(errorDigest == nil)
#endif📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// Verify errors were reported | |
| let errorDigest = errorReporter.flushReportedErrors() | |
| #if DEBUG | |
| #expect(errorDigest != nil) | |
| #else | |
| #expect(errorDigest == nil) | |
| #endif | |
| } | |
| /// Verify errors were reported | |
| let errorDigest = errorReporter.flushReportedErrors() | |
| #if DEBUG | |
| let errors = try #require(errorDigest).errors | |
| #expect(!errors.isEmpty) | |
| #expect(errors.contains { $0 is DecodingError }) | |
| #else | |
| #expect(errorDigest == nil) | |
| #endif | |
| } |
🤖 Prompt for AI Agents
In
Tests/KarrotCodableKitTests/PolymorphicCodable/OptionalPolymorphicArrayValueResilientTests.swift
around lines 175–183, strengthen the DEBUG assertion so it not only checks
errorDigest is non-nil but also verifies that the reported errors include at
least one DecodingError; update the DEBUG branch to unwrap errorDigest, inspect
its reported errors (or error list) and assert that any error is a DecodingError
(e.g. using a contains(where:) check or pattern match), while keeping the
release branch unchanged.
| /// Verify errors were reported | ||
| let errorDigest = errorReporter.flushReportedErrors() | ||
|
|
||
| #if DEBUG | ||
| #expect(errorDigest != nil) | ||
| #else | ||
| #expect(errorDigest == nil) | ||
| #endif | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Strengthen DEBUG assertions with error type check.
Same rationale as above; confirm a DecodingError is present.
Priority: [MEDIUM]
/// Verify errors were reported
let errorDigest = errorReporter.flushReportedErrors()
#if DEBUG
-#expect(errorDigest != nil)
+let errors = try #require(errorDigest).errors
+#expect(!errors.isEmpty)
+#expect(errors.contains { $0 is DecodingError })
#else
#expect(errorDigest == nil)
#endif📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// Verify errors were reported | |
| let errorDigest = errorReporter.flushReportedErrors() | |
| #if DEBUG | |
| #expect(errorDigest != nil) | |
| #else | |
| #expect(errorDigest == nil) | |
| #endif | |
| } | |
| /// Verify errors were reported | |
| let errorDigest = errorReporter.flushReportedErrors() | |
| #if DEBUG | |
| let errors = try #require(errorDigest).errors | |
| #expect(!errors.isEmpty) | |
| #expect(errors.contains { $0 is DecodingError }) | |
| #else | |
| #expect(errorDigest == nil) | |
| #endif |
🤖 Prompt for AI Agents
In
Tests/KarrotCodableKitTests/PolymorphicCodable/OptionalPolymorphicArrayValueResilientTests.swift
around lines 202 to 210, the DEBUG assertion currently only checks that
errorDigest is non-nil; strengthen it to also verify the reported error is a
DecodingError by unwrapping errorDigest (or its stored errors) and asserting at
least one error is a DecodingError (e.g., cast or pattern-match to
DecodingError) before the existing non-nil expectation; keep the non-DEBUG
branch unchanged.
| #if DEBUG | ||
| let digest = try #require(errorDigest) | ||
| let errors = digest.errors | ||
| #expect(errors.count >= 1) | ||
| #expect(errors.first is DecodingError) | ||
| #else | ||
| #expect(errorDigest == nil) | ||
| #endif |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Assertions are good; consider verifying reporter reset (DEBUG).
After reading once, flush should return nil, proving reset behavior.
Priority: [LOW]
#if DEBUG
let digest = try #require(errorDigest)
let errors = digest.errors
#expect(errors.count >= 1)
#expect(errors.first is DecodingError)
+ // subsequent flush should be empty
+ #expect(errorReporter.flushReportedErrors() == nil)
#else
#expect(errorDigest == nil)
#endif📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #if DEBUG | |
| let digest = try #require(errorDigest) | |
| let errors = digest.errors | |
| #expect(errors.count >= 1) | |
| #expect(errors.first is DecodingError) | |
| #else | |
| #expect(errorDigest == nil) | |
| #endif | |
| #if DEBUG | |
| let digest = try #require(errorDigest) | |
| let errors = digest.errors | |
| #expect(errors.count >= 1) | |
| #expect(errors.first is DecodingError) | |
| // subsequent flush should be empty | |
| #expect(errorReporter.flushReportedErrors() == nil) | |
| #else | |
| #expect(errorDigest == nil) | |
| #endif |
🤖 Prompt for AI Agents
In
Tests/KarrotCodableKitTests/PolymorphicCodable/OptionalValue/LossyOptionalPolymorphicValueResilientTests.swift
around lines 159 to 166, add an assertion in the DEBUG branch to verify the
reporter/reset behavior: after obtaining the digest (let digest = try
#require(errorDigest)) and asserting on digest.errors, call the reporter flush
(or the same errorDigest retrieval mechanism) a second time and assert it
returns nil to prove the buffer was cleared; keep the existing expectations and
only add the extra check that a subsequent flush/read yields nil.
| @Test("error reporting with JSONDecoder") | ||
| func errorReporting() async throws { | ||
| // given | ||
| /// given |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
[LOW] Use inline comment instead of doc comment inside function bodies
Doc comments attach to declarations and are unnecessary here. Prefer // given to avoid polluting symbol docs.
- /// given
+ // given📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// given | |
| // given |
🤖 Prompt for AI Agents
In
Tests/KarrotCodableKitTests/PolymorphicCodable/Value/PolymorphicValueResilientTests.swift
around line 104, there's a doc comment ("/// given") inside a function body;
replace it with a regular inline comment ("// given") so the comment doesn't
attach to a declaration or pollute generated docs — simply change the leading
triple slash to two slashes for in-function comments.
ElonPark
deleted the
feature/elon/change-ResilientDecodingOutcome-logic
branch
Background (Required)
Changes
Testing Methods
Review Notes
Summary by CodeRabbit
New Features
Public API
Tests
Documentation