Update to Swift 4, Xcode 9.2 by hannesoid · Pull Request #126 · br1sk/brisk

hannesoid · 2018-01-15T10:09:33Z

What this contains

updates project to Swift 4 and update project settings & warnings to Xcode 9.2
changes OpenRadar parsing to use KeyPaths and not blocks
get Attachement dropping to work (still a bit confused though)
Touched storyboard to make Xcode happy

Tests

I ran the unit tests successfully locally
I ran the application and clicked around
I didn't test any actual submissions or dupe-creating, maybe somebody could do this?

keith

Did a first pass, this looks great! I’ll look at the key path stuff soon

keith · 2018-01-15T12:20:20Z

Brisk/AppDelegate.swift


    private func setupStatusItem() {
-        let image = NSImage(named: "StatusItemIcon")!
+        let image = NSImage(named: NSImage.Name(rawValue: "StatusItemIcon"))!


Haha, super weird this isn’t the same as UIKit

indeed! 😜

could use an image literal, but I think it's not supported on older macs

the docs say this type is 10.13+, but I assume that's not actually true

keith · 2018-01-15T12:20:55Z

Brisk/Extensions/NSStatusItem+Extension.swift

 extension NSStatusItem {
    static func create(image: NSImage, menu: NSMenu) -> NSStatusItem {
-        let statusItem = NSStatusBar.system().statusItem(withLength: NSVariableStatusItemLength)
+        let statusItem = NSStatusBar.system.statusItem(withLength: NSStatusItem.variableLength)


Can you omit the enum or is this just a constant?

.variableLength seems to be a CGFloat, so NSStatusItem can't be omitted

keith · 2018-01-15T12:28:38Z

Brisk/Extensions/NSStoryboard+Extension.swift

 import AppKit

-private let kMainStoryboard = NSStoryboard(name: "Main", bundle: nil)
+private let kMainStoryboard = NSStoryboard(name: NSStoryboard.Name(rawValue: "Main"), bundle: nil)


So there’s no string one for this either?

I couldn't find anyhing simpler…

You can change NSStoryboard.Name to .init and that'll also solve the line length here

keith · 2018-01-15T12:28:59Z

Brisk/Extensions/NSTextView+Extension.swift

 extension NSTextView {
    var stringValue: String {
-        return self.string ?? ""
+        return self.string


Nice. I wonder if we can remove this property now

keith · 2018-01-15T12:29:14Z

Brisk/Extensions/NSTextView+Extension.swift

 extension NSTextView: Validatable {
    var isValid: Bool {
-        return self.string?.isEmpty == false
+        return self.string.isEmpty == false


We can use ! Here instead of == false now

hannesoid · 2018-01-15T14:43:18Z

Thanks @keith! I did the little string things

keith

few minor comments

keith · 2018-01-15T22:09:06Z

Brisk/Models/OpenRadar.swift

    public var steps: String?
    public var version: String?
+    fileprivate var appendingToActual: String? {
+        get { return actual }


self.actual

keith · 2018-01-15T22:09:20Z

Brisk/Models/OpenRadar.swift

    public var version: String?
+    fileprivate var appendingToActual: String? {
+        get { return actual }
+        set { actual = appendOrReturn(actual, newValue) }


self.actual twice

keith · 2018-01-15T22:13:03Z

Brisk/Views/AttachmentDroppableView.swift

-    private var files: [String] {
-        return self.draggingPasteboard().propertyList(forType: NSFilenamesPboardType) as? [String] ?? []
+    private var files: [URL] {
+        let asStrings = self.draggingPasteboard().propertyList(forType: makeFileNameType()) as? [String] ?? []


You can replace makeFileNameType inline with .init(rawValue: "foo") for this case IMO

I left the makeFileNameType() in place as it is used twice and gives room for the explainatory comment. Maybe in the future somebody with more thorough macOS SDK knowledge than me could find a way to use the enum and not stringy version.

keith · 2018-01-15T22:15:18Z

I think the build is failing here because of SWIFT_VERSION + Pods, you can apply this patch:

diff --git i/Podfile w/Podfile
index 66fb2ac..9066034 100644
--- i/Podfile
+++ w/Podfile
@@ -13,4 +13,10 @@ end
 
 post_install do |installer|
   installer.pods_project.root_object.attributes["LastUpgradeCheck"] = "9999"
+
+  installer.pods_project.targets.each do |target|
+    target.build_configurations.each do |config|
+      config.build_settings["SWIFT_VERSION"] = "4.0"
+    end
+  end
 end

And it should fix that

keith · 2018-01-15T22:16:44Z

Lint is failing here too, I think there's a false positive because I haven't updated it in a while, trying it locally. There's also some long lines, currently we require a max of 110 columns, see the log for where

keith · 2018-01-15T22:20:54Z

Brisk/Models/OpenRadar.swift

+    "observed results": \.appendingToActual,
+    "steps to reproduce": \.steps,
+    "summary": \.description,
+    "version": \.version


please add a trailing comma so the diff if we add another field will be smaller

keith · 2018-01-15T22:22:27Z

Also please add a changelog entry here under enhancements

keith · 2018-01-15T22:29:27Z

I don't see that lint failure locally with either version, I'll land #130 and you can rebase and hopefully it'll pass

hannesoid · 2018-01-16T09:32:22Z

I rebased, then did the other things, starting d4d28ae.

keith · 2018-01-16T17:04:10Z

CHANGELOG.md

 ## Enhancements

- None.
+- Update project to Swift 4.0 and Xcode 9.2


Please include the URL here like below

oh sure, I overlooked that

- Swift seems just a nicer named alias for objective-c

hannesoid · 2018-01-18T08:18:23Z

Travis was still set to xcode8.3, which doesn't support swift 4…

keith · 2018-01-18T17:45:23Z

Ah can you update it to 9.2 in this PR?

…

-- Keith Smiley

On Jan 18, 2018, at 00:18, Hannes Oud ***@***.***> wrote: Travis was still set to xcode8.3, which doesn't support swift 4… — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

hannesoid · 2018-01-19T15:42:34Z

Yep, did that.
Now the reason it is still failing on ci seems that Sonar is attempted to be compiled with Swift 4 instead of 3.

keith · 2018-01-19T16:29:22Z

Ah the problem now is we need to upgrade Sonar once I merge this PR br1sk/Sonar#33

I'm just waiting on CI

keith · 2018-01-19T23:32:24Z

That's merged so you can bundle exec pod update Sonar

hannesoid · 2018-01-23T13:14:59Z

Yay, the checks have passed, @keith I think we could merge this now?

keith · 2018-01-23T17:51:47Z

Thanks for doing this!

hannesoid · 2018-01-23T18:10:24Z

🎉

keith reviewed Jan 15, 2018

View reviewed changes

keith approved these changes Jan 15, 2018

View reviewed changes

keith reviewed Jan 15, 2018

View reviewed changes

hannesoid added 16 commits January 16, 2018 10:16

Run swift 4 migrator

3d69b87

Fix compilation of AttachmentDroppableView

57b95ac

Update project file for Xcode 9.2

ac0e0d8

Update usage of string characters for swift 4

5659716

Update NSTextView .string usage for current SDK to non-optional

78d4047

Update usage of NSOffState NSOnState to .off and .on

119bfdf

Use updated modal response return types

239f004

Update OpenRadar parsing to use KeyPaths

1b10445

Touch Main.storyboard with Xcode 9.2

d26b07a

Fix drag & drop of attachement

9a77fff

Remove SWIFT_SWIFT3_OBJC_INFERENCE

d4d28ae

Simplify NSTextField String handling

ba7a932

Add trailing comma and some self.

5f59086

Use .init shorthands for assets

7ebb763

Apply Podfile patch for swift version

bf9da2e

Add Enhancement to Changelog

17d25c6

hannesoid force-pushed the swift4 branch from 12a9902 to 17d25c6 Compare January 16, 2018 09:28

keith reviewed Jan 16, 2018

View reviewed changes

hannesoid added 3 commits January 18, 2018 09:07

Add PR link to Changelog

9b00fdb

Update .travis.yml to use xcode9.2 and swift

1814431

- Swift seems just a nicer named alias for objective-c

Shorten lines to satisfy swiftlint

374ec31

Update sonar by running bundle exec pod update Sonar

e413925

keith merged commit 096c995 into br1sk:master Jan 23, 2018

hannesoid deleted the swift4 branch January 23, 2018 18:10

Conversation

hannesoid commented Jan 15, 2018

What this contains

Tests

Uh oh!

keith left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hannesoid commented Jan 15, 2018

Uh oh!

keith left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

keith commented Jan 15, 2018

Uh oh!

keith commented Jan 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

keith commented Jan 15, 2018

Uh oh!

keith commented Jan 15, 2018

Uh oh!

hannesoid commented Jan 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hannesoid commented Jan 18, 2018

Uh oh!

keith commented Jan 18, 2018 via email

Uh oh!

hannesoid commented Jan 19, 2018

Uh oh!

keith commented Jan 19, 2018

Uh oh!

keith commented Jan 19, 2018

Uh oh!

hannesoid commented Jan 23, 2018

Uh oh!

keith commented Jan 23, 2018

Uh oh!

hannesoid commented Jan 23, 2018

keith commented Jan 15, 2018 •

edited

Loading

hannesoid commented Jan 16, 2018 •

edited

Loading