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

Migrate to Swift 4's Decodable APIs #84

Merged
merged 64 commits into from Oct 17, 2017

Conversation

Projects
None yet
4 participants
@Palleas
Copy link
Collaborator

Palleas commented Jun 15, 2017

  • Add CI target for Swift 4 in travis (cf #88)
  • Use Swift 4 Codable protocol
  • Update tests
  • Remove duplicate Result submodule

Let me know if I forgot something in the list of stuff to do!

@Palleas Palleas force-pushed the swift-4 branch from a6be31b to c0c74e4 Jun 16, 2017

@Palleas

This comment has been minimized.

Copy link
Collaborator

Palleas commented Jun 17, 2017

screen shot 2017-06-17 at 5 29 28 pm

Getting there!

@Palleas

This comment has been minimized.

Copy link
Collaborator

Palleas commented Jun 18, 2017

I'm running the unit tests now and I have an issue. Basically since we treat id in all the resources as String while the JSON returns Int, the decoding fails. The only solution I've found so far is doing is for Release:

    public init(from decoder: Decoder) throws {
        let container = try decoder.container(keyedBy: CodingKeys.self)
        let id = try container.decode(Int.self, forKey: .id)
        self.id = "\(id)"
        self.tag = try container.decode(String.self, forKey: .tag)
        self.url = try container.decode(URL.self, forKey: .url)
        self.name = try container.decode(String.self, forKey: .tag)
        self.isDraft = try container.decode(Bool.self, forKey: .isDraft)
        self.isPrerelease = try container.decode(Bool.self, forKey: .isPrerelease)
        self.assets = try container.decode([Asset].self, forKey: .assets)
    }

Do you see a different approach we could take with this? Right now the only solution I see is going back to treating id as Int.  ¯\(ツ)

@mdiep

This comment has been minimized.

Copy link
Owner

mdiep commented Jun 18, 2017

I think that's likely the only solution. I don't think the built-in JSON encoding is as flexible.

@mdiep

This comment has been minimized.

Copy link
Owner

mdiep commented Jun 18, 2017

On second thought: I think we should add a property type.

extension Release {
    public struct ID {
        private let id: Int
        public var string: String {
            return "\(id)"
        }
    }
}

That might make decoding easier?

@Palleas

This comment has been minimized.

Copy link
Collaborator

Palleas commented Jun 18, 2017

@mdiep Ha! That's exactly what we were talking about in Slack, Cf: https://gist.github.com/Palleas/645b6c30306603feaab148c9e167a307

Although since it will happen for all the Resources, maybe we need to move it to a higher level?

@mdiep

This comment has been minimized.

Copy link
Owner

mdiep commented Jun 18, 2017

I think it makes sense to have a per-type ID because it adds additional type safety. You can't accidentally pass an ID for the wrong type of resource to an API.

But if it made sense, we could add a top-level protocol that they all conform to.

@Palleas Palleas force-pushed the swift-4 branch from e6ac4b7 to b431236 Jun 18, 2017

@Palleas Palleas requested a review from mdiep Jun 22, 2017

@mdiep
Copy link
Owner

mdiep left a comment

Really neat to see this with the built-in Swift Decodable stuff! 👏

.filter { _, value in value != nil }
.map { name, value in URLQueryItem(name: name, value: "\(value!)") }
.filter { arg -> Bool in let (_, value) = arg; return value != nil }
.map { arg -> URLQueryItem in let (name, value) = arg; return URLQueryItem(name: name, value: "\(value!)") }

This comment has been minimized.

@mdiep

mdiep Jun 28, 2017

Owner

If we move to a newer Swift build, we should be able to revert this change since SE-0110 was reverted.

private func execute<Value>(_ request: Request<Value>, page: UInt?, perPage: UInt?) -> SignalProducer<(Response, Any), Error> {
return urlSession
private func execute<Value: Decodable>(_ request: Request<Value>, page: UInt?, perPage: UInt?) -> SignalProducer<(Response, Data), Error> {
let s = urlSession

This comment has been minimized.

@mdiep

mdiep Jun 28, 2017

Owner

Should be able to remove this local variable and go back to returning directly?


if httpResponse.statusCode >= 400 && httpResponse.statusCode < 600 {
let error: Result<GitHubError, Error> = decode(data).mapError { Error.jsonDecodingError($0) }
return .failure(.apiError(httpResponse.statusCode, githubResponse, error.value!))

This comment has been minimized.

@mdiep

mdiep Jun 28, 2017

Owner

This ! isn't really safe. We should go back to the previous structure using flatMap.

let s: SignalProducer<(Response, Data), Error> = execute(request, page: nil, perPage: nil)

return s.attemptMap({ (arg) -> Result<(Response, Resource), Client.Error> in
let (response, data) = arg

This comment has been minimized.

@mdiep

mdiep Jun 28, 2017

Owner

Same here WRT SE-0110.

.mapError(Error.jsonDecodingError)
}
) -> SignalProducer<(Response, Resource), Error> {
let s: SignalProducer<(Response, Data), Error> = execute(request, page: nil, perPage: nil)

This comment has been minimized.

@mdiep

mdiep Jun 28, 2017

Owner

Should be able to remove s and just return.

import Foundation

public struct Homepage: Decodable {

This comment has been minimized.

@mdiep

mdiep Jun 28, 2017

Owner

Maybe make this an enum instead?

enum Homepage {
    case url(URL)
    case string(String)
}
private enum CodingKeys: String, CodingKey {
case name
case color
}

This comment has been minimized.

@mdiep

mdiep Jun 28, 2017

Owner

Do we need these since they match what would be generated?

// return
// }
//
// XCTAssertEqual(expected, decoded)

This comment has been minimized.

@mdiep
// ])
//
// let encoded = file.encode()
// XCTAssertEqual(expected, encoded)

This comment has been minimized.

@mdiep
// ])
//
// let encoded = file.encode()
// XCTAssertEqual(expected, encoded)

This comment has been minimized.

@mdiep
.gitmodules Outdated
[submodule "Carthage/Checkouts/Runes"]
path = Carthage/Checkouts/Runes
url = https://github.com/thoughtbot/Runes.git
[submodule "Carthage/Checkouts/result"]

This comment has been minimized.

@dcaunt

dcaunt Jun 30, 2017

Collaborator

Result is now duplicated in this file, and (nit) it should be capitalized Result

@Palleas

This comment has been minimized.

Copy link
Collaborator

Palleas commented Jul 4, 2017

Thank you all for the feedback, I'll take care of most of them this week. The rollback of SE-0110 should be in the next Xcode seed right?

@mdiep

This comment has been minimized.

Copy link
Owner

mdiep commented Jul 4, 2017

The rollback of SE-0110 should be in the next Xcode seed right?

I think so, yes. Otherwise you can probably download a snapshot from swift.org.

@Palleas

This comment has been minimized.

Copy link
Collaborator

Palleas commented Jul 10, 2017

Xcode 9b3 reverts SE-110 so I'll tackle all the feedback this week!

@Palleas Palleas force-pushed the swift-4 branch from a8be0cd to a35d0b7 Aug 18, 2017

import Foundation
import ReactiveSwift
import Result

//extension Array where Element == ResourceType : ResourceType {}

This comment has been minimized.

@mdiep
.filter { _, value in value != nil }
.map { name, value in URLQueryItem(name: name, value: "\(value!)") }
.filter { (_, value) -> Bool in value != nil }
.map { (name, value) -> URLQueryItem in URLQueryItem(name: name, value: "\(value!)") }

This comment has been minimized.

@mdiep

mdiep Aug 25, 2017

Owner

You should be able to remove these return types?

This comment has been minimized.

@Palleas

Palleas Aug 27, 2017

Collaborator

Done!

}

return current.concat(self.execute(request, page: nextPage, perPage: perPage))
})

This comment has been minimized.

@mdiep

mdiep Aug 25, 2017

Owner

Trailing closure syntax here, please. 🙏

This comment has been minimized.

@Palleas

Palleas Aug 27, 2017

Collaborator

Not sure how I missed it, fixed!

@@ -272,7 +266,7 @@ extension Client.Error: Hashable {
return (error1 as NSError) == (error2 as NSError)

case let (.jsonDecodingError(error1), .jsonDecodingError(error2)):
return error1 == error2
return error1 == error2 // FIXME

This comment has been minimized.

@mdiep

mdiep Aug 25, 2017

Owner

Why is this labeled FIXME?

case mode
case url
case size
}

This comment has been minimized.

@mdiep

mdiep Aug 25, 2017

Owner

This shouldn't be needed since all the cases have the default value?

This comment has been minimized.

@Palleas

Palleas Aug 27, 2017

Collaborator

If think they are because url and size are not exposed as properties and we are using them for encoding, but maybe I'm missing something..?

@mdiep

This comment has been minimized.

Copy link
Owner

mdiep commented Aug 25, 2017

I'm liking these changes!

If it'd be helpful, we could add ID/Identifiable in a separate PR before this one.

Palleas added some commits Jun 15, 2017

import Foundation

public enum Homepage: Decodable {

This comment has been minimized.

@ikesyo

ikesyo Sep 28, 2017

Collaborator

What is the purpose of this type? Keep using URL seems better to me.

This comment has been minimized.

@dcaunt

dcaunt Sep 28, 2017

Collaborator

Unless the API is expected to respond with an arbitrary String and not a valid URL, I agree.

This comment has been minimized.

@ikesyo

ikesyo Sep 28, 2017

Collaborator

I understand now that the problem here is empty string handling (""). Empty string is not valid input even if a property is URL?.

This comment has been minimized.

@dcaunt

dcaunt Sep 28, 2017

Collaborator

In that case it seems like we should revert to URL? and change the decoding?

This comment has been minimized.

@mdiep

mdiep Oct 6, 2017

Owner

GitHub will let you set this field to any string.

BUT your github.com profile will only show the string if it's actually a URL. So I think we could revert to a URL and just set it to nil if it's not a valid URL.

This comment has been minimized.

@dcaunt

dcaunt Oct 6, 2017

Collaborator

Pushed aa43141 to address this.

This comment has been minimized.

@Palleas

Palleas Oct 7, 2017

Collaborator

Oops, missed this, removed my comment 🙊
Are we sure it’s the same with a repository? I remember having this conversation a while ago 🤔

ikesyo and others added some commits Sep 28, 2017


if let base = base {
try container.encode(base, forKey: .base)
}

This comment has been minimized.

@Palleas

Palleas Oct 7, 2017

Collaborator

TIL!

@mdiep
Copy link
Owner

mdiep left a comment

Looks good overall! Thanks for tackling this! It's no small effort. 🎉

.flatMap(.concat) { response, data -> SignalProducer<(Response, [Resource]), Error> in
let current = SignalProducer<(Response, [Resource]), Error>(value: (response, data))
guard let _ = response.links["next"] else {
return current.concat(.empty)

This comment has been minimized.

@mdiep

mdiep Oct 10, 2017

Owner

.concat(.empty) is effectively a noop. This can just be return current.

This comment has been minimized.

@Palleas

Palleas Oct 10, 2017

Collaborator

Fixed!

/// SHA of the commit
public let sha: SHA
public let sha: Hash

This comment has been minimized.

@mdiep

mdiep Oct 10, 2017

Owner

Why did this change from a SHA?

This comment has been minimized.

@Palleas

Palleas Oct 10, 2017

Collaborator

Because it’s not exactly the same type IIRC

This comment has been minimized.

@mdiep

mdiep Oct 15, 2017

Owner

Can you elaborate on this? I think it should be the same thing.

/// URL to see the parent commit in a browser
public let url: URL

/// SHA of the parent commit
public let sha: SHA
public let sha: Hash

This comment has been minimized.

@mdiep

mdiep Oct 10, 2017

Owner

Why did this change from a SHA?

debugDescription: "Invalid content-type \(type)"
)
}
}

This comment has been minimized.

@mdiep

This comment has been minimized.

@Palleas

Palleas Oct 10, 2017

Collaborator

How else would you handle this? 🤔

This comment has been minimized.

@Palleas

Palleas Oct 10, 2017

Collaborator

Made a proposal in 261c768, let me know what you think. At least we're no longer switching over strings.

extension DecodingError: Equatable {
static public func ==(lhs: DecodingError, rhs: DecodingError) -> Bool {
switch (lhs, rhs) {
default: return false // FIXME

This comment has been minimized.

@mdiep

mdiep Oct 10, 2017

Owner

FIXME

This comment has been minimized.

@Palleas

Palleas Oct 10, 2017

Collaborator

Any idea how to do proper Equatable implementation for DecodingError? 🤔

This comment has been minimized.

@Palleas

Palleas Oct 10, 2017

Collaborator

I have a proposition suggested to me in Slack in f9d214e, let me know what you think!

}

// TODO: remove this, or replace `Color` with something (De)codable
private enum CodingKeys: String, CodingKey {

This comment has been minimized.

@mdiep

mdiep Oct 10, 2017

Owner

I'm fine with being explicit. 👍

self.color = color
}

// TODO: remove this, or replace `Color` with something (De)codable

This comment has been minimized.

@mdiep

mdiep Oct 10, 2017

Owner

I think this comment can go away?

self.owner = try container.decode(UserInfo.self, forKey: .owner)
self.name = try container.decode(String.self, forKey: .name)
self.nameWithOwner = try container.decode(String.self, forKey: .nameWithOwner)
self.body = try? container.decode(String.self, forKey: .body)

This comment has been minimized.

@mdiep

mdiep Oct 10, 2017

Owner

I think this should be try container.decodeIfPresent?

This comment has been minimized.

@dcaunt

dcaunt Oct 10, 2017

Collaborator

Yeah, it should 😞

This comment has been minimized.

@Palleas

Palleas Oct 10, 2017

Collaborator

Fixed.

case isTruncated = "truncated"
}

public struct SHA: Codable {

This comment has been minimized.

@mdiep

mdiep Oct 10, 2017

Owner

We shouldn't need this one either. A single SHA type should be enough.

Palleas added some commits Oct 10, 2017

@Palleas

This comment has been minimized.

Copy link
Collaborator

Palleas commented Oct 10, 2017

I remember why there are 2 SHAs:

On Branch, we have:

  • Branch
    • Commit
      • Sha (String)

But on Tree, we have:

  • Tree
    • Sha (String)

I'll change the way it works for Branch, it feels like a hack.

Palleas added some commits Oct 10, 2017

self.nameWithOwner = try container.decode(String.self, forKey: .nameWithOwner)
self.body = try container.decodeIfPresent(String.self, forKey: .body)
self.url = try container.decode(URL.self, forKey: .url)
self.homepage = try container.decodeIfPresent(URL.self, forKey: .homepage)

This comment has been minimized.

@ikesyo

ikesyo Oct 10, 2017

Collaborator

This doesn't work for empty string. That should be why try? was used here.

This comment has been minimized.

@mdiep

mdiep Oct 10, 2017

Owner

That doesn't totally match the intent, since a string that's not a URL should be nil, but a number should throw. But it's probably close enough. 🤷‍♂️

This comment has been minimized.

@Palleas

Palleas Oct 11, 2017

Collaborator

Soooo, back to try? is what you meant?

This comment has been minimized.

@mdiep

mdiep Oct 11, 2017

Owner

Yeah, just for homepage.

This comment has been minimized.

@Palleas

Palleas Oct 11, 2017

Collaborator

Done!

Palleas added some commits Oct 11, 2017

@Palleas

This comment has been minimized.

Copy link
Collaborator

Palleas commented Oct 11, 2017

Also fixed the remaining failing test (in Tree encoding)

@mdiep
Copy link
Owner

mdiep left a comment

The question of the SHA types is really the only remaining thing.

/// Sha of the commit the branch points to
public let sha: SHA
/// The commit the branch points to
public let commit: Commit

This comment has been minimized.

@mdiep

mdiep Oct 15, 2017

Owner

I'm still confused about why this isn't just a SHA. 🙈

This comment has been minimized.

@Palleas

Palleas Oct 15, 2017

Collaborator

This is what the payload returns:

[
  {
    "name": "master",
    "commit": {
      "sha": "6dcb09b5b57875f334f61aebed695e2e4193db5e",
      "url": "https://api.github.com/repos/octocat/Hello-World/commits/c5b97d5ae6c19d5c5df71a34c7fbeeda2479ccbc"
    },
    "protected": true,
    "protection_url": "https://api.github.com/repos/octocat/Hello-World/branches/master/protection"
  }
]

I can retrieve the sha directly with a custom init, is that what you have in mind? Maybe I'm just missing something  🤷‍♂️

This comment has been minimized.

@mdiep

mdiep Oct 17, 2017

Owner

Ah, okay! I didn't realize there was another property returned on commit! 😅

.mapError(Error.jsonDecodingError)
.flatMap { error in
.failure(Error.apiError(response.statusCode, Response(headerFields: headers), error))
}

This comment has been minimized.

@mdiep

mdiep Oct 15, 2017

Owner

This } is under indented.

/// SHA of the commit
public let sha: SHA
public let sha: Hash

This comment has been minimized.

@mdiep

mdiep Oct 15, 2017

Owner

Can you elaborate on this? I think it should be the same thing.

Palleas added some commits Oct 15, 2017

@mdiep

mdiep approved these changes Oct 17, 2017

/// Sha of the commit the branch points to
public let sha: SHA
/// The commit the branch points to
public let commit: Commit

This comment has been minimized.

@mdiep

mdiep Oct 17, 2017

Owner

Ah, okay! I didn't realize there was another property returned on commit! 😅

@mdiep

This comment has been minimized.

Copy link
Owner

mdiep commented Oct 17, 2017

This is great! Thank you SO SO SO much for doing this. It's great to see what Swift's built-in decoding looks like, warts and all.

Sorry about the long review times and my general cluelessness in this PR. Things have been a little crazy here. 😅

@mdiep mdiep merged commit 90c5757 into master Oct 17, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@mdiep mdiep deleted the swift-4 branch Oct 17, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment