From 1b397c7bbe3faf8bc506dfb31c2ee22c069977c6 Mon Sep 17 00:00:00 2001 From: Gordon Fontenot Date: Fri, 27 Apr 2018 14:53:11 -0500 Subject: [PATCH] Remove optional decoding as an explicit operation Previously, we've used a special operator (`<|?`) for denoting that we were decoding to an optional property. This operator had special behavior that meant it would _only_ fail if the decoding operation itself failed, but would still succeed if the key was missing. I think that ultimately, this was a confusing distinction. Although it was nice and concise, it made it difficult to reason about some transformations (like flatMapping into a secondary transformation), and it also meant we needed to double the surface area of the API. The special behavior around _when_ we failed the decoding was also potentially confusing and could lead to unexpected results for people that weren't completely familiar with the distinction between how we handled various decoding failures internally. Additionally, we've had another solution to this problem for nearly the entire time this library has existed: `Decoded.optional`. This is a _much_ simpler transformation, and it simply converts the `Decoded` to an `Optional` and then wraps it in `.success`. It seems reasonable, then (especially with the additional API changes currently in flight) to reduce the surface area _and_ the complexity of our API at the same time by removing the `<|?` operators in favor of explicitly wrapping our extraction operations in a call to `.optional`. Note that this _also_ means we can use conditional conformance to make conform `Optional` conform to `Decoded` directly. --- Sources/Argo/Types/JSON.swift | 26 ------------------- Sources/Argo/Types/StandardTypes.swift | 4 +-- Tests/ArgoTests/Models/Post.swift | 2 +- Tests/ArgoTests/Models/TestModel.swift | 8 +++--- Tests/ArgoTests/Models/User.swift | 2 +- Tests/ArgoTests/Tests/DecodedTests.swift | 14 ++-------- .../Tests/EmbeddedDecodingTests.swift | 6 ++--- 7 files changed, 13 insertions(+), 49 deletions(-) diff --git a/Sources/Argo/Types/JSON.swift b/Sources/Argo/Types/JSON.swift index e7087ba..ad0ba1e 100644 --- a/Sources/Argo/Types/JSON.swift +++ b/Sources/Argo/Types/JSON.swift @@ -63,32 +63,6 @@ extension JSON { return flatReduce(keyPath, initial: self, combine: decodedJSON) .flatMap(T.decode) } - - /** - Attempt to extract an optional value at the specified key path and - transform it into the requested type. - - This method is used to decode an optional value from the `JSON`. If any of - the keys in the key path aren't present in the `JSON`, this will still return - `.success`. However, if the key path exists but the object assigned to the - final key is unable to be decoded into the requested type, this will return - `.failure`. - - - parameter keyPath: The key path for the object to decode, represented by - a variadic list of strings. - - returns: A `Decoded` optional value representing the success or failure - of the decode operation - */ - public subscript(optional keyPath: String...) -> Decoded where T == T.DecodedType { - switch flatReduce(keyPath, initial: self, combine: decodedJSON) { - case .failure: - return .success(.none) - - case let .success(x): - return T.decode(x) - .flatMap { .success(.some($0)) } - } - } } extension JSON: Decodable { diff --git a/Sources/Argo/Types/StandardTypes.swift b/Sources/Argo/Types/StandardTypes.swift index db276f8..f43133c 100644 --- a/Sources/Argo/Types/StandardTypes.swift +++ b/Sources/Argo/Types/StandardTypes.swift @@ -161,7 +161,7 @@ extension Bool: Decodable { } } -public extension Optional where Wrapped: Decodable, Wrapped == Wrapped.DecodedType { +extension Optional: Decodable where Wrapped: Decodable, Wrapped == Wrapped.DecodedType { /** Decode `JSON` into an `Optional` value where `Wrapped` is `Decodable`. @@ -172,7 +172,7 @@ public extension Optional where Wrapped: Decodable, Wrapped == Wrapped.DecodedTy - returns: A decoded optional `Wrapped` value */ - static func decode(_ json: JSON) -> Decoded { + public static func decode(_ json: JSON) -> Decoded { return Wrapped.decode(json) >>- { .success(.some($0)) } } } diff --git a/Tests/ArgoTests/Models/Post.swift b/Tests/ArgoTests/Models/Post.swift index a3277d5..047b160 100644 --- a/Tests/ArgoTests/Models/Post.swift +++ b/Tests/ArgoTests/Models/Post.swift @@ -34,7 +34,7 @@ extension LocationPost: Argo.Decodable { <*> json["text"] <*> json["author"] <*> json["comments"] - <*> json[optional: "location"] + <*> .optional(json["location"]) } } diff --git a/Tests/ArgoTests/Models/TestModel.swift b/Tests/ArgoTests/Models/TestModel.swift index 2bfee0b..a93a8a7 100644 --- a/Tests/ArgoTests/Models/TestModel.swift +++ b/Tests/ArgoTests/Models/TestModel.swift @@ -22,10 +22,10 @@ extension TestModel: Argo.Decodable { <*> json["user_opt", "name"] <*> json["bool"] <*> json["string_array"] - <*> json[optional: "string_array_opt"] + <*> .optional(json["string_array_opt"]) <*> json["embedded", "string_array"] - <*> json[optional: "embedded", "string_array_opt"] - <*> json[optional: "user_opt"] + <*> .optional(json["embedded", "string_array_opt"]) + <*> .optional(json["user_opt"]) <*> json["dict"] } } @@ -50,7 +50,7 @@ extension TestModelNumerics: Argo.Decodable { <*> json["int64_string"] <*> json["double"] <*> json["float"] - <*> json[optional: "int_opt"] + <*> .optional(json["int_opt"]) return f <*> json["uint"] diff --git a/Tests/ArgoTests/Models/User.swift b/Tests/ArgoTests/Models/User.swift index 6b5af79..3f7fc2f 100644 --- a/Tests/ArgoTests/Models/User.swift +++ b/Tests/ArgoTests/Models/User.swift @@ -13,6 +13,6 @@ extension User: Argo.Decodable { return curry(self.init) <^> json["id"] <*> (json["userinfo", "name"] <|> json["name"]) - <*> json[optional: "email"] + <*> .optional(json["email"]) } } diff --git a/Tests/ArgoTests/Tests/DecodedTests.swift b/Tests/ArgoTests/Tests/DecodedTests.swift index 56be115..ddea493 100644 --- a/Tests/ArgoTests/Tests/DecodedTests.swift +++ b/Tests/ArgoTests/Tests/DecodedTests.swift @@ -78,19 +78,9 @@ class DecodedTests: XCTestCase { "email": 1 ]) - let expected: [DecodeError] = [ - .missingKey("name"), - .typeMismatch(expected: "String", actual: String(describing: JSON.number(1))) - ] - - switch user { - case let .failure(.multiple(errors)): - print("expected: \(expected)") - print("actual: \(errors)") + let expected: DecodeError = .missingKey("name") - XCTAssertEqual(errors, expected) - default: XCTFail("Unexpected Case Occurred") - } + XCTAssertEqual(user.error, expected) } func testDecodedMaterializeSuccess() { diff --git a/Tests/ArgoTests/Tests/EmbeddedDecodingTests.swift b/Tests/ArgoTests/Tests/EmbeddedDecodingTests.swift index 9c0e178..2924a40 100644 --- a/Tests/ArgoTests/Tests/EmbeddedDecodingTests.swift +++ b/Tests/ArgoTests/Tests/EmbeddedDecodingTests.swift @@ -11,12 +11,12 @@ final class EmbeddedDecodingTests: XCTestCase { } } - func testFailOnEmbeddedObject() { + func testDecodeEmbeddedOptionalObject() { let post: Decoded = decode(json(fromFile: "bad_location_post")!) - switch post.error { + switch post.value { case .some: XCTAssert(true) - case .none: XCTFail("Unexpected Success") + case .none: XCTFail("Unexpected Failure") } } }