Swift 3.0 support #192

Liquidsoul · 2016-08-09T14:11:50Z

Hi !

I have picked up the work of @mxcl to make OHHTTPStubs support swift 3.0.
The objective was to make the code works on every version of swift (2.2, 2.3, 3.0) to fix issue #189.
To achieve this, I made some tradeoffs.

Remove first parameter label in Swift 3.0

To avoid declaring the methods multiple times to support both 2.3 and 3.0 syntax, some warnings are generated when using 2.x compilers.
For example, so that we can still use isPath("myPath") in Swift 3.0, the declaration has been changed to func isPath(_ path: String) -> OHHTTPStubsTestBlock.
This generates the following warning on 2.x compilers:
Extraneous '_' in parameter: 'path' has no keyword argument name

Multiple `#if` branching to instantiate either `URLRequest` or `NSURLRequest` in tests

While we could do some tricks using extensions to support 3.0 methods and attributes in swift 2.x, this is not feasible for a whole struct/class like URLRequest/NSURLRequest. So, in the tests, every time we were instantiating a NSURLRequest object, the call is now replaced by the creation of a URLRequest value-type using #if branching.
We may want to factorize this code into a helper function in the test class though. But this may need to convert all NSURLRequest to NSMutableURLRequest to have only one method call.

Use `NSURL`'s `path` in `isPath` instead of `URL`'s `path`

The path property on URL does not behave as the one in NSURL. In URL, the path is not truncated at the first semicolon. This has the effect to make the actual tests fail. So to fix this, I used NSURL's path.

For this pull request, I used the swift version of Xcode 8 beta 4.

I hope this helps!

mxcl · 2016-08-09T16:38:40Z

Sorry about the wip commits. I didn't expect them to get used. Make sure to squash them if this PR is used!

Liquidsoul · 2016-08-09T18:44:20Z

Not a problem, I can squash them if you want with a more meaningful message 😄
It is weird the CI did not pass though, everything worked fine on my repo branch 🤔

AliSoftware · 2016-08-17T21:56:56Z

Thanks a lot for this PR @Liquidsoul !

