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 #67

Merged
merged 2 commits into from
Dec 7, 2016
Merged

Migrate to Swift 3 #67

merged 2 commits into from
Dec 7, 2016

Conversation

DenTelezhkin
Copy link
Contributor

Hey!

Since we need this framework working for our app, we wanted to help with migration to Swift 3.

What's in this PR?

  • Migrated SkyFloatingLabelTextField framework and unit test target to Swift 3 syntax
  • Migrated example target to Swift 3
  • Verified that all unit tests pass
  • Verified that example app functions properly
  • Switched to Xcode 8 for running tests on Travis CI
  • Added .swift-version file to allow properly linting podspec when you are ready to release

I did not do any Swift API design guidelines-related changes, i thought that as library authors, you would probably want to make those choices yourselves. I would be happy to help with other issues of Swift 3 transition, if they arise.

Thanks for awesome library, and hope this PR will be helpful.

Copy link

@Al3x4nd3r Al3x4nd3r left a comment

Choose a reason for hiding this comment

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

I liked this pull too

@Tibb
Copy link

Tibb commented Sep 30, 2016

Hi @DenHeadless, could you consider putting your pull request on a public repo? Then we'll be able to integrate it with cocoapods and use it a temporary solution at least. Thanks a lot!

@DenTelezhkin
Copy link
Contributor Author

@Tibb What do you mean by public repo? It is public, we are using it in CocoaPods, like so:

pod 'SkyFloatingLabelTextField', git: "https://github.com/MLSDev/SkyFloatingLabelTextField.git", branch: "swift3"

@Tibb
Copy link

Tibb commented Sep 30, 2016

@DenHeadless Well, I might have made a mistake in the path in my podfile when I tried to use your branch... :/ Thanks for the tip, seems to work 👍

Copy link

@Tibb Tibb left a comment

Choose a reason for hiding this comment

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

Working great so far. Good job!

@DenTelezhkin
Copy link
Contributor Author

@gergelyorosz @intonarumori Guys, just pinging you in case you haven't seen this PR. I understand that's an open-source, and you don't owe us anything, but any hint on when this could be merged or not merged would be nice, since already 20 days have passed since PR was opened.

As i've said, we'll be happy to help if any other assistance is needed with Swift 3 migration.

@wolffan
Copy link
Contributor

wolffan commented Oct 10, 2016

@DenHeadless Hello! Thanks for this. I haven't had time to look at this.
My plan is though: migrate first the 2.3 PR -> #69

Then do a major version changes for this one.

Not sure I'll have time to do both today. But It's going to happen.
Thanks for your work and patience

@DenTelezhkin
Copy link
Contributor Author

@wolffan Thanks! No worries, take your time! 🍺

@chungbd
Copy link

chungbd commented Nov 8, 2016

I've just used this lib for my application, so I really so interested in updating code to Swift 3.0.

@wolffan you met an obstacle, didn't you? Let's know it, may everybody can help you. Thanks

@ctews
Copy link

ctews commented Nov 9, 2016

+1

@intonarumori
Copy link
Contributor

intonarumori commented Nov 9, 2016

+1000

@k0nserv
Copy link
Contributor

k0nserv commented Nov 29, 2016

Hey @DenHeadless can you rebase this on master?

@DenTelezhkin
Copy link
Contributor Author

@k0nserv Sure, rebased.

@DenTelezhkin
Copy link
Contributor Author

Travis failure seems completely unrelated to code changes in PR. Tests pass locally on my machine. iOS CI as always, incredibly stable =)

@k0nserv
Copy link
Contributor

k0nserv commented Nov 29, 2016

I'll run them locally too, restarted travis as well. Might be a fluke

@k0nserv
Copy link
Contributor

k0nserv commented Nov 29, 2016

Right tests, pass locally for me as well

@k0nserv
Copy link
Contributor

k0nserv commented Nov 29, 2016

@bogren might have fixed the travis issue in #76 if you wanna try and rebase again

@DenTelezhkin
Copy link
Contributor Author

Sure, let's give it a shot =) Rebased once more.

@k0nserv
Copy link
Contributor

k0nserv commented Nov 29, 2016

That did it, thanks @bogren 👍 Just gotta finish up the work on 2.3 then we can merge this

@bogren
Copy link
Contributor

bogren commented Nov 29, 2016

Awesome! 👏 No problem ☺️

@wolffan
Copy link
Contributor

wolffan commented Nov 30, 2016

Merged 2.3!
Can we resolve conflicts :) ?

@k0nserv k0nserv mentioned this pull request Nov 30, 2016
@DenTelezhkin
Copy link
Contributor Author

Rebased once more =)

@k0nserv
Copy link
Contributor

k0nserv commented Nov 30, 2016

Great thank you for all the help @DenHeadless

Copy link
Contributor

@k0nserv k0nserv left a comment

Choose a reason for hiding this comment

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

LGTM

@bogren
Copy link
Contributor

bogren commented Dec 3, 2016

Sorry, you have to rebase again 😇

@k0nserv
Copy link
Contributor

k0nserv commented Dec 3, 2016

We can always rebase ourselves, no need to do it

@DenTelezhkin
Copy link
Contributor Author

Yep, edits from maintainers is on =) Please just let me know when you are ready to merge, and i can rebase one final time =)

@k0nserv k0nserv merged commit 94766c0 into Skyscanner:master Dec 7, 2016
@k0nserv
Copy link
Contributor

k0nserv commented Dec 7, 2016

Massive thanks for this PR @DenHeadless :)

@DenTelezhkin
Copy link
Contributor Author

Thank you guys! This is great news! Are you planning 2.0 release for CocoaPods?

@k0nserv
Copy link
Contributor

k0nserv commented Dec 7, 2016

Yeah, I'm working on that at the moment

@k0nserv
Copy link
Contributor

k0nserv commented Dec 7, 2016

See #81

@DenTelezhkin DenTelezhkin deleted the swift3 branch December 7, 2016 11:44
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.

None yet

10 participants