Skip to content

Conversation

@erwald
Copy link
Member

@erwald erwald commented Jul 19, 2017

Checklist

  • Updated CHANGELOG.md.

This PR adds the .swiftlint.yml config file, and runs swiftlint autocorrect to fix some issues. Mind, there are still 740 violations left (of which 601 are classified as "serious").

I disabled the opening_braces rule because it didn't like arrays of closures, like this:

let expectations: [(String) -> Void] = [
  { event in expect(event) == "[] value 1" },
  { event in expect(event) == "[] completed" },
  { event in expect(event) == "[] terminated" },
  { event in expect(event) == "[] disposed" }
]

Maybe some other rules need to be disabled as well, such as the tuple size and identifier name length rules.

@andersio
Copy link
Member

It doesn’t need to be in changelog, since it is not relevant to the public API.

@erwald
Copy link
Member Author

erwald commented Jul 20, 2017

I suspected as much. Fixed!


public static let allEvents: Set<Signal> = [
.value, .completed, .failed, .terminated, .disposed, .interrupted,
.value, .completed, .failed, .terminated, .disposed, .interrupted
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we change this rule? By default SwiftLint enforces no trailing comma. I think we should enforce the trailing comma. This tends to make diffs more readable and merges easier since adding a line doesn't alter the previously last item in the array.

Copy link
Member Author

Choose a reason for hiding this comment

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

Makes sense. I added the exception and reinserted the commas.

erwald added 2 commits July 20, 2017 22:46
It makes diffs more readable, merges easier. Also, it makes it
easier to rearrange stuff.
.swiftlint.yml Outdated
@@ -0,0 +1,8 @@
disabled_rules:
- opening_brace
- trailing_comma
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of disabling it, you can actually require the comma by adding this at the top level:

trailing_comma:
  mandatory_comma: true

Would you mind doing that instead? Sorry that wasn't clear before. 😅

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah-ha! Done and done.

@erwald
Copy link
Member Author

erwald commented Jul 21, 2017

Not quite sure why the Linux build fails – it seems Test Case 'XCTestCase.MutableProperty, should not deadlock' started at 18:02:17.226 does, in fact, deadlock.

But I'm not sure how this PR could affect that. Are the tests sometimes unstable, by any chance?

@mdiep
Copy link
Contributor

mdiep commented Jul 23, 2017

I think that particular test is unstable. I've rerun those tests—hopefully they'll pass this time.

@andersio andersio merged commit 187f0f1 into ReactiveCocoa:master Jul 23, 2017
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.

3 participants