Conversation
Codecov Report
@@ Coverage Diff @@
## master #175 +/- ##
==========================================
+ Coverage 92.81% 94.75% +1.94%
==========================================
Files 6 8 +2
Lines 153 381 +228
==========================================
+ Hits 142 361 +219
- Misses 11 20 +9
Continue to review full report at Codecov.
|
# Conflicts: # Partial.xcodeproj/project.pbxproj
| private var decoders: [CodingKey: Decoder] = [:] | ||
|
|
||
| func encode(_ value: Any, forKey keyPath: PartialKeyPath<Root>, to container: inout KeyedEncodingContainer<CodingKey>) throws { | ||
| try encoders[keyPath]?(value, &container) |
There was a problem hiding this comment.
Should this throw an error if no encoder exists for a given keyPath?
There was a problem hiding this comment.
I hadn't considered that. It may be useful for consumers to be able to opt out certain values from being encoded, which making this throw would prevent.
But having this throw would also make debugging easier if a key path/coding key is missed.
Would it make sense to allow the CodingKey returned be nil, and if it's nil it'll never be encoded, but if it's missing it'll throw here?
| } | ||
|
|
||
| func decode(_ codingKey: CodingKey, in container: KeyedDecodingContainer<CodingKey>) throws -> (Any, PartialKeyPath<Root>)? { | ||
| try decoders[codingKey]?(codingKey, container) |
There was a problem hiding this comment.
Should probably throw here also?
| /// static var keyPathCodingKeyCollection: KeyPathCodingKeyCollection<Self, CodingKeys> { | ||
| /// (\Self.stringValue, CodingKeys.stringValue) | ||
| /// (\Self.intValue, CodingKeys.intValue) | ||
| /// } |
There was a problem hiding this comment.
I am familiar with @resultBuilder, but haven't had the chance to test it myself yet. Would this example code result in a new KeyPathCodingKeyCollection being created every time keyPathCodingKeyCollection is called? I know that View.body in SwiftUI is processed every time it is called, but it is an instance var, not static, so I'm not certain what the behaviour is here.
There was a problem hiding this comment.
The other possible issue with using @resultBuilder here is that it potentially increases the risk of missing a CodingKeys value. An alternative implementation would be similar, except the function being implemented is:
static func keyPath(for codingKey: CodingKeys) -> PartialKeyPath<Wrapped>The function would then use a switch internally. Eg, for your sample code:
static func keyPath(for codingKey: CodingKeys) -> PartialKeyPath<Wrapped> {
switch codingKey {
case .stringValue: return \Self.stringValue
case .intValue: return \Self.intValue
}
}The builder code would then do pretty much the same as it's doing now, but enumerate through the CodingKeys. This would be added as a default implementation of keyPathCodingKeyCollection:
extension PartialCodable {
static var keyPathCodingKeyCollection: KeyPathCodingKeyCollection<Self, CodingKey> = {
var result = KeyPathCodingKeyCollection<Self, CodingKey>()
for codingKey in Self.CodingKeys.allCases {
result.addPair(keyPath: keyPath(for: codingKey), codingKey: codingKey)
}
return result
}()
}The advantage from a resilience POV is that switch forces you to handle all cases (unless you use default) whereas the @resultBuilder will be totally fine if you accidentally miss a case.
There was a problem hiding this comment.
Would this example code result in a new KeyPathCodingKeyCollection being created every time keyPathCodingKeyCollection is called?
A new collection is created every call, but it's only created once per encode/decode.
An alternative implementation would be similar...
I initially had it like this but there's an issue knowing what the type to encode/decode is, so the implementer has to do the casting itself.
The best I could come up was https://github.com/JosephDuffy/Partial/blob/8a99ebbc1c6af9cd520da7654bcf327113eaf2c5/Tests/PartialTests/Tests/PartialCodableTests.swift but I wasn't a fan of it.
The other possible issue with using @resultBuilder here is that it potentially increases the risk of missing a CodingKeys value.
This is a concern for sure, especially since Swift will synthesise CodingKeys. Fixing this would require at least 2 functions (from key path to coding key and visa-versa) which for me is an acceptable traidoff, but I couldn't find a way to do this without the implementation also needing to perform a cast to the expected type.
|
That would be a really great feature! Imagine I have a profile and a complete profile with details. Full profile: With this PR merged, I could merge models and parse them from JSON. |
This is certainly a good use case, I do plan to work on this when I have more spare time. I don't personally need this feature so it's not a top priority for me currently! |
3 participants
TODO
Decodablesupportunwrappedthat's added byPartialConvertiblewhenWrappedisPartialCodablePartialCodable