Swiftlint #128

freak4pc · 2017-12-12T13:17:15Z

This PR:

Adds Swiftlint with a custom configuration
Cleans up entire project to match Swiftlint rules
Adds Swiftlint step to Travis

freak4pc · 2017-12-12T13:36:04Z

Source/RxSwift/cascade.swift

-			observer.onCompleted()
-			return Disposables.create()
-		}
+	public static func cascade<S: Sequence>(_ observables: S) -> Observable<T> where S.Iterator.Element == Element {


The big changes here is just spacing.
This seems to be the only file to use Tabs instead of Spaces, so changing one portion of the code made everything look strange, so I just re-indented it.

The old tabs-vs-spaces war eh? Always been a tabs guy, never understood the purpose of spaces, as:

some developers like to have 2, 3, 4 or 8 spaces indentation. Using a tab makes it flexible

using tabs makes smaller files (yeah I know in this day and age, not important)
:)

Well, it's not a war or anything. I just think people mostly use spaces, because it ensures the files look the same wherever you go.

I don't mind going one way or the other but then we need to see if we go the same everywhere. Otherwise, if you use tabs and someone who uses spaces edits your files, it will look very strange (like happened for me).

Good either way :)

I remember that editors like Emacs and VIM support a comment indicator that tells whether a file uses spaces or tabs, and adjust accordingly. AppCode and all IntelliJ-derived IDEs have a setting to automatically detect what's being use. Is there anything we could do for Xcode ?

Pray? :( 🙏

I also think the default setting is spaces so that could cause even more issues potentially.

I'm trying to think if there's any good way to support either method but I don't have enough experience here TBH

I'm trying to think if there's any good way to support either method

I think this defeats the purpose of avoiding such major diffs out of simple re-indent, correct?

When I'm saying "a way to support either" - i mean without messing up the code on each commit 😛

freak4pc · 2017-12-12T13:36:20Z

Tests/RxSwift/nwiseTests.swift

@@ -12,6 +12,7 @@ import RxSwift
 import RxSwiftExt
 import RxTest

+// swiftlint:disable:next type_name


I made this an exception since, even though you usually want Capitalized class named, in this case nwise would read NwiseTests, and It seemed odd. Any opinions welcomed :)

NWise might look even more odd, but I'd prefer sticking to this rule in particular :)

You mean NwiseTests or nwiseTests ?

NwiseTests, but would suggest a second opinion as well :)

mosamer · 2017-12-12T13:55:01Z

Tests/RxSwift/pausableBufferedTests.swift

        let underlying = scheduler.createHotObservable([
            next(150, 1),
            next(210, 2),
            next(230, 3),
            next(301, 4),
            next(350, 5),
            next(399, 6),
-            completed(500),
+            completed(500)


why do we drop trailing commas? I believe it's configurable from SwiftLint?

Doesn't make sense have a trailing comma for an array ... It's like a sentence ending with a comma instead of a dot, don't you think ?

Also, it's not across all tests which is even weirder. I personally highly prefer (and seems like its the standard) to have commas between elements, but not with a trailing one

Its main purpose is cleaner diffs in the future. I personally prefer them (and I don't believe there is a way to say they are not standard :D) but it's arguable of course ¯_(ツ)_/¯

I don't think it makes much difference in diffs
And also - in the codebase itself (not tests), there are no trailing commas ... so how can we reason about trailing commas specifically in tests? What makes this special ?

It does, no doubt :) so with a trailing comma it would look like;

next(50, "alpha"), + completed(50),

while without it will be

- next(50, "alpha") + next(50, "alpha"), + completed(50),

I'd say swiftlint is here to make it consistent across both codebase and tests. Again, this is a style preference so should be fine with any of them.

Not really. Sometimes I'd leave trailing commas (i.e. if I'm in a rush) or while in the process of working on array items that I move around, removing the last trailing comma means more work to re-add it if I move it so ... well that's expert-level nitpicking.

The argument that I like is @mosamer 's reasoning about cleaner diffs. Looks like a win!

Yup, Let me put those back in place :)

@fpillet @mosamer should we add it in places where it didn't exist? there are some places there aren't any commas.

Even tho TBH these tests aren't supposed to change that much so not sure on the diff value 🤔 anyways, do it everywhere?

Don't think so. Can't we just allow them?

Oh yeah, we can definitely allow :)
I just thought you want to retro-actively change. I'll just fix the rule. Thanks !

mosamer · 2017-12-12T13:56:42Z

Tests/RxSwift/retryWithBehaviorTests.swift

-        
-        // provide simple predicate that always return true
+
+        // provide simple predicate that always returns true


As other comments were modified to capitalize first word, shouldn't we modify this as well for sake of consistency.

Yup, for sure - I just did it as I go, it wasn't a purpose of this PR :)
We can keep cleaning as part of this PR or as a separate one TBH.

mosamer · 2017-12-12T13:59:43Z

.travis.yml

 before_script:
+  - swiftlint


I'm not sure if this will have any effect for warning violations rules. What about removing in favor of Danger integration once implemented?