Sometimes the CI fails for no reason (well… for timeout reasons and for Xcode CLI tools weirdness randomly returning error code 65, see #188). I just asked Travis to restart the build, let's hope and see.
[EDIT] Yay, that was it, tests now pass!

Also, could you please add an entry in the CHANGELOG crediting you and @mxcl for that change? Thanks!

AliSoftware · 2016-08-17T22:16:00Z

OHHTTPStubs/Sources/Swift/OHHTTPStubsSwift.swift

+    return { req in req.url?.path?.hasPrefix(path) ?? false }
+#else
+    return { req in req.url?.path?.hasPrefix(path) ?? false }
+#endif


Maybe I'm tired but I don't see the difference between the code for #if os(tvOS) and for #else?

Might be a bad merge. But this was there (with a difference I thought) because the legacy-swift setting for the tvOS target was YES (should be NO). I fixed this in my fork, so this #if os(tvOS) shouldn't be there and the legacy-swift setting should be checked.

I think 81017a6be3596ad16651bb5a522b7338fe63d984 fixes this but I forgot to remove this conditional 😊

AliSoftware · 2016-08-17T22:40:14Z

Additionally @mxcl @Liquidsoul As an effort to adhere to the Moya contribution guidelines*, I've added you as collaborator to OHHTTPStubs to thank you for your contributions, and to also make it easier for you to contribute to the project in the future.

You'll now be able to create branches directly on the project (instead of having to have your fork) and have the ability to triage and label issues & PRs too. Use those new rights to your liking — or don't use them at all, it's up to you.

* still haven't found the time to properly copy the Community.md files and adopt the guidelines fully properly, but that's the goal hopefully in a near future

Liquidsoul · 2016-08-18T06:12:10Z

Thanks @AliSoftware !
I've updated the CHANGELOG.

About this PR, I wonder if we should merge it as-is.
Here, we enable Swift 3.0, but I suppose that most people use OHHTTPStubs with Swift 2.x code.
So, merging this can break their code if they use the next release.
Maybe I'm missing some user-side settings that could make the lib works with Swift 2.x code without changing the SWIFT_VERSION here 🤔

My advice would be to set SWIFT_VERSION to 2.3 here and have a swift3.0 branch in the repository changing it to 3.0.
What's your thoughts about this?

ikesyo · 2016-08-18T09:19:24Z

Maybe I'm missing some user-side settings that could make the lib works with Swift 2.x code without changing the SWIFT_VERSION here 🤔

CocoaPods users can work around this thanks for the PRs: CocoaPods/CocoaPods#5540, CocoaPods/CocoaPods#5760

but Carthage users can't.

My advice would be to set SWIFT_VERSION to 2.3 here and have a swift3.0 branch in the repository changing it to 3.0.

👍 from me.

Liquidsoul · 2016-08-18T19:37:38Z

I've updated the PR to set the SWIFT_VERSION back to 2.3.
I let someone else review and merge this if everyone is ok with this 😊

mxcl · 2016-08-18T20:52:04Z

IMO people are more likely to use Swift 3.0 than 2.3. Anyone using Swift is used to having to update and will want to be using the latest versions of everything.

AliSoftware · 2016-08-19T12:38:38Z

@mxcl I honestly don't know. I mean, at work for client projects we're still on 2.3. Hell for some projects we're even still on 2.2. Because it would be really time-consuming to migrate your code to the new beta of each Swift 3 release every time a new beta is released, and because we didn't sell the time for that to the client and don't have time nor leisure to take the risk.

OTOH, on personal projects where there's less strict deadlines and where we want to use Swift 3 as soon as each new beta is released to be always on top, we might prefer Swift 3 over 2.3.

I think until Swift 3 / Xcode 8 is officially released we can keep 2.3 in the example project, and change that to 3 once Swift 3 is official (even if it should be quite soon now). Thoughts?

PS: @Liquidsoul I'm not sure I understand your last comment, since when I look at the Changed Files tab, I see SWIFT_VERSION=2.3 lines in the xcodeproj… being removed, not restored?

Liquidsoul · 2016-08-19T12:47:45Z

@AliSoftware The full diff can be misleading. This is because I've configured the Swift version at the project level and so removed the settings on the targets (see commit 9010c3a22636cbf8a7f904483001b76749d25f17).
So there is now only two lines that changes if we change the SWIFT_VERSION settings (see f83f1446e04e7b95fda3702cb4f37413ffb995f1).

All tests do not pass yet

SWIFT_VERSION is now configured only at the project level

We need to cast to NSURL because URL.path does not behave like NSURL.path in Swift 3.0. URL.path does not stop at the first ';' and returns the entire string.

AliSoftware · 2016-08-19T12:50:23Z

Got it!

Liquidsoul · 2016-08-19T12:52:04Z

I've fixed the conflicts, hope this will pass the CI on first try ! 😁

AliSoftware · 2016-08-19T13:38:36Z

We should probably add something in the README.md (+ note/link in CHANGELOG.md) about how to change the Swift Version, like:

Using the right Swift Version of OHHTTPStubs for your project

OHHTTPStubs support both Swift 2.2, 2.3 and Swift 3.0 🎉

For CocoaPods users, if you use CocoaPods version 1.1.0.beta.1, then CocoaPods will compile OHHTTPStubs with the right Swift Version matching the one you use for your project automatically. For more info, see

For Carthage Users, the framework on master is built using Swift 2.3. If you want Carthage to build the framework with Swift 3, you can use the swift3 branch (whose only difference with master is that the project's Build Settings set SWIFT_VERSION=3.0 instead of 2.3)

Problem is, if we do that, what about past September once Swift 3 is released, will we switch master to use SWIFT_VERSION=3.0 and remove branch swift3… and create a swift2.3 branch instead of legacy? Would probably be a pain for Carthage users to update all their URLs again in their Cartfile, right? While doing the opposite might work better for the long run?

Note: I'm not a Carthage user, so idk if there's a way to generate multiple pre-build framework for Carthage in GitHub Releases (one for 2.3 and one for 3.0) and let the users specify which pre-build version they want to download / depend on? Or will they be force to use --dont-use-binaries if they need Swift 3.0 so that Carthage rebuilds OHHTTPStubs from source instead of using the framework?

AliSoftware · 2016-08-19T14:04:16Z

CHANGELOG.md

@@ -8,6 +8,8 @@
  [@ikesyo](https://github.com/ikesyo), [#185](https://github.com/AliSoftware/OHHTTPStubs/pull/185)
 * Fix: Carthage support and Examples configurations
  [@Liquidsoul](https://github.com/Liquidsoul), [#190](https://github.com/AliSoftware/OHHTTPStubs/issues/190)
+* Added Swift 3.0 support.


Tip: please add two spaces after the full-stop, so that the markdown renders a line break and renders the next line as a separate line (otherwise it will join the lines) 😉

mxcl · 2016-08-19T16:39:12Z

@mxcl I honestly don't know. I mean, at work for client projects we're still on 2.3. Hell for some projects we're even still on 2.2. Because it would be really time-consuming to migrate your code to the new beta of each Swift 3 release every time a new beta is released, and because we didn't sell the time for that to the client and don't have time nor leisure to take the risk.

Fair point.

The ticket I've opened against Carthage may make our setting moot anyway.

It's a pity Apple didn't anticipate this sort of scenario and make the setting inheritable. Perhaps they did and we just don't understand how to use it.

Liquidsoul · 2016-08-19T20:39:12Z

Problem is, if we do that, what about past September once Swift 3 is released, will we switch master to use SWIFT_VERSION=3.0 and remove branch swift3… and create a swift2.3 branch instead of legacy? Would probably be a pain for Carthage users to update all their URLs again in their Cartfile, right? While doing the opposite might work better for the long run?

I do not think this is that bad for Carthage users. They would just have to update the dependency as for any version bump, but I may be wrong.

Maybe we should consider the standard user case, not the beta users? The ones using Xcode 7.x, with Swift 2.2 and who will migrate automatically to Xcode 8 in september.
They are not impacted by this setting because their version of Xcode does not care about it yet 😁 However, once they decide to move to Xcode 8 we should not force them to Swift 3.0 as soon as they update their IDE. This would mean that an unrelated change (the IDE version) will break the OHHTTPStubs dependency without changing the library version.
We should let them choose to update like Apple is doing with the migration tool. Meaning, leaving them the option to enable Swift 3.0 using a branch until we decide to merge it to master and make it the default value in a later release.

AliSoftware · 2016-08-19T20:45:48Z

@Liquidsoul Swift 2.2 and Swift 2.3 are not ABI-compatible afaik. This means that even in your example use case, after upgrading from Xcode 7 (and using Swift 2.2) to Xcode 8 (+ SWIFT_VERSION=2.3 in their project), they'll still have to rebuild their Carthage dependencies anyway. So they'll need to do some carthage update or something and thus use a different version of OHHTTPStubs (2.3 instead of 2.2) anyway, right?

Liquidsoul · 2016-08-19T21:14:24Z

They won't need to use a different version of OHHTTPStubs.
They need to do a carthage build in order to rebuild the frameworks. Something one needs to do for almost every Xcode update because of the ABI 😉
The rebuild will be using the current checkout version of the library. Meaning that, if we set SWIFT_VERSION=3.0, given a fully working Xcode 7/Swift 2.x project, the update to Xcode 8 will build OHHTTPStubs framework with swift 3.0 making it unusable for the now Xcode 8/Swift 2.x project.

AliSoftware · 2016-08-19T21:33:28Z

Fair enough. I'm convinced 😉

So here's what I propose, does it match your idea of the ideal solution you suggested above?

First stage
- merge that PR into master which will use SWIFT_VERSION=2.3
- make a branch swift-3.0 with only diff with master will be SWIFT_VERSION=3.0
- If we ever do some new commits on master, we could even rebase swift-3.0 on top of master in order to always keep SWIFT_VERSION to be the only difference between the two branches
Later, probably a couple of weeks after Xcode 8 GM is released or so
- merge branch swift-3.0 into master, making SWIFT_VERSION=3.0 the default
- Optionally, create branch swift-2.3 with only diff with master being SWIFT_VERSION=2.3, in order to still keep retro-compatibility around for some time (and rebase regularly as needed)
In the long run, we will probably drop the swift-2.3 branch when everybody will be using Swift 3

Does that feel right to you? (And if so, do you feel like documenting this on a user's perspective in the README.md?)

Liquidsoul · 2016-08-20T08:43:08Z

Seems perfect 👍
I'll add the information about a swift-3.0 branch in the README.md.

Liquidsoul · 2016-08-20T09:01:57Z

I suggest that I create another pull-request for the README.md update. Or do you prefer to have the change here?

AliSoftware · 2016-08-20T09:04:05Z

Both are fine by me. Seems more logical to me to have everything related in the same PR but it's up to you.

Liquidsoul · 2016-08-20T09:07:29Z

You're right, seems more logical to have it here.
I've just pushed the change here.

Liquidsoul · 2016-08-20T18:01:23Z

I've added something in the Carthage section about Xcode 7 😉

AliSoftware · 2016-08-21T00:30:37Z

🎉 time to merge this 👍

AliSoftware · 2016-08-21T03:31:37Z

5.2.0 is live now! 🎉
Tag 5.2.0 created; branch swift-3.0 + tag 5.2.0-swift3 created too.

Thanks again for the good work folks, nicely done! 👍 👌

This was referenced Aug 17, 2016

Swift 3 support #189

Closed

Support for Swift 3.0 projects #182

Closed

AliSoftware reviewed Aug 17, 2016
View reviewed changes

mxcl mentioned this pull request Aug 18, 2016

Xcode 8: Allowing for dependencies that support both Swift 2.3 and 3.0 Carthage/Carthage#1445

Closed

mxcl and others added 10 commits August 19, 2016 14:50

Set swift version used to 3.0

9220506

Update schemes, build options and use Swift 3 syntax

706ac3f

Add extensions to support Swift 3 syntax in Swift 2.x code

a31c5d4

Fix pathStartsWith for tvOS

10ad179

Make lib and tests compile with Xcode 8 beta 4

e5bb380

All tests do not pass yet

Remove target level SWIFT_VERSION setting

1ffda18

SWIFT_VERSION is now configured only at the project level

Workaround to fix isPath tests in Swift 3.0

9505490

We need to cast to NSURL because URL.path does not behave like NSURL.path in Swift 3.0. URL.path does not stop at the first ';' and returns the entire string.

Remove unneeded #if os(tvOS) conditional

1973182

Update changelog

6790e28

Use legacy Swift (2.3)

3475b96

AliSoftware reviewed Aug 19, 2016
View reviewed changes

Fix CHANGELOG formatting

c4a013a

Add swift 2.3/3.0 support details info in README.md

8442823

Liquidsoul added 2 commits August 20, 2016 19:57

Fix typo

9c24342

Add a note about SWIFT_VERSION and Xcode 7

f2f6956

AliSoftware merged commit e30dcdb into AliSoftware:master Aug 21, 2016

Liquidsoul mentioned this pull request Jan 24, 2017

Please set master to swift 3 :D #233

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Swift 3.0 support #192

Swift 3.0 support #192

Liquidsoul commented Aug 9, 2016 •

edited

Loading

mxcl commented Aug 9, 2016 •

edited

Loading

Liquidsoul commented Aug 9, 2016

AliSoftware commented Aug 17, 2016 •

edited

Loading

AliSoftware Aug 17, 2016

mxcl Aug 17, 2016 •

edited

Loading

Liquidsoul Aug 18, 2016

AliSoftware commented Aug 17, 2016

Liquidsoul commented Aug 18, 2016

ikesyo commented Aug 18, 2016

Liquidsoul commented Aug 18, 2016

mxcl commented Aug 18, 2016

AliSoftware commented Aug 19, 2016

Liquidsoul commented Aug 19, 2016

AliSoftware commented Aug 19, 2016

Liquidsoul commented Aug 19, 2016

AliSoftware commented Aug 19, 2016 •

edited

Loading

Using the right Swift Version of OHHTTPStubs for your project

AliSoftware Aug 19, 2016

mxcl commented Aug 19, 2016

Liquidsoul commented Aug 19, 2016

AliSoftware commented Aug 19, 2016

Liquidsoul commented Aug 19, 2016

AliSoftware commented Aug 19, 2016 •

edited

Loading

Liquidsoul commented Aug 20, 2016

Liquidsoul commented Aug 20, 2016

AliSoftware commented Aug 20, 2016

Liquidsoul commented Aug 20, 2016

Liquidsoul commented Aug 20, 2016

AliSoftware commented Aug 21, 2016

AliSoftware commented Aug 21, 2016

Swift 3.0 support #192

Swift 3.0 support #192

Conversation

Liquidsoul commented Aug 9, 2016 • edited Loading

Remove first parameter label in Swift 3.0

Multiple #if branching to instantiate either URLRequest or NSURLRequest in tests

Use NSURL's path in isPath instead of URL's path

mxcl commented Aug 9, 2016 • edited Loading

Liquidsoul commented Aug 9, 2016

AliSoftware commented Aug 17, 2016 • edited Loading

AliSoftware Aug 17, 2016

Choose a reason for hiding this comment

mxcl Aug 17, 2016 • edited Loading

Choose a reason for hiding this comment

Liquidsoul Aug 18, 2016

Choose a reason for hiding this comment

AliSoftware commented Aug 17, 2016

Liquidsoul commented Aug 18, 2016

ikesyo commented Aug 18, 2016

Liquidsoul commented Aug 18, 2016

mxcl commented Aug 18, 2016

AliSoftware commented Aug 19, 2016

Liquidsoul commented Aug 19, 2016

AliSoftware commented Aug 19, 2016

Liquidsoul commented Aug 19, 2016

AliSoftware commented Aug 19, 2016 • edited Loading

Using the right Swift Version of OHHTTPStubs for your project

AliSoftware Aug 19, 2016

Choose a reason for hiding this comment

mxcl commented Aug 19, 2016

Liquidsoul commented Aug 19, 2016

AliSoftware commented Aug 19, 2016

Liquidsoul commented Aug 19, 2016

AliSoftware commented Aug 19, 2016 • edited Loading

Liquidsoul commented Aug 20, 2016

Liquidsoul commented Aug 20, 2016

AliSoftware commented Aug 20, 2016

Liquidsoul commented Aug 20, 2016

Liquidsoul commented Aug 20, 2016

AliSoftware commented Aug 21, 2016

AliSoftware commented Aug 21, 2016

Liquidsoul commented Aug 9, 2016 •

edited

Loading

Multiple `#if` branching to instantiate either `URLRequest` or `NSURLRequest` in tests

Use `NSURL`'s `path` in `isPath` instead of `URL`'s `path`

mxcl commented Aug 9, 2016 •

edited

Loading

AliSoftware commented Aug 17, 2016 •

edited

Loading

mxcl Aug 17, 2016 •

edited

Loading

AliSoftware commented Aug 19, 2016 •

edited

Loading

AliSoftware commented Aug 19, 2016 •

edited

Loading