Skip to content
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.

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

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

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

Conversation

gfontenot
Copy link
Collaborator

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.

@gfontenot
Copy link
Collaborator Author

It's possible that I'm proposing removing this as an implicit operation, now that I'm thinking about it.

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.
@gfontenot gfontenot force-pushed the gfontenot/remove-optional-version branch from 56ecd1a to 1b397c7 Compare April 29, 2018 15:11
@gfontenot
Copy link
Collaborator Author

Updated this for the new subscript syntax. Feels more like pushing peas around on the plate now than it did when it was replacing <|? but I still think that simplifying the behavior and reducing the API surface area is worth it. Would love to hear if anyone has opinions about this.

@@ -34,7 +34,7 @@ extension LocationPost: Argo.Decodable {
<*> json["text"]
<*> json["author"]
<*> json["comments"]
<*> json[optional: "location"]
<*> .optional(json["location"])
Copy link
Contributor

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

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

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

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants