revamped rule 2.5 + updated style guide for swift 3 #8

cezarywojcik · 2016-10-06T03:30:58Z

See changes in diff.

Rule 2.5 is a major difference; other changes are mainly updating code examples to fit with Swift 3.

Two other new rules: 3.2.5 and 3.2.6 about usage of fileprivate and open.

drumnkyle · 2016-10-06T05:47:07Z

README.md

@@ -682,24 +689,18 @@ doSomethingWithClosure() { response: NSURLResponse in
 [1, 2, 3].flatMap { String($0) }
 ```

-* **3.8.2** When listing parameters, if using a capture list and/or specifying a (non-void) return type, wrap the parameter list in parentheses. Otherwise, the parentheses can be avoided.
+* **3.8.2** When listing parameters in a closure, wrap the parameter list in parentheses.


Isn't this a compiler error now? If so, we don't need this rule.

True, removing.

drumnkyle · 2016-10-06T05:47:47Z

README.md

 ```

 * **3.8.3** If specifying a closure as a type, you don’t need to wrap it in parentheses unless it is required (e.g. if the type is optional or the closure is within another closure). Always wrap the arguments in the closure in a set of parentheses - use `()` to indicate no arguments and use `Void` to indicate that nothing is returned.

 ```swift
-let completionBlock: (success: Bool) -> Void = {
+let completionBlock: (Bool) -> Void = { success in


This doesn't follow the rule from above, correct?

It is. The rule is talking about wrapping the entire declaration in parentheses, e.g. ((Bool -> Void) instead of (Bool) -> Void.

Sorry I was talking about the parameter name before the in

Ah, yes, my bad. Fixing!

drumnkyle · 2016-10-06T05:48:38Z

README.md


 ```swift
-struct Error: ErrorType {
+struct Error: Swift.Error {


I feel like you should name this differently instead of Error. Use like MyError

I was struggling to come up with a good name for this, since Error works so well and anything else just seems extraneous. I wish Apple had kept this as ErrorType or ErrorProtocol =.

This works just fine if you just use Swift.Error here instead however, and it lets you keep the good naming. None of the alternatives that I've considered (ErrorWithMessage, DescriptiveError, ErrorContainer, Errrrroooorrrr, etc.) seem to be as clean as just Error.

This is especially useful when you want to namespace an error type within a class. For example:

class FileLoader { enum Error: Swift.Error { // ... } // V.S. enum FileLoaderError: Error { // ... } }

FileLoader.Error is a far better than FileLoader.FileLoaderError and fits more with the Swift API Guideline of avoiding needless words.

drumnkyle · 2016-10-06T05:49:40Z

README.md

@@ -395,10 +402,10 @@ let lastName = name.lastName

 * **3.1.5** Be wary of retain cycles when creating delegates/protocols for your classes; typically, these properties should be declared `weak`.

-* **3.1.6** Be careful when calling `self` directly from a closure as this can cause a retain cycle - use a [capture list](https://developer.apple.com/library/ios/documentation/swift/conceptual/Swift_Programming_Language/Closures.html#//apple_ref/doc/uid/TP40014097-CH11-XID_163) when this might be the case:
+* **3.1.6** Be careful when calling `self` directly from an escaping closure as this can cause a retain cycle - use a [capture list](https://developer.apple.com/library/ios/documentation/swift/conceptual/Swift_Programming_Language/Closures.html#//apple_ref/doc/uid/TP40014097-CH11-XID_163) when this might be the case:


I feel like we should probably add either another rule or something hear about preferring to always use non-escaping. And probably putting a link to something that explains what the differences are.

I agree that might be useful, but it will be out of scope for this PR.

Should we recommend not using [weak self] when it wouldn't be preventing a memory leak?

I think that instead of adding [weak self] everywhere just to be safe, it would be better to have people stop, pause, and think about the object graph and whether there would actually be a leak or not. By getting into the habit of thinking about cycles in object graphs, people would be more likely to notice leaks in other situations.

@staguer We can address this separately since your comment is about the current rule whether we're using Swift 3 or not. Out of scope for this PR.

Sounds good.

staguer · 2016-10-06T23:44:28Z

README.md


 // NOT PREFERRED
-static private let kMyPrivateNumber: Int
+static private let myPrivateNumber: Int
 ```

 * **3.2.2** The access modifier keyword should not be on a line by itself - keep it inline with what it is describing.


should we add something about @discardableResult and @testable being on the same or on their own lines?

Hm, I agree that a rule about annotations might be useful, but it's not something new for Swift 3. We can talk about it separately.

staguer · 2016-10-06T23:47:59Z

README.md

@@ -710,19 +707,20 @@ let completionBlock: () -> Void = {
 let completionBlock: (() -> Void)? = nil


don't need the = nil default value, right?

If it's a local constant, you do need it. It only gets auto-initialized to nil if it is a property.

drumnkyle

LGTM after

drumnkyle · 2016-10-07T00:07:52Z

README.md

@@ -514,6 +521,10 @@ class Pirate {
 let pirateName = "LeChuck"
 ```

+* **3.2.5** Prefer `private` to `fileprivate` where possible.
+
+* **3.2.6** When choosing between `public` and `open`, prefer `open` if you intend for something to be subclassable outside of a given module and `public` otherwise. Note that anything `internal` and above can be subclassed in tests by using `@testable import`, so this shouldn't be a reason to use `open`. In general, lean towards being a bit more liberal with using `open` when it comes to libraries, but a bit more conservative when it comes to a modules in a codebase such as an app where it is easy to change things in multiple modules simultaneously.


Remove "a" before modules.

d'oh, fixed, thanks!

staguer · 2016-10-07T00:11:34Z

LGTM

drumnkyle suggested changes Oct 6, 2016

View reviewed changes

cezarywojcik force-pushed the master branch 2 times, most recently from b42c17e to 6da003d Compare October 6, 2016 21:27

staguer reviewed Oct 6, 2016

View reviewed changes

cezarywojcik force-pushed the master branch from 6da003d to 6e53b63 Compare October 6, 2016 23:57

drumnkyle suggested changes Oct 7, 2016

View reviewed changes

revamped rule 2.5 + updated style guide for swift 3

61cca5c

cezarywojcik force-pushed the master branch from 6e53b63 to 61cca5c Compare October 7, 2016 00:09

cezarywojcik merged commit 1a99e39 into linkedin:master Oct 7, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

revamped rule 2.5 + updated style guide for swift 3 #8

revamped rule 2.5 + updated style guide for swift 3 #8

cezarywojcik commented Oct 6, 2016 •

edited

Loading

drumnkyle Oct 6, 2016

cezarywojcik Oct 6, 2016

drumnkyle Oct 6, 2016

cezarywojcik Oct 6, 2016

drumnkyle Oct 6, 2016

cezarywojcik Oct 6, 2016

drumnkyle Oct 6, 2016

cezarywojcik Oct 6, 2016 •

edited

Loading

drumnkyle Oct 6, 2016

cezarywojcik Oct 6, 2016

staguer Oct 6, 2016

cezarywojcik Oct 6, 2016

staguer Oct 7, 2016

staguer Oct 6, 2016

cezarywojcik Oct 6, 2016

staguer Oct 6, 2016

cezarywojcik Oct 6, 2016

drumnkyle left a comment

drumnkyle Oct 7, 2016

cezarywojcik Oct 7, 2016

staguer commented Oct 7, 2016

		@@ -710,19 +707,20 @@ let completionBlock: () -> Void = {
		let completionBlock: (() -> Void)? = nil

revamped rule 2.5 + updated style guide for swift 3 #8

revamped rule 2.5 + updated style guide for swift 3 #8

Conversation

cezarywojcik commented Oct 6, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cezarywojcik Oct 6, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drumnkyle left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

staguer commented Oct 7, 2016

cezarywojcik commented Oct 6, 2016 •

edited

Loading

cezarywojcik Oct 6, 2016 •

edited

Loading