[Abandoned] Fix number of lines reported by function_body_length and type_body_length #1753
Conversation
ornithocoder
force-pushed
the
blank_line
branch
from
f72b796 to
dc38fdb
Compare
Codecov Report
@@ Coverage Diff @@
## master #1753 +/- ##
========================================
Coverage ? 88.8%
========================================
Files ? 244
Lines ? 11902
Branches ? 0
========================================
Hits ? 10570
Misses ? 1332
Partials ? 0
Continue to review full report at Codecov.
|
ornithocoder
changed the title
ornithocoder
force-pushed
the
blank_line
branch
3 times, most recently
from
47d1d57 to
81fb8a6
Compare
|
Reminder to my future self: run |
|
@marcelofabri I'll try to cache the result. Changing it to WIP for now. |
ornithocoder
changed the title
|
@marcelofabri I've tried the cached version, but got the same results. Cache implementation can be found in this gist https://gist.github.com/ornithocoder/eb4b407fdbd8e74cbeb3d7bdb0254bd9. The result is pretty much the same, but honestly, I prefer the cache implementation better and am thinking of pushing it. What do you think? |
|
Since the results are the same, I wouldn't add the cache to keep things simpler. One thing that might help is to only evaluate |
ornithocoder
changed the title
ornithocoder
force-pushed
the
blank_line
branch
from
23cc078 to
9959280
Compare
ornithocoder
changed the title
|
@marcelofabri I decided to give it another try and increased the number of iterations to 20. Got much better results this time. |
ornithocoder
force-pushed
the
blank_line
branch
3 times, most recently
from
4a228ef to
40de68e
Compare
|
@marcelofabri @jpsim this one is ready for code review. I changed #1747 to WIP because I want to rebase that one once the number of lines issue is fixed. |
ornithocoder
changed the title
|
@ornithocoder Just to let you know that I didn't forget about this one. However, this needs a more careful review and I haven't got time yet. 😬 |
ornithocoder
force-pushed
the
blank_line
branch
4 times, most recently
from
0fbef60 to
c14a67d
Compare
ornithocoder
force-pushed
the
blank_line
branch
3 times, most recently
from
659ab6f to
e9d738f
Compare
|
@marcelofabri I've rebased so it shows the performance impact before all the other messages. |
ornithocoder
force-pushed
the
blank_line
branch
from
eec2581 to
aa522de
Compare
|
Hi @marcelofabri @jpsim, any updates on this one? I know it looks like a long PR, but in fact it isn't. The first commit contains the real changes to improve the way the number of lines is calculated and the second one is just to adapt SwiftLint's codebase so function_body_length and type_body_length don't throw any violations. If there's anything I can do to make it easier to review, please let me know. |
|
@marcelofabri @jpsim should I keep rebasing to resolve the CHANGELOG.md conflict or wait for the review first? |
|
I thought about it and I think we probably should raise the default limits instead of applying those fixes. Most of them don't make code easier to read IMO. Or we can even keep the current behavior of considering things that are not tokens ( I'd love to hear @jpsim's thoughts on this too. |
|
I like the idea of applying the first commit (fix) and increasing the default values ;-) |
ornithocoder
force-pushed
the
blank_line
branch
from
aa522de to
f170adf
Compare
|
I've update the PR w/ fix + new default values. I still have the old commit in a different branch, just in case ;-) @marcelofabri @jpsim |
|
@jpsim what's your opinion on increasing the default values for function_body_length from 40 to 50 and type_body_length from 200 to 250? |
|
Hi folks. Any chance of getting this into master? |
ornithocoder
force-pushed
the
blank_line
branch
from
f170adf to
57fbac6
Compare
|
@jpsim @marcelofabri |
|
Finally took a first pass at reviewing this. I agree that if caching results doesn't impact performance, we simply shouldn't cache results to simplify this change. Also, this seems overly complex for what it's trying to do, but maybe I'm missing something? Why isn't this PR as simple as just this: fileprivate func numberOfCommentAndWhitespaceOnlyLines(startLine: Int, endLine: Int) -> Int {
let commentKinds = SyntaxKind.commentKinds
let linesAndSyntaxKinds = zip(lines[(startLine - 1)...(endLine - 1)], syntaxKindsByLines[startLine...endLine])
return linesAndSyntaxKinds.filter { line, kinds in
let whitespaceOnly = line.content.trimmingCharacters(in: .whitespaces).isEmpty
if whitespaceOnly { return true }
let commentOnly = kinds.filter { !commentKinds.contains($0) }.isEmpty
return commentOnly
}.count
} |
ornithocoder
force-pushed
the
blank_line
branch
from
084039a to
c367c42
Compare
|
@jpsim thanks! That's pretty close to the original implementation we had, but for some stupid reason I decided to insist on the cached version :-) I'm pushing a new version that doesn't cache. |
ornithocoder
force-pushed
the
blank_line
branch
from
c367c42 to
041ca54
Compare
| @@ -9,7 +9,7 @@ | |||
| import SourceKittenFramework | |||
|
|
|||
| public struct FunctionBodyLengthRule: ASTRule, ConfigurationProviderRule { | |||
| public var configuration = SeverityLevelsConfiguration(warning: 40, error: 100) | |||
| public var configuration = SeverityLevelsConfiguration(warning: 50, error: 100) | |||
There was a problem hiding this comment.
What's the motivation for this change and why isn't it noted in the changelog?
There was a problem hiding this comment.
The number of violations it throws for the SwiftLint codebase. See here.
There was a problem hiding this comment.
You can override default configuration values in .swiftlint.yml.
| guard !isSwiftLintCommand(token: token, file: file) else { | ||
| continue | ||
| } | ||
| guard !isSwiftLintCommand(token: token, file: file) else { continue } |
There was a problem hiding this comment.
This doesn't help with readability.
There was a problem hiding this comment.
I agree. Any suggestion to decrease the number of lines there to avoid throwing a violation because of the number of lines?
There was a problem hiding this comment.
Could split up into multiple functions.
|
|
||
| return linesAndSyntaxKinds.filter { line, kinds in | ||
| let isLineEmpty = line.content.trimmingCharacters(in: .whitespaces).isEmpty | ||
| return isLineEmpty ? true : !kinds.filter { commentKinds.contains($0) }.isEmpty |
There was a problem hiding this comment.
As a general rule, filter { contains }.isEmpty can always be restructured to contains { contains }
There was a problem hiding this comment.
I'll try to improve it.
There was a problem hiding this comment.
Should be fixed now. Thanks!
| XCTAssertEqual(violations(longFunctionBody), []) | ||
|
|
||
| let longerFunctionBody = violatingFuncWithBody(repeatElement("x = 0\n", count: 40).joined()) | ||
| XCTAssertEqual(violations(longerFunctionBody), [StyleViolation( | ||
| ruleDescription: FunctionBodyLengthRule.description, | ||
| location: Location(file: nil, line: 1, character: 1), | ||
| reason: "Function body should span 40 lines or less excluding comments and " + | ||
| "whitespace: currently spans 41 lines")]) | ||
| "whitespace: currently spans 42 lines")]) |
There was a problem hiding this comment.
Sorry, I fail to see how this was previously incorrect and is now correct.
We're adding 40 lines to the function body additional to its first line:
violatingFuncWithBody(repeatElement("x = 0\n", count: 40).joined())So the body should be considered to span 41 lines.
Where are you getting 42 from?
There was a problem hiding this comment.
I didn't touch the logic in
internal func exceedsLineCountExcludingCommentsAndWhitespace(_ start: Int, _ end: Int,
_ limit: Int) -> (Bool, Int) {
guard end - start > limit else {
return (false, end - start)
}
let count = end - start - numberOfCommentAndWhitespaceOnlyLines(startLine: start, endLine: end)
return (count > limit, count)
}which takes the endLine - startLine.
There was a problem hiding this comment.
Except that the counts did change, you even had to change the tests to accommodate it.
If you look at your screenshot, the body spans from line 2 to line 42, meaning 41 lines.
There was a problem hiding this comment.
That's because the last line contains a single "}" and was computed as empty line :-)
There was a problem hiding this comment.
So the problem now isn't the numberOfCommentAndWhitespaceOnlyLines(startLine:endLine). it now reports the correct number of lines that don't have code. Rules that use exceedsLineCountExcludingCommentsAndWhitespace(_:_:_:) have to be changed to pass the correct start line and end line of blocks.
There was a problem hiding this comment.
I wouldn't count that as the body. i.e. this function should have a body line count of 2:
func foo() { // not body
_ = 0 // body line 1
// not body, neither is above
_ = 0 // body line 2
} // not bodyThere was a problem hiding this comment.
I agree, but that's a different problem. Now,
numberOfCommentAndWhitespaceOnlyLines, andexceedsLineCountExcludingCommentsAndWhitespace
report the correct number of lines with this PR.
What needs to be changed now are the rules that use exceedsLineCountExcludingCommentsAndWhitespace.
The only reason why the rule was reporting the correct number of lines (41) was because numberOfCommentAndWhitespaceOnlyLines was reporting the wrong of valid lines of code. One bug was hiding the other.
Next step is to review all _length rules to pass the correct startLine and endLine to exceedsLineCountExcludingCommentsAndWhitespaceexceedsLineCountExcludingCommentsAndWhitespace.
There was a problem hiding this comment.
I'll squash some of these the commits as "Fixes numberOfCommentAndWhitespaceOnlyLines" and will changed it to WIP, while the rules are not fixed.
| @@ -270,8 +270,11 @@ extension File { | |||
|
|
|||
| fileprivate func numberOfCommentAndWhitespaceOnlyLines(startLine: Int, endLine: Int) -> Int { | |||
| let commentKinds = SyntaxKind.commentKinds | |||
| return syntaxKindsByLines[startLine...endLine].filter { kinds in | |||
| kinds.filter { !commentKinds.contains($0) }.isEmpty | |||
| let linesAndSyntaxKinds = zip(lines[(startLine - 1)...(endLine - 1)], syntaxKindsByLines[startLine...endLine]) | |||
There was a problem hiding this comment.
How about this?
return (startLine...endLine).filter { lineNumber in
let line = lines[lineNumber - 1]
let isLineEmpty = line.content.rangeOfCharacter(from: CharacterSet.whitespaces.inverted) == nil
if isLineEmpty { return true }
return syntaxKindsByLines[lineNumber].contains(where: SyntaxKind.commentKinds.contains)
}.countIt's simpler (IMHO) and more optimized because it avoids unconditionally iterating over all characters in the string, allocating a new string, and mutating it once for every whitespace character.
ornithocoder
force-pushed
the
blank_line
branch
3 times, most recently
from
feb0e3a to
3f99774
Compare
ornithocoder
changed the title
This commit implements a fix for the issue described in realm#1750. The following rules are returning the incorrect number of lines: * `function_body_length` * `type_body_length` This commit: * updates `numberOfCommentAndWhitespaceOnlyLines` to count lines that include the characters { and } as valid lines of code. With the change, the three lines below _are_ valid lines of code: if a == b { a.foo() } Before the fix, only the first two were valid lines. * updates `exceedsLineCountExcludingCommentsAndWhitespace` to take the number of lines in consideration rather than the difference between two line numbers. For instance, the span between lines 1 and 6 is 4 (lines 2, 3, 4, and 5) instead of 5 (endline - startline).
ornithocoder
force-pushed
the
blank_line
branch
from
3f99774 to
5dfed75
Compare
|
Hi @jpsim @marcelofabri, since the method to count the number of lines doesn't understand context (and it shouldn't, so it can be reused for type length, closure length, function length, etc...), it's important to call it with the correct startLine and lastLine. I'm struggling to find the correct firstLine and finalLine for the rule passing 1 and 3 for the code below it returns two valid lines - not empty and not comment - (1 and 3) and one line with empty content (2). So I'm looking for a way to change the Any idea how can I achieve this? Recap:
I've pushed a workaround that includes a Thanks. |
ornithocoder
changed the title
|
I'll have to find another way to
I'm abandoning this PR to start fresh on this issue. |
This commit implements a fix for the issue described in #1750. The following rules are returning the incorrect number of lines:
function_body_lengthtype_body_lengthThis PR contains several commits to make the code review easier.