Make conformances to Codable conditional

[mrwerdo]

I've ran into a problem when using the graph types with types that don't conform to Codable. Previously, if you used WeightedGraph<V, W>, then V and W both must conform to Codable, which prohibits WeightedGraph from being used in the case where V and W don't conform to Codable.

This pull request makes the conformance to Encodable and Decodable conditional for UnweightedGraph, WeightedGraph, and UniqueElementsGraph.

The conformances should be the same as the generated code by the compiler when confirming to Codable.

There are breaking changes, namely that:

• UnweightedGraph, WeightedGraph, and UniqueElementsGraph have a required convenience initialiser for constructing a graph from a Decoder. This would mean that any subclasses would fail to compile.
• Graph and Edge no longer inherits Codable. This means that conforming types would need to also conform to Codable themselves to maintain compatibility.

Let me know what you think. I'm welcome to any feedback you have to offer. Thanks.

[davecom]

Hi @Mrwerdo,

Thanks for the contribution and detailed explanation. There's actually quite a bit of history here and I'm not sure if I remember it all correctly because it's been years now, but: When Codable was introduced in Swift 4, we initially added subclasses of WeightedGraph and UnweightedGraph that conformed to it. Some time later, if I'm remembering correctly, we tried this approach of conditional conformance when it became available and there were some issues in the Swift language (bugs really I think) that prevented us from going this direction.

We went to full Codable compliance as a simplification for most people. May I ask what your use case is? Do you have a type that's not Codable that you want to use with SwiftGraph?

I'm not opposed to the change really, but since we already have changed this multiple times, I don't want to "pull the rug out from under people" so to speak who may be using it the way it is. I'd also like to maybe leave this open for a while to hear other people's perspectives (sometimes that never happens in a middling popular project, but we'll see).

Thanks, David

[mrwerdo]

Hi David,

You're welcome. Hopefully someone will be able to shed some light on whether these changes are practical or not for the community.

My use case is modelling a network for a game I'm making in my spare time. Some of the structures are like so:

struct Message {
    enum Address {
         case all
         case device(Device)
    }
    var destination: Address
    var source: Device
}

class Device {
    var sentMessages: [Message] = []
    var receivedMessages: [Message] = []
}

var topology = UnweightedGraph<Device>()

Making Message.Address conform to Codable is unclear because the encoder would need to resolve the references to instances of Device. At this point, it's easier for me to make UnweightedGraph conform conditionally to Codable than it is to implement an encoding and decoding mechanism.

Maybe there's another way of making the changes without breaking existing clients? Perhaps by creating new types and deprecating old ones? I'm not fussed if you reject the pull request, I just thought that I'd see whether you were interested in them.

-Andrew

[sbeitzel]

I'm not yet a user of this project (although I'm planning to incorporate it into a personal project soon) so my opinion may not have much weight. That said, @Mrwerdo , I think the affordance that comes from enforced adherence to Codable is that when/if GraphML or GraphViz exports are supported (through custom encoders) then everything will Just Work. Meanwhile, your specific case could be handled with a little work:

struct Message {
    enum Address {
        case all
        case device(String) // device ID
    }
    var destination: Address
    var source: String // device ID
}

class Device: Identifiable {
    static let devNull = Device()

    let id: String = UUID().uuidString
...
}

class DeviceManager {
    var deviceDictionary = [String: Device]()

    func createDevice() -> Device {
        let device = Device()
        deviceDictionary[device.id] = device
        return device
    }

    func lookup(_ deviceID: String) -> Device {
        if let found = deviceDictionary[deviceID] {
            return found
        }
        return Device.devNull
    }
}

...and so on. This would make it easy for Device and Message to be Codable, making it easy to save and restore your game state, as well.

[davecom]

I really appreciate the effort here, but I think for most users of the library the default conformance is more of a help than a hindrance. I am also concerned about changing the defaults around this area once again when we've already changed them in the past. Going back and forth for a few users of the library who are working with it in this area is not good. And I agree with @sbeitzel, that it may be really nice that we had it by default in the future if a contribution for GraphML or GraphViz comes through.