Skip to content

Update for Xcode 9.3/Swift 4.1#244

Merged
pcantrell merged 2 commits intobustoutsolutions:swift-4.1from
joaomvfsantos:master
Jun 3, 2018
Merged

Update for Xcode 9.3/Swift 4.1#244
pcantrell merged 2 commits intobustoutsolutions:swift-4.1from
joaomvfsantos:master

Conversation

@joaomvfsantos
Copy link
Copy Markdown

@joaomvfsantos joaomvfsantos commented Apr 3, 2018

Basically I just did 2 things:

  • Automatic Xcode upgrade of the project
  • Replaced flatMap with compactMap where said map returned optionals

Note: Travis Build shall fail until it's updated to use Xcode 9.3 image

@edwardmp
Copy link
Copy Markdown

edwardmp commented Apr 6, 2018

Was going to introduce a PR for the same.
But the tests are failing because compactMap is only implemented in Swift 4.1+.

Hence, the tests fail on Swift 4.0 and earlier. The solution should be to conditionally use compactMap/flatMap based on the Swift compiler version used.

@joaomvfsantos
Copy link
Copy Markdown
Author

@edwardmp Hey! Yes that makes sense, I could do those changes. How would you suggest doing this? Using a pre processor macro like this:

#if swift(>=4.1)
// .compactMap
#else
// .flatMap
#endif

I confess I'm not used to use pre processor macros, as such my only concern is that I would probably duplicate a lot of code (at least the whole block inside the map function).

@pcantrell
Copy link
Copy Markdown
Member

My usual approach with Swift language updates has been to fold them into the next minor version, and require a higher Swift version for that release while back-porting any critical fixes to a new patch release on the previous minor version. In other words, Siesta 1.4 will require Swift 4.1, and 1.3.x will maintain 4.0 support.

So no need for conditional compilation or 4.0 backward compatibility; I'll just keep this PR handy until I'm ready to start putting together Siesta 1.4.

@edwardmp
Copy link
Copy Markdown

@pcantrell I would normally agree, but since flatMap is used in just 5 places it might make sense to do this the "easy" way. Although I do have to say that a lot of my dependencies cause warnings for the same issue, so I guess I'll be stuck with those warnings for a while!

@Linnk
Copy link
Copy Markdown

Linnk commented Apr 17, 2018

@joaomvfsantos Hi. In order to avoid code duplication as you said, you can store the closure in a variable. This way you only need one line inside the conditional cases.

For Example:

let stringNumbers = ["1", "2", "3", "x", "4", "5"]
let closure: (String) -> Int? = { Int($0) }

#if swift(>=4.1)
    let numbers = stringNumbers.compactMap(closure)
#else
    let numbers = stringNumbers.flatMap(closure)
#endif

print(numbers)

Edit: I mean, in this example the closure is just one line too, but it could be a bigger block of code with multiple lines.

PS. I was about to make a PR about these annoying warnings lol, good think I checked first.

@edwardmp
Copy link
Copy Markdown

@Linnk
Good idea! Can you submit a PR implementing this, at least, if @pcantrell is willing to settle for this?

@Linnk
Copy link
Copy Markdown

Linnk commented Apr 17, 2018

@edwardmp Well, I was doing it… but then I read the comment about the siesta and swift versions and stuff. And I get it, I'm not fan either of those #if conditions.

@joaomvfsantos
Copy link
Copy Markdown
Author

@Linnk Thanks for taking the time to give a concrete example. I started to doing an approach like that but after @pcantrell comment I did not proceed. His view also makes sense to me.

@pcantrell
Copy link
Copy Markdown
Member

If we were going to simultaneously support Swift 4.0 and 4.1, I’d prefer to do it with a conditional extension, which can live in its own separate file and doesn’t clutter up usage sites:

#if !swift(>=4.1)
    extension Sequence
        {
        func compactMap<ElementOfResult>(_ transform: (Self.Element) throws -> ElementOfResult?) rethrows -> [ElementOfResult]
            { return try flatMap(transform) }
        }
#endif

However, as I said above, I don’t see any particular reason not to just require 4.1 in the next Siesta release. If somebody does find a compelling reason to keep 4.0 support, we could add the above.

For now, if you want to get this PR passing CI, try updating the .travis.yml to require the newer Xcode.

@pcantrell
Copy link
Copy Markdown
Member

Also P.S. I expect to be pulling together Siesta 1.4 in early May. There are some awesome new caching and custom request building features in the works, mostly complete; just have to survive the end of the semester first! If anybody wants a sneak preview, all that work is on the file-cache branch.

@joaomvfsantos
Copy link
Copy Markdown
Author

@pcantrell I changed travis to use the Xcode 9.3 image but it's failing only on iPhone 4s simulator. Any ideas why?

@pcantrell pcantrell changed the base branch from master to swift-4.1 June 3, 2018 15:48
@pcantrell pcantrell merged commit d70e19d into bustoutsolutions:swift-4.1 Jun 3, 2018
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