Upgrade to swift 4 #663

bsudekum · 2017-09-27T20:35:34Z

This begins the process of upgrading to swift 4.

1ec5

Would it be possible to maintain Swift 3 support by changing SWIFT_VERSION back to 3.1 and enclosing any incompatible code in #if swift checks?

bsudekum · 2017-09-27T20:49:19Z

enclosing any incompatible code in #if swift checks?

Let's see how many there are. This could become very tiresome if it's a lot of code. The other option is to maintain a branch for x more months... and with a set EOL.

frederoni · 2017-09-27T20:56:03Z

MapboxCoreNavigationTests/GeometryTests.swift

@@ -190,7 +190,7 @@ class GeometryTests: XCTestCase {

        // https://github.com/Turfjs/turf/blob/142e137ce0c758e2825a260ab32b24db0aa19439/packages/turf-distance/test.js
        let a = distance(along: line)
-        XCTAssertEqualWithAccuracy(a, 97_159.57803131901, accuracy: 1)
+        XCTAssertEqual(a, 97_159.57803131901, accuracy: 1)


This won't build using an earlier version.
Try adding: https://github.com/mapbox/turf-swift/blob/e17d1d05c08796536dea0b36a38b9c2bbde0fb06/TurfTests/TurfTests.swift#L7-L12

frederoni · 2017-09-27T20:56:25Z

Should we add an additional Bitrise app with the new Xcode 9 stack?

bsudekum · 2017-09-27T20:59:07Z

Yes, we should test both.

bsudekum · 2017-09-27T22:42:35Z

Added bitrise test for xcode 9. It will only run on this branch.

bsudekum · 2017-09-28T00:16:23Z

So I added quite a few @objc declarations but there are still a handful of public methods that are not bridging.

bsudekum · 2017-09-28T15:31:46Z

A line line https://github.com/mapbox/mapbox-navigation-ios/pull/663/files#diff-851f438872328e98cbe395ac0733c16bR130 will not accept the prefix @objc because it doesn't bridge. However, I guess it never bridged to begin with?

1ec5 · 2017-09-28T18:54:00Z

Not sure which line you’re referring to, but there are probably a number of methods that never bridged to Objective-C. Let’s track those issues separately. Making these situations explicit is one of the benefits of Swift 4’s @objc behavior.

JThramer · 2017-10-09T22:45:25Z

MapboxCoreNavigation/NavigationLocationManager.swift

@@ -17,13 +17,13 @@ open class NavigationLocationManager: CLLocationManager {
    /**
     Indicates whether the device is plugged in or not.
     */
-    public private(set) var isPluggedIn: Bool = false
+    @objc public private(set) var isPluggedIn: Bool = false


private(set)

I did not know you could do that.

bsudekum · 2017-10-23T15:51:07Z

We'll need to update MapboxDirections.swift to swift 4 before merging this.

bsudekum · 2017-10-24T17:21:52Z

This is blocked by CocoaPods/CocoaPods#7134

1ec5 · 2017-10-24T18:03:56Z

Is it really blocked? This PR was designed to work when compiled as Swift 3, so we could have Carthage compiling it as Swift 4 and CocoaPods compiling it as Swift 3 for the time being.

bsudekum · 2017-10-24T18:11:01Z

@1ec5 I guess that would work.

bsudekum · 2017-11-20T21:13:27Z

Cartfile

@@ -2,7 +2,7 @@ binary "https://www.mapbox.com/ios-sdk/Mapbox-iOS-SDK.json" ~> 3.6
 github "mapbox/MapboxDirections.swift" ~> 0.12
 github "mapbox/turf-swift" ~> 0.0.3
 github "rs/SDWebImage" ~> 4.1
-github "Project-OSRM/osrm-text-instructions.swift" ~> 0.5
+github "Project-OSRM/osrm-text-instructions.swift" "f60d5bc175794aee6c7199d60548820e0fb23097"


Will change when Project-OSRM/osrm-text-instructions.swift#50 is released.

bsudekum · 2017-11-21T00:56:01Z

@mapbox/navigation-ios this is ready for review.

1ec5

This PR is functionally sound, but there are a number of places where running the migrator resulted in unnecessarily verbose code.

1ec5 · 2017-11-21T02:48:47Z

Examples/Objective-C/ViewController.m

@@ -126,7 +126,6 @@ -(void)prepareForSegue:(UIStoryboardSegue *)segue sender:(id)sender {

        MGLPointAnnotation *destination = [[MGLPointAnnotation alloc] init];
        destination.coordinate = self.destination;
-        controller.destination = destination;


Why was this line removed? Doesn’t this cause the preceding two lines to be useless?

destination was deprecated eons ago.

So shouldn’t the two lines above be removed too?

1ec5 · 2017-11-21T02:49:59Z

MapboxCoreNavigationTests/MapboxCoreNavigationTests.swift

@@ -21,7 +21,7 @@ class MapboxCoreNavigationTests: XCTestCase {
        navigation.resume()
        let depart = CLLocation(coordinate: CLLocationCoordinate2D(latitude: 37.795042, longitude: -122.413165), altitude: 1, horizontalAccuracy: 1, verticalAccuracy: 1, course: 0, speed: 10, timestamp: Date())

-        self.expectation(forNotification: RouteControllerDidPassSpokenInstructionPoint.rawValue, object: navigation) { (notification) -> Bool in
+        self.expectation(forNotification: NSNotification.Name(rawValue: RouteControllerDidPassSpokenInstructionPoint.rawValue), object: navigation) { (notification) -> Bool in


Use RouteControllerDidPassSpokenInstructionPoint directly instead of converting to a raw value and back to an NSNotification.Name (×4).

1ec5 · 2017-11-21T02:51:07Z

MapboxNavigation/BottomBannerViewLayout.swift

@@ -6,13 +6,13 @@ extension BottomBannerView {

        let timeRemainingLabel = TimeRemainingLabel()
        timeRemainingLabel.translatesAutoresizingMaskIntoConstraints = false
-        timeRemainingLabel.font = .systemFont(ofSize: 28, weight: UIFontWeightMedium)
+        timeRemainingLabel.font = .systemFont(ofSize: 28, weight: UIFont.Weight.medium)


Nit: drop UIFont.Weight (×2).

1ec5 · 2017-11-21T02:51:37Z

MapboxNavigation/DayStyle.swift

@@ -58,22 +58,22 @@ open class DayStyle: Style {
            tintColor = .defaultTint
        }

-        ArrivalTimeLabel.appearance().font = UIFont.systemFont(ofSize: 18, weight: UIFontWeightMedium).adjustedFont
+        ArrivalTimeLabel.appearance().font = UIFont.systemFont(ofSize: 18, weight: UIFont.Weight.medium).adjustedFont


Nit: drop UIFont.Weight (×10).

1ec5 · 2017-11-21T02:52:19Z

MapboxNavigation/InstructionLabel.swift

-    var attributes: [String: Any] {
-        return [NSFontAttributeName: font, NSForegroundColorAttributeName: textColor]
+    var attributes: [NSAttributedStringKey: Any] {
+        return [NSAttributedStringKey.font: font, NSAttributedStringKey.foregroundColor: textColor]


Nit: drop NSAttributedStringKey.

1ec5 · 2017-11-21T02:54:34Z

MapboxNavigation/Style.swift

-        let valueAttributes: [String: Any] = [NSForegroundColorAttributeName: valueTextColor, NSFontAttributeName: valueFont]
-        let unitAttributes: [String: Any] = [NSForegroundColorAttributeName: unitTextColor, NSFontAttributeName: unitFont]
+        let valueAttributes: [NSAttributedStringKey: Any] = [NSAttributedStringKey.foregroundColor: valueTextColor, NSAttributedStringKey.font: valueFont]
+        let unitAttributes: [NSAttributedStringKey: Any] = [NSAttributedStringKey.foregroundColor: unitTextColor, NSAttributedStringKey.font: unitFont]


Nit: drop NSAttributedStringKey from both these lines.

1ec5 · 2017-11-21T02:55:55Z

MapboxNavigation/UIFont.swift

@@ -37,14 +37,14 @@ extension UIFont {

    func with(fontFamily: String?) -> UIFont {
        guard let fontFamily = fontFamily else { return self }
-        let weight = (fontDescriptor.object(forKey: UIFontDescriptorTraitsAttribute) as! [String:Any])[UIFontWeightTrait]
-        let attributes = [UIFontDescriptorTraitsAttribute: [UIFontWeightTrait: weight]]
+        let weight = (fontDescriptor.object(forKey: UIFontDescriptor.AttributeName.traits) as! [String:Any])[UIFontDescriptor.TraitKey.weight.rawValue]


Nit: drop UIFontDescriptor.AttributeName.

1ec5 · 2017-11-21T02:56:41Z

MapboxNavigation/UIFont.swift

-        let weight = (fontDescriptor.object(forKey: UIFontDescriptorTraitsAttribute) as! [String:Any])[UIFontWeightTrait]
-        let attributes = [UIFontDescriptorTraitsAttribute: [UIFontWeightTrait: weight]]
+        let weight = (fontDescriptor.object(forKey: UIFontDescriptor.AttributeName.traits) as! [String:Any])[UIFontDescriptor.TraitKey.weight.rawValue]
+        let attributes = [UIFontDescriptor.AttributeName.traits: [UIFontDescriptor.TraitKey.weight: weight]]


Inline this variable so that you can drop UIFontDescriptor.AttributeName and possibly UIFontDescriptor.TraitKey.

1ec5 · 2017-11-21T02:56:55Z

MapboxNavigation/UIFont.swift

        let descriptor = UIFontDescriptor(name: fontName, size: pointSize).withFamily(fontFamily).addingAttributes(attributes)
        return UIFont(descriptor: descriptor, size: pointSize)
    }

    func with(weight: CGFloat) -> UIFont {
-        let font = UIFont(descriptor: fontDescriptor.addingAttributes([UIFontWeightTrait: weight]), size: pointSize)
+        let font = UIFont(descriptor: fontDescriptor.addingAttributes([UIFontDescriptor.AttributeName.traits: weight]), size: pointSize)


Nit: drop UIFontDescriptor.AttributeName.

1ec5 · 2017-11-21T02:57:15Z

MapboxNavigationTests/InstructionsBannerViewTests.swift

-        view.primaryLabel.font = UIFont.systemFont(ofSize: 30, weight: UIFontWeightMedium)
-        view.secondaryLabel.font = UIFont.systemFont(ofSize: 26, weight: UIFontWeightMedium)
+        view.primaryLabel.font = UIFont.systemFont(ofSize: 30, weight: UIFont.Weight.medium)
+        view.secondaryLabel.font = UIFont.systemFont(ofSize: 26, weight: UIFont.Weight.medium)


Nit: drop UIFont.Weight from both these lines.

bsudekum · 2017-11-21T15:36:47Z

@1ec5 fixed.

1ec5 · 2017-11-21T18:30:10Z

MapboxCoreNavigationTests/MapboxCoreNavigationTests.swift

@@ -43,7 +43,7 @@ class MapboxCoreNavigationTests: XCTestCase {
        let locationManager = ReplayLocationManager(locations: [location, location])
        let navigation = RouteController(along: route, directions: directions, locationManager: locationManager)

-        self.expectation(forNotification: NSNotification.Name(rawValue: RouteControllerDidPassSpokenInstructionPoint.rawValue), object: navigation) { (notification) -> Bool in
+        self.expectation(forNotification: RouteControllerDidPassSpokenInstructionPoint), object: navigation) { (notification) -> Bool in


Unbalanced parentheses are causing a build failure (×3).

1ec5 · 2017-11-22T18:59:46Z

Fixed a serving or two of copypasta in fcb0e12c0434a574d77a655c25723004c7b3f27b and ac5f85e2e8da6c710a0f902cedecacd4f83a6eb6.

1ec5 reviewed Sep 27, 2017

View reviewed changes

frederoni reviewed Sep 27, 2017

View reviewed changes

bsudekum force-pushed the swift4 branch 2 times, most recently from b4f150f to e35203a Compare September 27, 2017 22:21

bsudekum changed the title ~~Upgrade to swift 4~~ Upgrade to swift 4, allow swift 3 Sep 28, 2017

bsudekum force-pushed the swift4 branch 2 times, most recently from 1e22f1a to 7d7ba69 Compare September 29, 2017 17:06

frederoni mentioned this pull request Oct 4, 2017

Example application is incompatible with CocoaPods #583

Closed

JThramer reviewed Oct 9, 2017

View reviewed changes

JThramer approved these changes Oct 9, 2017

View reviewed changes

bsudekum force-pushed the swift4 branch from 7d7ba69 to b5f2535 Compare October 20, 2017 22:28

bsudekum force-pushed the swift4 branch from 2dcdc33 to 52cae98 Compare November 14, 2017 18:03

bsudekum commented Nov 20, 2017

View reviewed changes

bsudekum added the ✓ ready for review label Nov 21, 2017

1ec5 mentioned this pull request Nov 21, 2017

Facing an issues with PolyLine raphaelmor/Polyline#48

Closed

1ec5 reviewed Nov 21, 2017

View reviewed changes

1ec5 changed the title ~~Upgrade to swift 4, allow swift 3~~ Upgrade to swift 4 Nov 21, 2017

1ec5 suggested changes Nov 21, 2017

View reviewed changes

1ec5 approved these changes Nov 21, 2017

View reviewed changes

captainbarbosa mentioned this pull request Nov 21, 2017

Add tutorial code from mapbox.com/help mapbox/ios-sdk-examples#127

Merged

1ec5 force-pushed the swift4 branch from ac5f85e to c18f652 Compare November 22, 2017 21:03

1ec5 added build Issues related to builds and dependency management. and removed ✓ ready for review labels Nov 22, 2017

Bobby Sudekum and others added 5 commits November 22, 2017 13:24

swift 4

4080b9c

Fixed syntax errors

0fa0531

Removed vestigial destination annotation

4d1aca7

Specify a version rather than a tag

89231f5

Updated README

b176bbf

1ec5 force-pushed the swift4 branch 2 times, most recently from b176bbf to e7e45da Compare November 22, 2017 21:45

Expose more symbols to Objective-C

ce43472

1ec5 force-pushed the swift4 branch from e7e45da to ce43472 Compare November 22, 2017 22:18

1ec5 merged commit f8081f8 into master Nov 22, 2017

1ec5 deleted the swift4 branch November 22, 2017 23:33

1ec5 added the backwards incompatible changes that break backwards compatibility of public API label Dec 18, 2018

1ec5 added this to the v0.11.0 milestone Dec 18, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade to swift 4 #663

Upgrade to swift 4 #663

bsudekum commented Sep 27, 2017

1ec5 left a comment

bsudekum commented Sep 27, 2017

frederoni Sep 27, 2017

frederoni commented Sep 27, 2017

bsudekum commented Sep 27, 2017

bsudekum commented Sep 27, 2017

bsudekum commented Sep 28, 2017 •

edited

Loading

bsudekum commented Sep 28, 2017

1ec5 commented Sep 28, 2017

JThramer Oct 9, 2017

bsudekum commented Oct 23, 2017

bsudekum commented Oct 24, 2017

1ec5 commented Oct 24, 2017

bsudekum commented Oct 24, 2017

bsudekum Nov 20, 2017

bsudekum commented Nov 21, 2017

1ec5 left a comment

1ec5 Nov 21, 2017

bsudekum Nov 21, 2017

1ec5 Nov 21, 2017

1ec5 Nov 21, 2017

1ec5 Nov 21, 2017

1ec5 Nov 21, 2017

1ec5 Nov 21, 2017

1ec5 Nov 21, 2017

1ec5 Nov 21, 2017

1ec5 Nov 21, 2017

1ec5 Nov 21, 2017

1ec5 Nov 21, 2017

bsudekum commented Nov 21, 2017

1ec5 Nov 21, 2017

1ec5 commented Nov 22, 2017

Upgrade to swift 4 #663

Upgrade to swift 4 #663

Conversation

bsudekum commented Sep 27, 2017

1ec5 left a comment

Choose a reason for hiding this comment

bsudekum commented Sep 27, 2017

Choose a reason for hiding this comment

frederoni commented Sep 27, 2017

bsudekum commented Sep 27, 2017

bsudekum commented Sep 27, 2017

bsudekum commented Sep 28, 2017 • edited Loading

bsudekum commented Sep 28, 2017

1ec5 commented Sep 28, 2017

Choose a reason for hiding this comment

bsudekum commented Oct 23, 2017

bsudekum commented Oct 24, 2017

1ec5 commented Oct 24, 2017

bsudekum commented Oct 24, 2017

Choose a reason for hiding this comment

bsudekum commented Nov 21, 2017

1ec5 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bsudekum commented Nov 21, 2017

Choose a reason for hiding this comment

1ec5 commented Nov 22, 2017

bsudekum commented Sep 28, 2017 •

edited

Loading