Skip to content

[RFC] Remove optional decoding as an explicit operation #490

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

[RFC] Remove optional decoding as an explicit operation #490

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 0 additions & 26 deletions Sources/Argo/Types/JSON.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<T: Decodable>(optional keyPath: String...) -> Decoded<T?> 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 {
Expand Down
4 changes: 2 additions & 2 deletions Sources/Argo/Types/StandardTypes.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<Wrapped>` value where `Wrapped` is `Decodable`.

Expand All @@ -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<Wrapped?> {
public static func decode(_ json: JSON) -> Decoded<Optional> {
return Wrapped.decode(json) >>- { .success(.some($0)) }
}
}
Expand Down
2 changes: 1 addition & 1 deletion Tests/ArgoTests/Models/Post.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ extension LocationPost: Argo.Decodable {
<*> json["text"]
<*> json["author"]
<*> json["comments"]
<*> json[optional: "location"]
<*> .optional(json["location"])
Copy link
Contributor

@mdiep mdiep Apr 29, 2018

Choose a reason for hiding this comment

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

These aren't equivalent, right? The new way would be just <*> json["location"]?

If location has the wrong type, this would fail before but would pass now?

Copy link
Collaborator Author

@gfontenot gfontenot May 6, 2018

Choose a reason for hiding this comment

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

Not 100% equivalent, no.

The old behavior (json[optional: "location"] or json <|? "location) would have failed the decode process if the object under the location key wasn't able to be decoded to the type Location. But it would have passed if there wasn't any object at that key.

The new behavior (.optional(json["location"])) will always succeed, no matter why the decode operation failed.

This is definitely a change in the behavior, but I think it's a simplification that makes it easier to reason about what might fail and for what reasons.

Copy link
Contributor

@mdiep mdiep May 6, 2018

Choose a reason for hiding this comment

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

But this could just be <*> json["location"] to keep the old behavior, right?

Since Argo still has both behaviors, it seems odd to me that you'd change this test. Presumably there are other tests for the .optional behavior.

Copy link
Collaborator Author

@gfontenot gfontenot May 6, 2018

Choose a reason for hiding this comment

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

Ahhhh. No. I see the confusion. This might actually be a good argument for not making Optional conform to Decodable actually. Because of the way decoding works, we can’t bake the old <|? behavior into that implementation (we could hit a failure before we ever get to that decode method), so .optional syntax would be the only way to get the old behavior (or a simplified version of it). You do bring up a good point that this would compile (but would fail) without using .optional though. That could be super confusing for people

}
}

Expand Down
8 changes: 4 additions & 4 deletions Tests/ArgoTests/Models/TestModel.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"]
}
}
Expand All @@ -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"]
Expand Down
2 changes: 1 addition & 1 deletion Tests/ArgoTests/Models/User.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"])
}
}
14 changes: 2 additions & 12 deletions Tests/ArgoTests/Tests/DecodedTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down
6 changes: 3 additions & 3 deletions Tests/ArgoTests/Tests/EmbeddedDecodingTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,12 +11,12 @@ final class EmbeddedDecodingTests: XCTestCase {
}
}

func testFailOnEmbeddedObject() {
func testDecodeEmbeddedOptionalObject() {
let post: Decoded<LocationPost> = 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")
}
}
}