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

[Release] Hero 1.4.0 with Swift 4.2 support #534

Merged
merged 12 commits into from
Oct 14, 2018
Merged

[Release] Hero 1.4.0 with Swift 4.2 support #534

merged 12 commits into from
Oct 14, 2018

Conversation

SD10
Copy link
Member

@SD10 SD10 commented Sep 16, 2018

Summary

  • Adds support for Swift 4.2
  • The example target for iOS does not compile because CollectionKit needs a Swift 4.2 update still

Related

Replaces #490

TODO ⏰

N/A

@SD10 SD10 self-assigned this Sep 16, 2018
@SD10 SD10 requested a review from lkzhao September 16, 2018 02:44
@SD10
Copy link
Member Author

SD10 commented Sep 16, 2018

cc @freak4pc @alexanderkhitev @andreipt @rennarda

Copy link
Contributor

@freak4pc freak4pc left a comment

Choose a reason for hiding this comment

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

Builds with no issues! Just a few minor notes :)

@@ -69,18 +69,18 @@ class HeroDebugView: UIView {
addSubview(backgroundView)

doneButton = UIButton(type: .system)
doneButton.setTitle("Done", for: .normal)
doneButton.setTitle("Done", for: UIControl.State.normal)
Copy link
Contributor

Choose a reason for hiding this comment

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

You can just do .normal, no need to be this explicit :)

doneButton.addTarget(self, action: #selector(onDone), for: .touchUpInside)
backgroundView.addSubview(doneButton)

perspectiveButton = UIButton(type: .system)
perspectiveButton.setTitle("3D View", for: .normal)
perspectiveButton.setTitle("3D View", for: UIControl.State.normal)
Copy link
Contributor

Choose a reason for hiding this comment

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

same

perspectiveButton.addTarget(self, action: #selector(onPerspective), for: .touchUpInside)
backgroundView.addSubview(perspectiveButton)

if showCurveButton {
arcCurveButton = UIButton(type: .system)
arcCurveButton!.setTitle("Show Arcs", for: .normal)
arcCurveButton!.setTitle("Show Arcs", for: UIControl.State.normal)
Copy link
Contributor

Choose a reason for hiding this comment

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

same

@@ -162,7 +162,6 @@ class HeroDebugView: UIView {
startLocation = pinchGR.location(in: nil)
startTranslation = translation
startScale = scale
fallthrough
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did you remove fallthrough here? I think this needs to fall to the case below

@@ -61,12 +61,12 @@ class HeroProgressRunner {
self.isReversed = reverse
self.duration = totalTime
displayLink = CADisplayLink(target: self, selector: #selector(displayUpdate(_:)))
displayLink!.add(to: RunLoop.main, forMode: RunLoopMode(rawValue: RunLoopMode.commonModes.rawValue))
displayLink!.add(to: RunLoop.main, forMode: RunLoop.Mode(rawValue: RunLoop.Mode.common.rawValue))
Copy link
Contributor

Choose a reason for hiding this comment

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

This can simply be

displayLink!.add(to: .main, forMode: .common)

}

func stop() {
displayLink?.isPaused = true
displayLink?.remove(from: RunLoop.main, forMode: RunLoopMode(rawValue: RunLoopMode.commonModes.rawValue))
displayLink?.remove(from: RunLoop.main, forMode: RunLoop.Mode(rawValue: RunLoop.Mode.common.rawValue))
Copy link
Contributor

Choose a reason for hiding this comment

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

same

@freak4pc
Copy link
Contributor

By the way just realized one more issue here - won't you be breaking support for 4.0 this way? Maybe we should add conditional build statements?

@SD10
Copy link
Member Author

SD10 commented Sep 17, 2018

Yeah, @freak4pc conditionals isn't a bad improvement 👍 Given that this library will move slow I'm almost tempted to make 1.3.1 the last Swift 4.0 release but I'll see what I can do

@freak4pc
Copy link
Contributor

Even so, migrations to Swift 4.2 will take some time, so it's great supporting the last 2 at least.

@SD10
Copy link
Member Author

SD10 commented Sep 18, 2018

Even so, migrations to Swift 4.2 will take some time, so it's great supporting the last 2 at least.

I agree 😢 We're still on Swift 4.0 for our app

But it will delay the release on my end. I guess people can use this branch until I can make the backwards compatability changes

@skydivedan
Copy link

Is backward compatibility a problem, though?

  • Version 1.3.x of Hero could be for Swift 4.0, 4.1 users.
  • Version 1.4.x could be for Swift 4.2 users.

Also, you could set s.swift_version = 4.2 in the Podspec file, as an extra hint for Cocoapods.

If there's a critical change that you think is needed for Swift 4 and Swift 4.2, you could rev 2 separate pods. Probably not ideal I guess, though. Just my $0.02 from an early adopter of Swift 4.2

@SD10
Copy link
Member Author

SD10 commented Sep 19, 2018

@skydivedan

Is backward compatibility a problem, though?

I'm not really sure how much of an issue backward compatibility is and how long we would want to support both versions if we did.

I'm personally fine using Hero 1.3.1 and I can't think of any major upcoming Hero releases that would take place before most people have migrated to Swift 4.2. Unless there are strong objections I'll probably just end up pushing this

If there's a critical change that you think is needed for Swift 4 and Swift 4.2, you could rev 2 separate pods. Probably not ideal I guess, though. Just my $0.02 from an early adopter of Swift 4.2

Yeah, probably not ideal 😅 But thanks for the input 👍

@CEBATOR
Copy link

CEBATOR commented Oct 2, 2018

@SD10 just FYI https://github.com/SoySauceLab/CollectionKit/releases/tag/2.2.0

@alexanderkhitev
Copy link

@SD10 any news?

@SD10
Copy link
Member Author

SD10 commented Oct 14, 2018

Sorry for the delay everyone, I had finally burned out 😓 I feel like I'm slowly recovering but that was rough for me.

I added backward compatibility for previous Swift versions and will release this now. Feel free to open an issue if I botched anything in the environment

@SD10 SD10 merged commit 6a08cc1 into master Oct 14, 2018
@SD10 SD10 deleted the swift-4.2 branch October 14, 2018 23:12
@metabren
Copy link

No problem at all @SD10 — never a nice thing to go through. Thank you for this, and all the best for your recovery. Hope it's a speedy one!

@freak4pc
Copy link
Contributor

freak4pc commented Oct 15, 2018

Hey @SD10,
Please take more time - I know how hard this can be very up close but the community is here to support you. Feel free to at-me in every PR on any organization you need help with.

Take care of yourself, the rest is meaningless ❤️

@JoeMatt JoeMatt mentioned this pull request Oct 7, 2019
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.

7 participants