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

Make compatible with swift 4.1 #1271

Merged
merged 3 commits into from
Apr 4, 2018
Merged

Make compatible with swift 4.1 #1271

merged 3 commits into from
Apr 4, 2018

Conversation

bsudekum
Copy link
Contributor

@bsudekum bsudekum commented Mar 30, 2018

This gentle upgrades the project to xcode 9.3 and swift 4.1. In swift 4.1, flatMap was renamed to companctMap. To allow previous versions of swift to build, I added an extension that uses the appropriate function.

This also updates the project settings presented to me in xcode 9.3. Unsure about a few of them.

/cc @mapbox/navigation-ios

import Foundation

extension Sequence where Element: Hashable {
func compatibleFlatMap<ElementOfResult>(_ transform: (Element) throws -> ElementOfResult?) rethrows -> [ElementOfResult] {
Copy link
Contributor

@akitchen akitchen Mar 30, 2018

Choose a reason for hiding this comment

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

I'm not crazy about the name here -- is there another way we can define/extend compactMap to delegate to flatMap where compactMap doesn't exist?


extension Sequence where Element: Hashable {
func compatibleFlatMap<ElementOfResult>(_ transform: (Element) throws -> ElementOfResult?) rethrows -> [ElementOfResult] {
#if swift(>=4.1)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm also not crazy about using static version checks -- as an old ObjC'er, I prefer to check for API availability. Just my 2¢, but if we address my other comment we will be effectively doing that, so...

@frederoni
Copy link
Contributor

I was under the impression that Xcode 9.3 had a backward incompatible project structure 🤔 If that's the case, I think we should postpone this migration.

@akitchen
Copy link
Contributor

Perhaps there is a new format, but it doesn't appear that this PR migrates us to it.

In any event I don't think we should merge this as-is -- I would prefer a shim which doesn't introduce new API

@bsudekum
Copy link
Contributor Author

bsudekum commented Apr 2, 2018

@frederoni is right, from the release notes:

Projects created in Xcode 9.3 use a new project format that is incompatible with earlier versions of Xcode. To open projects in earlier versions, change the project format by selecting the project in the Project navigator, opening the Document inspector, and selecting the desired format from the Project Format pop-up menu. (35207662)

We should hold on any xcode proj updates and just focus on swift 4.1 build time warnings.

@bsudekum bsudekum changed the title Upgrade to xcode 9.3, swift 4.1 Make compatible with swift 4.1 Apr 2, 2018
@bsudekum
Copy link
Contributor Author

bsudekum commented Apr 2, 2018

Removed xcode proj 9.3 changes.

@bsudekum
Copy link
Contributor Author

bsudekum commented Apr 2, 2018

@akitchen I think I made this as least intrusive as possible in 612a7e0. Going forward, we'd always use compactMap, and it'd port backwards nicely in swift 4.0 without a new API. Let me know what you think.

Noting, I grabbed this from https://bugs.swift.org/browse/SR-6970

import Foundation

extension Sequence where Element: Hashable {
#if !swift(>=4.1)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Strangely, #if swift(<4.1) does not work.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, it's silly, but 🤷‍♂️

Copy link
Contributor

Choose a reason for hiding this comment

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

Should this file name have a capital S ?

@akitchen
Copy link
Contributor

akitchen commented Apr 4, 2018

This looks good to me. We can iterate from here if needed.

Copy link
Contributor

@akitchen akitchen left a comment

Choose a reason for hiding this comment

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

Let's rename that file with a capital S and we're good to go

@bsudekum bsudekum merged commit 9cde612 into master Apr 4, 2018
@bsudekum bsudekum deleted the compatibleFlatMap branch April 4, 2018 16:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants