Add human readable reporter that’s also funny because emoji. #916

justMaku · 2016-12-01T17:55:45Z

This pull request adds a new reporter type that's much easier to read for human eye. I believe it could be useful for people that use swiftlint for example in a pre-commit hook or on CI.

Example:

/Users/maku/Projects/Temp/Otter.swift
⛔️ Line 159: Function should have complexity 10 or less: currently complexity equals 21
⛔️ Line 185: Function should have complexity 10 or less: currently complexity equals 96
⚠️ Line 12: Lines should not have trailing whitespace.
⚠️ Line 18: Lines should not have trailing whitespace.
⚠️ Line 115: Lines should not have trailing whitespace.

/Users/maku/Projects/Temp/Other.swift
⚠️ Line 23: Lines should not have trailing whitespace.
⚠️ Line 69: Lines should not have trailing whitespace.
⚠️ Line 92: Lines should not have trailing whitespace.

jpsim

Please add a test to ReporterTests.swift.

jpsim · 2016-12-01T18:28:46Z

CHANGELOG.md

@@ -11,6 +11,8 @@
  [Marcelo Fabri](https://github.com/marcelofabri)
  [#910](https://github.com/realm/SwiftLint/issues/910)
  [#911](https://github.com/realm/SwiftLint/issues/911)
+* Add `EmojiReporter`: a human readble reporter.


we generally use a single empty line between changelog entries

jpsim · 2016-12-01T18:29:19Z

Source/SwiftLintFramework/Extensions/Array+SwiftLint.swift

+}
+
+public extension SequenceType {
+    func categorise<U: Hashable>(@noescape by transform: Generator.Element -> U) -> [U:[Generator.Element]] {


we use American English spellings throughout the project: categorize

jpsim · 2016-12-01T18:30:04Z

Source/SwiftLintFramework/Extensions/Array+SwiftLint.swift

+
+public extension SequenceType {
+    func categorise<U: Hashable>(@noescape by transform: Generator.Element -> U) -> [U:[Generator.Element]] {
+        var dictionary: [U:[Generator.Element]] = [:]


we're in the process of adding a new rule requiring a space after the colon in dictionaries: var dictionary = [U: [Generator.Element]]()

jpsim · 2016-12-01T18:30:51Z

Source/SwiftLintFramework/Reporters/EmojiReporter.swift

+
+    public static func generateReport(violations: [StyleViolation]) -> String {
+        return violations.categorise {
+                (violation) in


closure parameters on the same line as opening brace please: { violation in

jpsim · 2016-12-01T18:31:04Z

Source/SwiftLintFramework/Reporters/EmojiReporter.swift

+                (violation) in
+                violation.location.file ?? "Other"
+            }.map {
+                (filename, violations) in


closure parameters on the same line as opening brace please: { filename, violations in

justMaku · 2016-12-01T21:08:55Z

Thanks for the feedback @jpsim!

I've updated the PR with the changes you've requested and also added a mention about the emoji reporter to the both README files.

jpsim · 2016-12-01T21:09:29Z

CHANGELOG.md

@@ -26,6 +26,9 @@
  [#910](https://github.com/realm/SwiftLint/issues/910)
  [#911](https://github.com/realm/SwiftLint/issues/911)

+* Add `EmojiReporter`: a human readble reporter.


missing two trailing spaces. See https://github.com/realm/SwiftLint/blob/master/CONTRIBUTING.md#tracking-changes

Oh, okay 😅

Out of curiosity: what's the reason behind this rule?

we follow the same changelog format as CocoaPods, I don't know where the motivation for this came from.

jpsim · 2016-12-01T21:10:04Z

Source/SwiftLintFramework/Extensions/Array+SwiftLint.swift

@@ -24,3 +24,27 @@ extension Array where Element: NSTextCheckingResult {
        return map { $0.range }
    }
 }
+
+extension Array where Element: Equatable {
+    var unique: [Element] {


internal var?

Isn't it internal by default?

yes, but we prefer explicit ACLs.

Should I add them everywhere in the file then?

nah, it's fine, this file in particular doesn't seem to follow that convention.

jpsim · 2016-12-01T21:10:30Z

Source/SwiftLintFramework/Extensions/Array+SwiftLint.swift

+    }
+}
+
+public extension SequenceType {


internal extension? This doesn't seem to be relevant in SwiftLint's public API.

Right, I copy-pasted it from my other project, doesn't make any sense for this to be public.

jpsim · 2016-12-01T21:10:54Z

Source/SwiftLintFramework/Extensions/Array+SwiftLint.swift

+
+public extension SequenceType {
+    // swiftlint:disable line_length
+    func categorize<U: Hashable>(@noescape by transform: Generator.Element -> U) -> [U: [Generator.Element]] {


maybe group instead of categorize?

Hmmm… I was wondering about the same thing, but you're right, group sound much nicer.

By the way, SourceKitten already has implementations for these, in case you want to compare:

https://github.com/jpsim/SourceKitten/blob/0.15.2/Source/SourceKittenFramework/ClangTranslationUnit.swift#L16-L33

jpsim

Just one more tiny thing, then I think this can land.

jpsim · 2016-12-01T22:01:15Z

Source/SwiftLintFramework/Extensions/Array+SwiftLint.swift

+}
+
+extension SequenceType {
+    // swiftlint:disable line_length


if you just want to disable for the next line (as opposed to the entire rest of the file, as is happening currently), change this to // swiftlint:disable:next line_length

justMaku · 2016-12-02T11:15:02Z

@jpsim okay, fixed that tiny thing and also rebased, thanks for all the help!

justMaku · 2016-12-06T11:05:57Z

Hey @jpsim, any chance you could take another look on this? :)

marcelofabri · 2016-12-07T01:55:42Z

@justMaku Are you sure you've rebased this properly? This is failing on CI because there's no SequenceType in Swift 3.0 (it was renamed).

justMaku · 2016-12-07T10:37:09Z

Uhm, this is embarrassing 😅. Somehow I was working on a stale master branch still supporting Swift 2.0

Fixed now.

codecov-io · 2016-12-07T10:44:18Z

Current coverage is 83.17% (diff: 75.75%)

Merging #916 into master will decrease coverage by 0.05%

@@             master       #916   diff @@
==========================================
  Files           114        115     +1   
  Lines          5136       5201    +65   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           4275       4326    +51   
- Misses          861        875    +14   
  Partials          0          0

Powered by Codecov. Last update 291ecd6...a5db9f7

jpsim · 2016-12-08T07:52:41Z

Thanks for the PR @justMaku! I addressed the merge conflict and refactored things a bit, so I'll now be merging this as part of #945.

jpsim requested changes Dec 1, 2016

View reviewed changes

justMaku force-pushed the feature/emoji_reporter branch 2 times, most recently from 6ac4ae2 to b7ef2d0 Compare December 1, 2016 21:07

jpsim requested changes Dec 1, 2016

View reviewed changes

justMaku force-pushed the feature/emoji_reporter branch from b7ef2d0 to 6791388 Compare December 1, 2016 21:39

jpsim requested changes Dec 1, 2016

View reviewed changes

justMaku force-pushed the feature/emoji_reporter branch 2 times, most recently from 4fbafc1 to 53a1115 Compare December 2, 2016 11:14

marcelofabri mentioned this pull request Dec 6, 2016

Closure parameters on the same line as opening brace #931

Closed

Add human readable reporter that’s also funny because emoji.

a5db9f7

justMaku force-pushed the feature/emoji_reporter branch from 53a1115 to a5db9f7 Compare December 7, 2016 10:29

jpsim mentioned this pull request Dec 8, 2016

Emoji reporter #945

Merged

jpsim closed this Dec 8, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add human readable reporter that’s also funny because emoji. #916

Add human readable reporter that’s also funny because emoji. #916

justMaku commented Dec 1, 2016 •

edited

Loading

jpsim left a comment

jpsim Dec 1, 2016

jpsim Dec 1, 2016

jpsim Dec 1, 2016

jpsim Dec 1, 2016

jpsim Dec 1, 2016

justMaku commented Dec 1, 2016

jpsim Dec 1, 2016

justMaku Dec 1, 2016

justMaku Dec 1, 2016

jpsim Dec 1, 2016

jpsim Dec 1, 2016

justMaku Dec 1, 2016

jpsim Dec 1, 2016

justMaku Dec 1, 2016

jpsim Dec 1, 2016

jpsim Dec 1, 2016

justMaku Dec 1, 2016

jpsim Dec 1, 2016

justMaku Dec 1, 2016

jpsim Dec 1, 2016

jpsim left a comment

jpsim Dec 1, 2016

justMaku commented Dec 2, 2016

justMaku commented Dec 6, 2016

marcelofabri commented Dec 7, 2016

justMaku commented Dec 7, 2016 •

edited

Loading

codecov-io commented Dec 7, 2016

jpsim commented Dec 8, 2016

Add human readable reporter that’s also funny because emoji. #916

Add human readable reporter that’s also funny because emoji. #916

Conversation

justMaku commented Dec 1, 2016 • edited Loading

jpsim left a 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

justMaku commented Dec 1, 2016

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

jpsim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justMaku commented Dec 2, 2016

justMaku commented Dec 6, 2016

marcelofabri commented Dec 7, 2016

justMaku commented Dec 7, 2016 • edited Loading

codecov-io commented Dec 7, 2016

Current coverage is 83.17% (diff: 75.75%)

jpsim commented Dec 8, 2016

justMaku commented Dec 1, 2016 •

edited

Loading

justMaku commented Dec 7, 2016 •

edited

Loading