bignerdranch / Freddy

A reusable framework for parsing JSON in Swift.
MIT License
1.09k stars 119 forks source link

WIP: First draft at a possible JSONValueTransformer protocol #190

Closed jgallagher closed 6 years ago

jgallagher commented 8 years ago

See #118 for some background. This PR is a WIP / proof of concept / starting point for discussion. It’s not very long, so the following assumes you’ve read the changes.

Pros

This technique essentially gives us a way to write a “state-bearing” JSONDecodable. The particular pain point of parsing ISO8601 dates is directly addressed in this PR. More complex dates (e.g., .NET timestamp things) are easily pluggable into this system too.

The In/Out protocol system also allows more general things like the JSONTransformerChainLink in the PR. I currently don’t think this class should be included in Freddy, but putting it out as a proof of what’s possible.

Cons

The number of functions we have to add to JSONSubscripting is annoying, unless I’m missing a way to collapse them down. The PR adds a single one for In=String, but if that’s how we have to do it, there need to be 7-8 functions (one for each JSON type plus JSON itself, probably), plus all the “missing key” etc. variants.

It’s not immediately obvious when one should choose JSONDecodable vs JSONValueTransformable - JSONValueTransformable is “more powerful” than JSONDecodable, with the drawback that it requires instantiating an instance of the transformer type. Can we coalesce these into one?

Alternatives

One alternative I considered is to not have an In type in the protocol, and instead provide methods for each of the JSON types:

public protocol JSONValueTransformer {
    associatedtype Out

    func transformValue(value: JSON) throws -> Out
    func transformValue(value: [JSON]) throws -> Out
    func transformValue(value: [String : JSON]) throws -> Out
    func transformValue(value: Double) throws -> Out
    func transformValue(value: Int) throws -> Out
    func transformValue(value: String) throws -> Out
    func transformValue(value: Bool) throws -> Out
    func transformNull() throws -> Out
}

extension JSONValueTransformer {
    public func transformValue(value: JSON) throws -> Out {
        switch value {
        case .Array(let arr):       return try transformValue(arr)
        case .Dictionary(let dict): return try transformValue(dict)
        case .String(let string):   return try transformValue(string)
        case .Double(let double):   return try transformValue(double)
        case .Int(let int):         return try transformValue(int)
        case .Bool(let bool):       return try transformValue(bool)
        case .Null:                 return try transformNull()
        }
    }

    public func transformValue(value: [JSON]) throws -> Out          { throw JSONValueTransformerError.InvalidInputType }
    public func transformValue(value: [String : JSON]) throws -> Out { throw JSONValueTransformerError.InvalidInputType }
    public func transformValue(value: Double) throws -> Out          { throw JSONValueTransformerError.InvalidInputType }
    public func transformValue(value: Int) throws -> Out             { throw JSONValueTransformerError.InvalidInputType }
    public func transformValue(value: String) throws -> Out          { throw JSONValueTransformerError.InvalidInputType }
    public func transformValue(value: Bool) throws -> Out            { throw JSONValueTransformerError.InvalidInputType }
    public func transformNull() throws -> Out                        { throw JSONValueTransformerError.InvalidInputType }
}

This reduces the API surface we have to add to JSONSubscripting, but seems less nice in every other way.

zwaldowski commented 8 years ago

I admit to liking:

protocol JSONValueTransformer {
    associatedtype Out
    func transformed(value: JSON) throws -> Out
}

and whatever else we want to add on top of it.

The overlap between JSONValueTransformer and JSONDecodable is concerning. If you think of JSONDecodable as:

protocol ProtoJSONDecodable {
    associated type Out = Self
    static func transformed(value: JSON) throws -> Out
}

the difference is even more troubling.

jgallagher commented 8 years ago

Yeah I'm really not a fan of that either, but I'm not immediately seeing how to avoid keeping them as two separate protocols.

zwaldowski commented 8 years ago

Well, we have two solutions:

I am in favor of adding less overloads if we have to, so I'd be in favor of JSONValueTransformer only having Out.

jgallagher commented 8 years ago

👍 Updated - I'm fine with this since the transformer chaining was a solution in search of a problem anyway (I think). Still not thrilled with how similar it is to JSONDecodable, but maybe I should get over that.

zwaldowski commented 8 years ago

Yeah, I'm not a fan either.

Maybe we could clean things up by making JSONValueTransformer the canonical way-to-do-that-thing and everything JSONDecodable forwards to it via a wrapper struct.