Conversation
|
@onmyway133, thanks for your PR! By analyzing the history of the files in this pull request, we identified @vadymmarkov, @zenangst and @jessearmand to be potential reviewers. |
| } | ||
| } | ||
|
|
||
| extension MutableCollectionType where Index == Int { |
There was a problem hiding this comment.
Have a hard time extending MutableCollection, so extend on Array for now
| public protocol ErrorRoutable { | ||
|
|
||
| func handle(routeError: ErrorType, from currentController: Controller) | ||
| func handle(_ routeError: Error, from currentController: Controller) |
There was a problem hiding this comment.
I would leave routeError in function signature, or maybe just error.
| } | ||
|
|
||
| static func parseAsURL(url: NSURL, payload: Any? = nil) -> Location? { | ||
| static func parseAsURL(_ url: URL, payload: Any? = nil) -> Location? { |
There was a problem hiding this comment.
I think that parseAsURL is actually dealing with url components, so I rename it to parseComponents
|
|
||
| public static func navigate(urn: String, scheme: String = Compass.scheme) { | ||
| guard let url = NSURL(string: "\(scheme)\(urn)") else { return } | ||
| public static func navigate(_ urn: String, scheme: String = Compass.scheme) { |
|
|
||
| func replace(string: String, with withString: String) -> String { | ||
| return stringByReplacingOccurrencesOfString(string, withString: withString) | ||
| func replace(_ string: String, with withString: String) -> String { |
There was a problem hiding this comment.
Why do we need this method?
There was a problem hiding this comment.
It's just a syntactic sugar
There was a problem hiding this comment.
Yeah, but it makes less sense in Swift 3 I think.
There was a problem hiding this comment.
I agree, lets get rid of the Sugar that are fixed by Apple in Swift 3.
There was a problem hiding this comment.
@onmyway133 Would you mind to remove this functions an use replacingOccurrences(of: string, with: withString) instead?
|
|
||
| func openURL(URL: NSURL) { | ||
| NSWorkspace.sharedWorkspace().openURL(URL) | ||
| func openURL(_ url: URL) { |
There was a problem hiding this comment.
I think func open(url: URL) is better.
There was a problem hiding this comment.
Putting the _ seems to be the default everywhere tho. Which is a bit stupid, since, if everyone needs underscore, why don't we delete the underscore.
There was a problem hiding this comment.
@RamonGilabert I think the idea is to move the last part of the function's name to be an argument name, like in the example above (like in good old Objective C times) So we have to use _ as less as possible.
There was a problem hiding this comment.
Yeah, sorry, I thought you suggested func openURL(url: URL).
|
|
||
| func openURL(URL: NSURL) { | ||
| UIApplication.sharedApplication().openURL(URL) | ||
| func openURL(_ url: Foundation.URL) { |
There was a problem hiding this comment.
Don't need Foundation here.
|
Fixed as you suggest @vadymmarkov 😉 |
|
|
||
| public static func parse(url: NSURL, payload: Any? = nil) -> Location? { | ||
| let path = url.absoluteString.substringFromIndex(scheme.endIndex) | ||
| public static func parse(_ url: URL, payload: Any? = nil) -> Location? { |
There was a problem hiding this comment.
I would leave it like public static func parse(url: NSURL, payload: Any? = nil). Thought? @onmyway133 @zenangst @RamonGilabert ?
There was a problem hiding this comment.
Yeah, remove _. I like labels :)
| s.source_files = 'Sources/**/*' | ||
| s.frameworks = 'Foundation' | ||
|
|
||
| s.pod_target_xcconfig = { 'SWIFT_VERSION' => '3.0' } |
|
I've started a GitHub review here and now it's hard to read this PR tbh, it's not clear what is fixed and what is not 😞 But there are 2 comments waiting for your reply @onmyway133 😄 |
|
|
||
| #if os(OSX) | ||
| public typealias Controller = NSViewController | ||
| public typealias CurrentController = NSViewController |
There was a problem hiding this comment.
We preserve Controller for another use case?
There was a problem hiding this comment.
I had a conflict with Controller in Spots so I decided to change this one to no "kidnap" the name.
There was a problem hiding this comment.
@zenangst oh ja, because we have struct Compass so the namespace is lost
There was a problem hiding this comment.
I think we should try and fix that thing, now is probably a good idea to address it as we are already doing some breaking changes in the migration. (breaking as in having to adopt new Swift 3 syntax).
5 participants
NSWorkspace.shared().open(url)vsUIApplication.shared.openURL(url)