Yeah, that was the plan indeed :)
I just wanted something until we have Danger working and didn't want to have everything in a single PR.

mosamer · 2017-12-12T14:14:54Z

Given the context of #127, I may suggest this PR -along with future ones- to be pointed to another branch than master.

freak4pc · 2017-12-12T14:16:23Z

@mosamer This specific PR doesn't create any breaking changes so I don't think there is any reason to point it to a different branch. Do you mean like a develop branch? Since we don't have one right now.

The parts of the list that are code-breaking would definitely need to be on a separate branch, but this specifically (like Danger) isn't really a code-breaking change (or a code change, at all 😋 )

mosamer · 2017-12-12T14:23:48Z

I don't think any of the items on the list is going to introduce a code breaking change. However, the overall context is a major change I'd say. That's why I tend to prefer having that on a separate branch (as a staging environment) before fully committing to it. That said, I totally acknowledge that this specific PR is needed anyway in master so again feel free to move forward on this (unless somebody second my motion 😀 )

freak4pc · 2017-12-12T14:24:40Z

@mosamer I think specifically for non-code changes (e.g. this and Danger), master is fine.

For the other code-related changed I'm going to make a separate branch linked to a milestone, probably.

Sounds good to you ?

freak4pc · 2017-12-12T15:17:02Z

$ swiftlint
Loading configuration from '.swiftlint.yml'
Linting Swift files in current working directory
Linting 'distinct+RxCocoa.swift' (1/51)
Linting 'mapTo+RxCocoa.swift' (2/51)
Linting 'not+RxCocoa.swift' (3/51)
Linting 'and.swift' (4/51)
Linting 'apply.swift' (5/51)
Linting 'cascade.swift' (6/51)
Linting 'catchErrorJustComplete.swift' (7/51)
Linting 'distinct.swift' (8/51)
Linting 'filterMap.swift' (9/51)
Linting 'flatMapSync.swift' (10/51)
Linting 'fromAsync.swift' (11/51)
Linting 'ignore.swift' (12/51)
Linting 'ignoreErrors.swift' (13/51)
Linting 'ignoreWhen.swift' (14/51)
Linting 'mapTo.swift' (15/51)
Linting 'materialized+elements.swift' (16/51)
Linting 'not.swift' (17/51)
Linting 'nwise.swift' (18/51)
Linting 'ObservableType+Weak.swift' (19/51)
Linting 'once.swift' (20/51)
Linting 'pausable.swift' (21/51)
Linting 'pausableBuffered.swift' (22/51)
Linting 'repeatWithBehavior.swift' (23/51)
Linting 'retryWithBehavior.swift' (24/51)
Linting 'unwrap.swift' (25/51)
Linting 'curry.swift' (26/51)
Linting 'distinct+RxCocoa.swift' (27/51)
Linting 'mapToTests+RxCocoa.swift' (28/51)
Linting 'notTests+RxCocoa.swift' (29/51)
Linting 'andTests.swift' (30/51)
Linting 'applyTests.swift' (31/51)
Linting 'cascadeTests.swift' (32/51)
Linting 'catchErrorJustCompleteTests.swift' (33/51)
Linting 'DistinctTests.swift' (34/51)
Linting 'filterMapTests.swift' (35/51)
Linting 'fromAsyncTests.swift' (36/51)
Linting 'ignoreErrorsTests.swift' (37/51)
Linting 'IgnoreTests.swift' (38/51)
Linting 'ignoreWhenTests.swift' (39/51)
Linting 'mapToTests.swift' (40/51)
Linting 'materialized+elementsTests.swift' (41/51)
Linting 'notTests.swift' (42/51)
Linting 'nwiseTests.swift' (43/51)
Linting 'OnceTests.swift' (44/51)
Linting 'pausableBufferedTests.swift' (45/51)
Linting 'pausableTests.swift' (46/51)
Linting 'repeatWithBehaviorTests.swift' (47/51)
Linting 'retryWithBehaviorTests.swift' (48/51)
Linting 'UnwrapTests.swift' (49/51)
Linting 'WeakTarget.swift' (50/51)
Linting 'WeakTests.swift' (51/51)
Done linting! Found 0 violations, 0 serious in 51 files.

All looks good here !
Would appreciate a review approval unless you have more notes.

Gonna do Danger next, so we actually have something that uses this in PRs.

fpillet · 2017-12-12T17:03:25Z

.swiftlint.yml

+type_body_length: 500
+cyclomatic_complexity:
+  warning: 15
+  error: 25


Don't we need a trailing newline here? I'm surprised that swiftlint doesn't complain

newline after what? the last line here?
Neh its YAML, no need :)

freak4pc · 2017-12-12T17:55:43Z

Ok so three last changes:

I disabled the trailing_comma rule per our discussion.
Uppercased all Test filenames (ClassCase).
Added a rule to enforce a unified copyright header and fixed wrong copyright headers across files.

This should be it for this PR :) Let me know if any other open points.

fpillet · 2017-12-12T18:07:52Z

Great work. Now I have to install Swiftlint 💯

bobgodwinx · 2017-12-12T18:14:23Z

