Skip to content
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 3 #64

Merged
merged 44 commits into from
Feb 20, 2017
Merged

Migrate to Swift 3 #64

merged 44 commits into from
Feb 20, 2017

Conversation

aataraxiaa
Copy link
Contributor

@aataraxiaa aataraxiaa commented Aug 3, 2016

What changed

  • Update directions target SWIFT_VERSION and test targets
  • Run Swift 3 migration on directions framework
  • Convert example to swift 3

Todos

  • Integrate with a Polyline swift3 branch
  • Swiftify the API
  • Document

Notify

@1ec5

Note

  • This of course won't build without a swift3 version of Polyline

@aataraxiaa
Copy link
Contributor Author

@1ec5, I have forked Polyline (no access to source), and have a swift 3 branch up which seems to work as expected with MapboxDirections

@@ -33,21 +33,21 @@ public enum RouteShapeFormat: UInt, CustomStringConvertible {

This standard format is human-readable and can be parsed straightforwardly, but it is far more verbose than `Polyline`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lowercase polyline.

@1ec5
Copy link
Contributor

1ec5 commented Aug 9, 2016

Note that once raphaelmor/Polyline upgrades to Swift 3, you’ll need to update the podspec to require a newer version of that library.

@aataraxiaa
Copy link
Contributor Author

aataraxiaa commented Aug 10, 2016

Swift 3 has changed to using lower case enums - see here -> https://www.hackingwithswift.com/swift3. Also, I asked for write access to Polyline so received no response. So I forked Polyline and pointed the podspec at the migrated fork. Once aphaelmor/Polyline is migrated, I can point at the original repo

@1ec5
Copy link
Contributor

1ec5 commented Aug 10, 2016

The podspec only covers the example application in this repository. The podfile still points to the official repository, and to my knowledge, there’s no way to automatically point it elsewhere. Every developer integrating this library will need to add your Polyline fork as a dependency ahead of any pod that uses Polyline. We’ll need a blurb in the readme to that effect. (For example, this library used to depend on my fork of RequestKit.)

@aataraxiaa
Copy link
Contributor Author

@1ec5 Apologies, you are correct. I did the same with my app. I added my Polyline fork as a dependency ahead of the original Polyline. I'll add the blurb to the readme

@aataraxiaa
Copy link
Contributor Author

@1ec5 I updated the readme to include the forked Polyline. However, I am unsure about how to handle Carthage. Any ideas?

@1ec5
Copy link
Contributor

1ec5 commented Aug 10, 2016

I think you’d add your branch in place of the existing Polyline entry:

github "superpeteblaze/Polyline" "swift3"

@aataraxiaa
Copy link
Contributor Author

@1ec5 Updated with the latest Beta 6 Swift language changes

let request = NSMutableURLRequest(url: url)
request.setValue(userAgent, forHTTPHeaderField: "User-Agent")
return URLSession.shared.dataTask(with: request as URLRequest) { (data, response, error) in
var json: JSONDictionary = [:]
if let data = data {
do {
do {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: an extra space snuck in.

Update readme, it was referencing V0.6 of MapboxDirections
@aataraxiaa
Copy link
Contributor Author

@1ec5 Ran Xcode GM Swift migrator

@1ec5 1ec5 mentioned this pull request Dec 9, 2016
@simonseyer
Copy link
Contributor

I have no idea why one of the tvOS builds now fails:

❌ /Users/vagrant/git/MapboxDirections/MBRouteLeg.swift:1:8: no such module 'Polyline'
import Polyline

@1ec5
Copy link
Contributor

1ec5 commented Dec 9, 2016

Ugh, that’s the same error that’s been failing the watchOS build bot.

@1ec5
Copy link
Contributor

1ec5 commented Feb 20, 2017

221e451 fixes the tvOS and watchOS builds by replacing the superpeteblaze/Polyline dependency with raphaelmor/Polyline@f86f34d, which includes raphaelmor/Polyline#38. (Polyline’s master branch is currently broken: raphaelmor/Polyline#40.)

@1ec5 1ec5 changed the title [WIP] Swift3 Migration Migrate to Swift 3 Feb 20, 2017
Copy link
Contributor

@1ec5 1ec5 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The latest commit addresses the remaining suggested changes. v0.7.0 just went out the door as the last Swift 2.3 release, so we’re ready to land this PR.

Thanks for all your help, @superpeteblaze @Eldorado234!

Corrected some syntax errors in the examples. Point Podfile and Cartfile to master for now, until the first Swift 3 release. Updated links to the macOS SDK.
@1ec5 1ec5 merged commit 75dd09c into master Feb 20, 2017
@1ec5
Copy link
Contributor

1ec5 commented Feb 20, 2017

This PR’s swift3 branch was so long-lived that, out of expediency, we suggested that developers pin their Podfile or Cartfile directly to the branch (as opposed to a commit on the branch). Now that the swift3 branch has been merged to master, 5384a42 logs a warning to the console urging the developer to move to the master branch. The swift3 branch is no longer being maintained and will be deleted soon. If you pinned to a specific commit on this branch, that commit will continue to exist because it was merged into master. Otherwise, we urge you to heed the warning and move to master, and thence to the upcoming release once it’s published.

@1ec5 1ec5 added the backwards incompatible changes that break backwards compatibility of public API label Dec 18, 2018
@1ec5 1ec5 added this to the v0.8.0 milestone Dec 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backwards incompatible changes that break backwards compatibility of public API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants