Skip to content
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

Vertical whitespace msg fix #1774

Merged
merged 7 commits into from
Aug 14, 2017

Conversation

hossamghareeb
Copy link
Contributor

Fixing issue #1763
Fixes:

  1. Fix the warning message to display the maximum empty lines allowed based on maxEmptyLines.
  2. Fix autocorrect to delete empty lines till and keep the maxEmptyLines and not 1.

@SwiftLintBot
Copy link

SwiftLintBot commented Aug 13, 2017

12 Messages
📖 Linting Aerial with this PR took 0.36s vs 0.34s on master (5% slower)
📖 Linting Alamofire with this PR took 2.35s vs 2.47s on master (4% faster)
📖 Linting Firefox with this PR took 9.98s vs 10.47s on master (4% faster)
📖 Linting Kickstarter with this PR took 14.78s vs 15.33s on master (3% faster)
📖 Linting Moya with this PR took 1.01s vs 1.04s on master (2% faster)
📖 Linting Nimble with this PR took 1.37s vs 1.41s on master (2% faster)
📖 Linting Quick with this PR took 0.44s vs 0.45s on master (2% faster)
📖 Linting Realm with this PR took 2.08s vs 2.19s on master (5% faster)
📖 Linting SourceKitten with this PR took 0.78s vs 0.81s on master (3% faster)
📖 Linting Sourcery with this PR took 3.47s vs 3.63s on master (4% faster)
📖 Linting Swift with this PR took 9.99s vs 10.32s on master (3% faster)
📖 Linting WordPress with this PR took 9.19s vs 9.48s on master (3% faster)

Generated by 🚫 Danger

CHANGELOG.md Outdated
@@ -38,6 +38,23 @@
[Otávio Lima](https://github.com/otaviolima)
[#1710](https://github.com/realm/SwiftLint/issues/1710)

## 0.22.0: Coffee Maker
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this here?

This change should be added to Master section.

@@ -11,6 +11,8 @@ import XCTest

class VerticalWhitespaceRuleTests: XCTestCase {

private let ruleID = "vertical_whitespace"
Copy link
Collaborator

Choose a reason for hiding this comment

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

you should use VerticalWhitespaceRule.description.identifier

}
}

func testVilationMessageWithDefaultConfiguration() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

typo

}
let allViolations = violations("let aaaa = 0\n\n\n\nlet bbb = 2\n", config: config)
let verticalWhiteSpaceViolation = allViolations.first(where: { $0.ruleDescription.identifier == ruleID })
if let violation = verticalWhiteSpaceViolation {
Copy link
Collaborator

Choose a reason for hiding this comment

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

this test will silently fail if no violation is triggered. It should fail instead.

func testVilationMessageWithDefaultConfiguration() {
let allViolations = violations("let aaaa = 0\n\n\n\nlet bbb = 2\n")
let verticalWhiteSpaceViolation = allViolations.first(where: { $0.ruleDescription.identifier == ruleID })
if let violation = verticalWhiteSpaceViolation {
Copy link
Collaborator

Choose a reason for hiding this comment

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

this test will silently fail if no violation is triggered. It should fail instead.

for eachLine in linesSections {
let start = eachLine.lastLine.index - eachLine.linesToRemove
indexOfLinesToDelete.append(contentsOf: start..<eachLine.lastLine.index)
for section in linesSections {
Copy link
Collaborator

Choose a reason for hiding this comment

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

You need to add tests for this change as well.

CHANGELOG.md Outdated
##### Bug Fixes

* Fix the warning message of `vertical-whitespace` rule to display the maximum empty lines allowed if `maxEmptyLines` is greater than 1.
* Fix the autocorrection of `vertical-whitespace` rule to keep the `maxEmptyLines` in the cleaned section.
Copy link
Collaborator

Choose a reason for hiding this comment

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

duplicated message

CHANGELOG.md Outdated
##### Bug Fixes

* Fix the warning message of `vertical-whitespace` rule to display the maximum empty lines allowed if `maxEmptyLines` is greater than 1.
* Fix the autocorrection of `vertical-whitespace` rule to keep the `maxEmptyLines` in the cleaned section.
Copy link
Collaborator

Choose a reason for hiding this comment

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

requires two trailing spaces as described in CONTRIBUTING.md: https://github.com/realm/SwiftLint/blob/master/CONTRIBUTING.md#tracking-changes

Copy link
Collaborator

@marcelofabri marcelofabri left a comment

Choose a reason for hiding this comment

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

I've made some comments that need to be addressed before merging this.

@marcelofabri marcelofabri merged commit ab8b3af into realm:master Aug 14, 2017
@marcelofabri
Copy link
Collaborator

marcelofabri commented Aug 14, 2017

Thanks, @hossamghareeb! I just did some minor changes in 7f5e236.

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