Great job guys 👍 in relation to dangling commas I would enforce it because it creates a good habit. Actually I was against it at the beginning but now I found it useful. and FYI they use it in swift devs :I check this link and draw your conclusions. read also the comments 🌮 https://medium.com/@nikgraf/why-you-should-enforce-dangling-commas-for-multiline-statements-d034c98e36f8

freak4pc · 2017-12-12T18:16:43Z

@bobgodwinx thanks for the feedback :) I don't think its a "Good habit" more like it is a preference. I also, in my own codebase, don't like having dangling commas. But I really don't it this way or the other, specifically for these multi-lined cases where it benefits the diffs.

Let's see if this is abused in next PRs, and iterate 💯

bobgodwinx · 2017-12-12T18:31:36Z

@freak4pc no worries. The same topic also exploded the swift dev team :) just read their mailing list. FYI I am standing on the fence on this. But it's good to hear all options.

http://ericasadun.com/2016/05/27/trailing-commas-open-source-and-community-participation/
http://thread.gmane.org/gmane.comp.lang.swift.evolution/18568
https://github.com/apple/swift-evolution/blob/master/proposals/0084-trailing-commas.md

freak4pc · 2017-12-12T18:33:08Z

@bobgodwinx Yeah I've read the last two before responding to @mosamer but those are about tuples, where it's really not as relevant. That was also the Rejection Rationale by Lattner.

mosamer · 2017-12-12T19:08:49Z

.swiftlint.yml

@@ -3,6 +3,7 @@ included:
  - Tests
 disabled_rules:
  - line_length
+  - trailing_comma


this disables the rule, so no checks are made. To mandate the use of a trailing comma on a multiline collections I think it should read as

trailing_comma: mandatory_comma: true

I think this is a bit towards the other extreme and I'm not sure that's a good choice 🤔

I'm in favor of not having the rule at all so you could have either with or without. There seems to be several that are opposed and several that are for, so I wouldn't say it's enough of a call it mandatory for everyone.

never mind, i misunderstood earlier comments as opting-in for the use of trailing commas. Disabling the rule should be the best choice for now until we settle :)

Added Swiftlint rules file

f4d2235

freak4pc force-pushed the feature/swiftlint branch 2 times, most recently from 5e95dc0 to bfda8b5 Compare December 12, 2017 13:25

freak4pc mentioned this pull request Dec 12, 2017

[WIP] RxSwiftExt & RxCocoaExt Action Checklist #127

Open

14 tasks

freak4pc force-pushed the feature/swiftlint branch from bfda8b5 to edfa2e6 Compare December 12, 2017 13:33

freak4pc added 2 commits December 12, 2017 15:34

Updated Sources

79166c0

Updated Tests

f9c6ff4

freak4pc force-pushed the feature/swiftlint branch from edfa2e6 to 122edcb Compare December 12, 2017 13:34

freak4pc commented Dec 12, 2017

View reviewed changes

mosamer reviewed Dec 12, 2017

View reviewed changes

Added Swiftlint to Travis

a3c7a48

freak4pc force-pushed the feature/swiftlint branch from 122edcb to a3c7a48 Compare December 12, 2017 14:03

fpillet reviewed Dec 12, 2017

View reviewed changes

Disabled trailing_comma rule.

6bf998b

freak4pc force-pushed the feature/swiftlint branch from 755283f to c6c2e35 Compare December 12, 2017 17:54

freak4pc force-pushed the feature/swiftlint branch from c6c2e35 to 52dfeaa Compare December 12, 2017 18:06

fpillet approved these changes Dec 12, 2017

View reviewed changes

mosamer reviewed Dec 12, 2017

View reviewed changes

Uppercased test filenames and unified copyright header.

4d67ba3

freak4pc force-pushed the feature/swiftlint branch from 52dfeaa to 4d67ba3 Compare December 12, 2017 19:46

freak4pc merged commit b113518 into master Dec 12, 2017

freak4pc deleted the feature/swiftlint branch December 12, 2017 20:58

freak4pc added this to the RxCocoaExt milestone Dec 25, 2017


		// provide simple predicate that always return true

		// provide simple predicate that always returns true

Swiftlint #128

Swiftlint #128

Conversation

freak4pc commented Dec 12, 2017

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

mosamer Dec 12, 2017 • 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

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

mosamer Dec 12, 2017 • 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

mosamer commented Dec 12, 2017

freak4pc commented Dec 12, 2017

mosamer commented Dec 12, 2017

freak4pc commented Dec 12, 2017

freak4pc commented Dec 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

freak4pc commented Dec 12, 2017 • edited Loading

fpillet commented Dec 12, 2017

bobgodwinx commented Dec 12, 2017

freak4pc commented Dec 12, 2017

bobgodwinx commented Dec 12, 2017

freak4pc commented Dec 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mosamer Dec 12, 2017 •

edited

Loading

mosamer Dec 12, 2017 •

edited

Loading

freak4pc commented Dec 12, 2017 •

edited

Loading

freak4pc commented Dec 12, 2017 •

edited

Loading