-
Notifications
You must be signed in to change notification settings - Fork 2.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add human readable reporter that’s also funny because emoji. #916
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a test to ReporterTests.swift
.
@@ -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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we generally use a single empty line between changelog entries
} | ||
|
||
public extension SequenceType { | ||
func categorise<U: Hashable>(@noescape by transform: Generator.Element -> U) -> [U:[Generator.Element]] { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we use American English spellings throughout the project: categorize
|
||
public extension SequenceType { | ||
func categorise<U: Hashable>(@noescape by transform: Generator.Element -> U) -> [U:[Generator.Element]] { | ||
var dictionary: [U:[Generator.Element]] = [:] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we're in the process of adding a new rule requiring a space after the colon in dictionaries: var dictionary = [U: [Generator.Element]]()
|
||
public static func generateReport(violations: [StyleViolation]) -> String { | ||
return violations.categorise { | ||
(violation) in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
closure parameters on the same line as opening brace please: { violation in
(violation) in | ||
violation.location.file ?? "Other" | ||
}.map { | ||
(filename, violations) in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
closure parameters on the same line as opening brace please: { filename, violations in
6ac4ae2
to
b7ef2d0
Compare
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. |
@@ -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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing two trailing spaces. See https://github.com/realm/SwiftLint/blob/master/CONTRIBUTING.md#tracking-changes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, okay 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Out of curiosity: what's the reason behind this rule?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we follow the same changelog format as CocoaPods, I don't know where the motivation for this came from.
@@ -24,3 +24,27 @@ extension Array where Element: NSTextCheckingResult { | |||
return map { $0.range } | |||
} | |||
} | |||
|
|||
extension Array where Element: Equatable { | |||
var unique: [Element] { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
internal var
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't it internal by default?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, but we prefer explicit ACLs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should I add them everywhere in the file then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nah, it's fine, this file in particular doesn't seem to follow that convention.
} | ||
} | ||
|
||
public extension SequenceType { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
internal extension
? This doesn't seem to be relevant in SwiftLint's public API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, I copy-pasted it from my other project, doesn't make any sense for this to be public.
|
||
public extension SequenceType { | ||
// swiftlint:disable line_length | ||
func categorize<U: Hashable>(@noescape by transform: Generator.Element -> U) -> [U: [Generator.Element]] { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe group
instead of categorize
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm… I was wondering about the same thing, but you're right, group
sound much nicer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By the way, SourceKitten already has implementations for these, in case you want to compare:
b7ef2d0
to
6791388
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just one more tiny thing, then I think this can land.
} | ||
|
||
extension SequenceType { | ||
// swiftlint:disable line_length |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
4fbafc1
to
53a1115
Compare
@jpsim okay, fixed that tiny thing and also rebased, thanks for all the help! |
Hey @jpsim, any chance you could take another look on this? :) |
@justMaku Are you sure you've rebased this properly? This is failing on CI because there's no |
53a1115
to
a5db9f7
Compare
Uhm, this is embarrassing 😅. Somehow I was working on a stale master branch still supporting Swift 2.0 Fixed now. |
Current coverage is 83.17% (diff: 75.75%)@@ 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
|
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: