Codable support

[yoiang]

Initial commit! Pushing to show progress, potentially get insight from others.

It would be great to see if we could implement it such that the generic classes' types didn't require conforming to Codable and that it would only be necessary when wanted to the classes as Codable. However Swift's init methods as well as where clauses have limitations with extensions that I haven't figured out how to work the way I am hoping.

Also tests need cleaning and elaboration.

[davecom]

Hi @yoiang, I think this is a great start! I think what would make sense stylistically, is to put everything related to Codable support into its own file(s) in the project with a name like Graph+Codable.swift or something along those lines and implement it as an extension. I just don't want the existing classes to get too messy and I think it makes some sense organizationally. Best, DK

[yoiang]

This is the first limitation of Swift come in. The first obvious one is that the Decodable protocol requires an initializer, initializers that are implemented as a result of protocol conformance must be marked as required and thus can't put added as an extension.

I've been trying to think of another architecture we could use to get around this in particular, say a factory, but so far everything has come short of leveraging the strengths of Decodable.

[davecom]

Ah, that makes sense. Sorry, I didn't think of that issue when I suggested it. I guess we can just put it right into the classes then. I rather that than have to do something funky like a factory.

[davecom]

I guess the only other issue I have with it is that this is going to permanently require that all vertices and edges are Codable as well. So, that's an extra hoop that people will have to jump through to use their custom class as a vertex, right? Ensure it too has Codable support. I don't really like that idea on the surface of it. I guess I was envisioning that there would be some way with Swift conditional conformance to do this so that we could have our cake and eat it: allow Graph to be used with both Codable vertices and not Codable vertices.

[yoiang]

Yea, that's what I called out above as well in the initial commit message. I agree about not putting that requirement on consumers of the library. So far my attempts haven't been valid Swift, I'm going to spend a little more time, ask around and see if there is a way to do it.

[davecom]

I know this example might seem too trivial, but I was hoping we could do something like this: https://twitter.com/stephencelis/status/979394451280982016

[yoiang]

I have tried that route, I believe that is only possible in a struct which does allow for adopting required protocol initializers within extensions. The poster didn't clarify what Result originally is ;)

[davecom]

I wonder if this will change with the improvements to conditional conformance in Swift 4.2. I am waiting on the Mac App Store release of Xcode 10 to try (tomorrow? Maybe with Mojave next week?).

[yoiang]

Unfortunately it looks like it doesn't.

I understand punting on this PR for now, I'm going to maintain it in my own fork as long as I'm using the library, hopefully a later version of Swift will add what we're looking for!

[davecom]

Hmm, it looks like an example in the Ray Wenderlich article of exactly what we're trying to do, but again it's on a struct, not a class. I will mess with this too later in the week after Xcode 10 launches and try to build on what you've started. The other long term option is to create subclasses of UnweightedGraph and WeightedGraph like SerializableUnweightedGraph and SerializableWeightedGraph. And then yes, we could also go down the factory route, check if the generic types are Codable at runtime and return the right type. If this would be API changing, we could just make it part of the 2.0 version of SwiftGraph.

[davecom]

Okay, I just found this in the release notes for Swift 4.2 in Xcode 10. Apparently it's a bug:

Finally, due to Decodable having an initializer requirement, it's not possible to conform to Decodable in an extension to a class that isn't final. Such a class needs to have any initializers from protocols be required, which means they need to be in the class definition. (SR-6803) (39199726)

https://developer.apple.com/documentation/xcode_release_notes/xcode_10_release_notes/swift_4_2_release_notes_for_xcode_10

https://bugs.swift.org/browse/SR-6803

[yoiang]

Oh interesting, if I'm reading this correctly they're going to extend automatic synthesis to extensions as long as you're within the same file to allow this flexibility. Feels like a strange inconsistency in Swift but hey, our benefit ;)

Hopefully this will be enough, on first glance it doesn't sound like it does anything for cases where the automatic initializer synthesis is not enough and we still must create a custom initializer.

[davecom]

Okay, apparently that fix actually did go into Swift 4.2. I asked on Twitter about it: https://twitter.com/dgregor79/status/1042064271411109889

I'll probably get a chance to try it out later today.

[davecom]

@yoiang I've individually added your CodableTests file with some modifications. I ended up creating two subclasses of WeightedGraph and UnweightedGraph. I think this will have to do for now for the main project until we have conditional conformance in the base Graph protocol. Thanks for all of your help with this. It's good to see it make it into master.

[yoiang]

Hey @davecom ! Since my work isn't represented in the git history would you mind adding me to the list of contributors?

Cheers!

[davecom]

Sure, I added a note to Contributors. Thanks.

[yoiang]

Cheers dude, I appreciate it!