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

Jump to bottom

[SR-8649] Conform Range to Codable when Bound conforms to Codable #51164

Open
ankitspd opened this issue Aug 27, 2018 · 5 comments
Open

[SR-8649] Conform Range to Codable when Bound conforms to Codable #51164

ankitspd opened this issue Aug 27, 2018 · 5 comments
Labels
bug A deviation from expected or documented behavior. Also: expected but undesirable behavior. Codable Area → standard library: `Codable` and co. standard library Area: Standard library umbrella

Comments

@ankitspd
Copy link
Member

Previous ID SR-8649
Radar rdar://problem/36460457
Original Reporter @aciidb0mb3r
Type Bug
Additional Detail from JIRA
Votes 2
Component/s Standard Library
Labels Bug, Codable
Assignee None
Priority Medium

md5: dc1975fb85524422eb8278fd8aed41fc

Issue Description:

It is possible to conform Range to Codable when Bound is Codable:

extension Range: Codable where Bound: Codable {

    private enum CodingKeys: CodingKey {
        case lowerBound
        case upperBound
    }

    public init(from decoder: Decoder) throws {

        let container = try decoder.container(keyedBy: CodingKeys.self)

        let lowerBound = try container.decode(Bound.self, forKey: .lowerBound)
        let upperBound = try container.decode(Bound.self, forKey: .upperBound)

        self.init(uncheckedBounds: (lower: lowerBound, upper: upperBound))
    }

    public func encode(to encoder: Encoder) throws {

        var container = encoder.container(keyedBy: CodingKeys.self)

        try container.encode(self.lowerBound, forKey: .lowerBound)
        try container.encode(self.upperBound, forKey: .upperBound)
    }
}

I think this conformance should be provided by the standard library.
@itaiferber
Copy link
Contributor

+1 to this — we can likely go through parts of the stdlib and now conditionally conform more things that are useful.
One implementation note: I would not recommend using init(uncheckedBounds🙂 in the implementation as there is no guarantee that the bounds are valid (it's trivial to modify the payload to do this).

We should do this for the other variants (ClosedRange, PartialRangeFrom, PartialRangeThrough, PartialRangeUpTo), and maybe insert markers to distinguish between them. (i.e. if you encode a Range, you might not want to be able to accidentally decode it as a ClosedRange as the lower and upper bounds look identical but you'd get off-by-one errors)

@swift-ci
Copy link
Collaborator

Comment by Dale Buckley (JIRA)

@itaiferber I was thinking about what you suggested with the markers and I don't think they are needed if we name the keys correctly. If you think about server side implementation of this that yields a JSON document that's sent back to a client, then there needs to be an obvious way to distinguish what the `lowerBound` and `upperBound` actually relates to.

So for `Range` you would have the keys `from` and `upTo` and `ClosedRange` would be something like `from` and `through`. A subtile difference, but enough to make it more obvious to the consumer of a JSON document how the data is intended to be used and for the decoder to distinguish the difference.

Type

lowerBound

upperBound

Range

{code:java}
"from"

@swift-ci
Copy link
Collaborator

Comment by Dale Buckley (JIRA)

I've started a PR for this feature here: #19532

@itaiferber
Copy link
Contributor

dlbuckley (JIRA User) That works equivalently, yes. Thanks for starting a PR! However, @airspeedswift, this should be going through Swift Evolution, yeah? Or are we considering this a bug fix?

@swift-ci
Copy link
Collaborator

Comment by Dale Buckley (JIRA)

@itaiferber I didn't know whether it had to go through swift evolution or not. I started a discussion on the forum here a little while ago and there wasn't any real decision if it was a bug or enhancement that needed a proposal or not.

Anyway if it needs a proposal I will go that route, I just need some clarification one way or the other 🙂

@swift-ci swift-ci transferred this issue from apple/swift-issues Apr 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug A deviation from expected or documented behavior. Also: expected but undesirable behavior. Codable Area → standard library: `Codable` and co. standard library Area: Standard library umbrella
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@itaiferber @ankitspd @swift-ci