-
Notifications
You must be signed in to change notification settings - Fork 24
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
Upgrade to Swift 3 #43
Conversation
let f = curry(RepositoryInfo.init) | ||
|
||
return f | ||
let ff = f |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to understand what's happening, you're splitting the mapping so it's less complicated for Swift?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exactly. Without the change, the files can't be compiled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great! Thanks for working on this.
I noted the API changes I think we need for the Swift 3 style. I'm happy to make them here or elsewhere if you don't want to do it here.
} | ||
|
||
/// Fetch the user with the given login. | ||
public func userWithLogin(login: String) -> SignalProducer<(Response, UserInfo), Error> { | ||
return fetchOne(.UserInfo(login: login)) | ||
public func userWithLogin(_ login: String) -> SignalProducer<(Response, UserInfo), Error> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should become user(login:)
} | ||
|
||
public func issuesInRepository(repository: Repository, page: UInt = 1, perPage: UInt = 30) -> SignalProducer<(Response, [Issue]), Error> { | ||
public func issuesInRepository(_ repository: Repository, page: UInt = 1, perPage: UInt = 30) -> SignalProducer<(Response, [Issue]), Error> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should become issues(in:page:perPage:)
} | ||
|
||
/// Fetch the comments posted on an issue | ||
public func commentsOnIssue(issue: Int, repository: Repository, page: UInt = 1, perPage: UInt = 30) -> SignalProducer<(Response, [Comment]), Error> { | ||
public func commentsOnIssue(_ issue: Int, repository: Repository, page: UInt = 1, perPage: UInt = 30) -> SignalProducer<(Response, [Comment]), Error> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should become comments(on:repository:page:perPage:)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the first parameter is Int
, this should be considered: https://swift.org/documentation/api-design-guidelines/#weak-type-information.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I consistently used in
for the second parameter of Repository
type. How do you think about it?
} | ||
|
||
/// Fetch the repositories for a specific user | ||
public func repositoriesForUser(user: String, page: UInt = 1, perPage: UInt = 30) -> SignalProducer<(Response, [RepositoryInfo]), Error> { | ||
return fetchMany(.RepositoriesForUser(user: user), page: page, pageSize: perPage) | ||
public func repositoriesForUser(_ user: String, page: UInt = 1, perPage: UInt = 30) -> SignalProducer<(Response, [RepositoryInfo]), Error> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should become repositories(for:page:perPage:)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the first parameter is String
, this should be considered: https://swift.org/documentation/api-design-guidelines/#weak-type-information.
} | ||
|
||
/// Fetch the repositories for a specific organisation | ||
public func repositoriesForOrganization(organization: String, page: UInt = 1, perPage: UInt = 30) -> SignalProducer<(Response, [RepositoryInfo]), Error> { | ||
return fetchMany(.RepositoriesForOrganization(organization: organization), page: page, pageSize: perPage) | ||
public func repositoriesForOrganization(_ organization: String, page: UInt = 1, perPage: UInt = 30) -> SignalProducer<(Response, [RepositoryInfo]), Error> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should become repositories(for:page:perPage:)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the first parameter is String
, this should be considered: https://swift.org/documentation/api-design-guidelines/#weak-type-information.
Thanks @ikesyo! We'll want to wait for a stable version of ReactiveSwift before we release this, but this is great! |
Although I don't fully audit API naming nor addthis looks working well.@available
s yet,