Encode/Decode SemanticVersion as a single value string

[chriseplettsonos]

Since SemanticVersion is LosslessStringConvertible, it seems to make more sense for its encoding/decoding strategy to allow it to be serialized more succinctly as a single string value, rather than using the synthesized structured encoding provided by simply declaring Codable conformance. This is also more likely to conform to how such values will be served up by external APIs.

This change implements custom init(from:) and encode(to:) methods to provide this behavior, as well as unit tests to verify the behavior.

Note this is a breaking change to the encoded format of the structure. I'd be curious if anyone had thoughts on how this could be made optional (a la dateDecodingStrategy, perhaps?) so that it can be made backwards compatible.

To do:

• check Codable format use in SPI-Server (@finestructure)

[finestructure]

Note this is a breaking change to the encoded format of the structure. I'd be curious if anyone had thoughts on how this could be made optional (a la dateDecodingStrategy, perhaps?) so that it can be made backwards compatible.

That's a good point and I think we (in our SPI-Server project, which is the main reason this package exists) probably rely on the current codable format, I'll need to double check. If so, we're persisting the struct in the db, so this would be massively breaking for us.

Even if we made this a semver 1.0.0 change it would effectively exclude us from using this package as a dependency at 1.0.0 or higher forever unless this behaviour was configurable.

[finestructure]

Ok, I can confirm that we're persisting SemanticVersion with the current Codable format so we cannot adopt this as is.

I agree that a more succinct format would be nice but I don't see how we could adopt this without migrating our data.

We could have a custom decoder that supports both format by trying them both.

As to encoding, I think the only option is to use JSONEncode.userInfo to pass an encoding strategy in which could be made nicer by adding a strategy wrapper similar to the existing strategies:

extension CodingUserInfoKey {
    static var semanticVersionEncodingStrategy: Self { .init(rawValue: "SemanticVersionEncodingStrategy")! }
}

extension JSONEncoder {
    enum SemanticVersionEncodingStrategy {
        case memberwise
        case succinct
    }

    var semanticVersionEncodingStrategy: SemanticVersionEncodingStrategy {
        get {
            (userInfo[.semanticVersionEncodingStrategy] as? SemanticVersionEncodingStrategy) ?? .memberwise
        }
        set {
            userInfo[.semanticVersionEncodingStrategy] = newValue
        }
    }
}


struct SemanticVersion: Codable {
    var major: Int
    var minor: Int
    ...

    enum CodingKeys: CodingKey {
        case major
        case minor
        ...
    }

    init(from decoder: Decoder) throws {
        // could also add a decoding strategy here instead of trying both formats
        ...
    }

    func encode(to encoder: Encoder) throws {
        var container: KeyedEncodingContainer<SemanticVersion.CodingKeys> = encoder.container(keyedBy: SemanticVersion.CodingKeys.self)

        if let encoder = encoder as? JSONEncoder {
            switch encoder.semanticVersionEncodingStrategy {
                case .memberwise:
                    ...
                case .succinct:
                    ...
            }
        } else {
            try container.encode(self.major, forKey: SemanticVersion.CodingKeys.major)
            try container.encode(self.minor, forKey: SemanticVersion.CodingKeys.minor)
            ...
        }
    }
}


let version = SemanticVersion(...)
let enc = JSONEncoder()
enc.semanticVersionEncodingStrategy = .succinct
try enc.encode(version)

[chriseplettsonos]

We could have a custom decoder that supports both format by trying them both.

I like this approach. Let me re-imagine the change this way (although it seems you did most of the work here ;) ).

Loaded question: which strategy do you think should be the default?

[chriseplettsonos]

I've updated this PR to add optional coding strategy support (in both directions).

I chose to require explicit use of strategy in both directions (even when decoding) to minimize possible performance impact.

Additionally, I chose to make .semverString the default strategy as that is, in my opinion, more generally useful. @finestructure if you strongly disagree with this choice, feel free to switch it back to .memberwise before merging.

[finestructure]

Thanks a lot for updating the PR with these changes, @chriseplettsonos!

I've thought about this some more and I believe that actually memberwise is the correct default coding strategy. Let me explain why.

I said initially that we'd choose semverString as the default if we did this over and didn't have data stored in the db but I've actually come around to that we wouldn't. The reason is that the default Codable format allows us to write the following SQL queries using Postgres' JSON operators:

select count(*) from builds where swift_version->'major' = '5' and swift_version->'minor' = '9'

We're able to use the fact that the version has already been parsed and are effectively encoding the result instead of the raw string. This has immense value, because SemVer parsing can be tricky and is not something you'll want to replicate in an SQL function when you've already parsed it once. The above query would be very hard to do if we'd chosen to store in the semverString format.

Considering this I would definitely not change the format if we did it over again.

I also don't think this should be the default format. I think it's important for the parser to store its actual parsed results in the codable format, even if they're 1:1 raw string representable because there's information in the parsed structure and making it available to consumers of the codable data that aren't Swift (like in the SQL snippet above) is a feature.

Given all that I think we should rename memberwise to defaultCodable (or perhaps simply default) and make it the default.

Does that make sense?

[chriseplettsonos]

Given all that I think we should rename memberwise to defaultCodable (or perhaps simply default) and make it the default.

Does that make sense?

I can see that, I guess. My thinking was that most web apis that serve up JSON are more likely to represent the version as a single string value, which makes semverString more useful in that scenario. But, since it's now an option, I guess it can work either way, so I don't feel super strongly about it.

I'll make the suggested changes

[finestructure]

Great, thanks a lot @chriseplettsonos !

[chriseplettsonos]

@finestructure any ETA on getting this in an official release? :)

[finestructure]

Done!

https://github.com/SwiftPackageIndex/SemanticVersion/releases/tag/0.4.0