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

Add rule for single space after period on comments #4624

Merged
merged 7 commits into from
Dec 15, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .swiftlint.yml
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ opt_in_rules:
- overridden_super_call
- override_in_extension
- pattern_matching_keywords
- period_spacing
- prefer_self_type_over_type_of_self
- private_action
- private_outlet
Expand Down
5 changes: 5 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,11 @@
`single_test_class` and `empty_xctest_method` rules.
[Martin Redington](https://github.com/mildm8nnered)
[#4200](https://github.com/realm/SwiftLint/issues/4200)

* Add `period_spacing` opt-in rule that checks periods are not followed
by 2 or more spaces in comments.
[Julioacarrettoni](https://github.com/Julioacarrettoni)
[#4624](https://github.com/realm/SwiftLint/pull/4624)

#### Bug Fixes

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
import IDEUtils

extension SyntaxClassification {
// True if it is any kind of comment.
var isComment: Bool {
switch self {
case .lineComment, .docLineComment, .blockComment, .docBlockComment:
return true
case .none, .keyword, .identifier, .typeIdentifier, .operatorIdentifier, .dollarIdentifier, .integerLiteral,
.floatingLiteral, .stringLiteral, .stringInterpolationAnchor, .poundDirectiveKeyword, .buildConfigId,
.attribute, .objectLiteral, .editorPlaceholder:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why the full set of cases here? To make sure the compiler warns if there are new kinds?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Exactly 💯

return false
}
}
}
1 change: 1 addition & 0 deletions Source/SwiftLintFramework/Models/PrimaryRuleList.swift
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,7 @@ public let primaryRuleList = RuleList(rules: [
OverriddenSuperCallRule.self,
OverrideInExtensionRule.self,
PatternMatchingKeywordsRule.self,
PeriodSpacingRule.self,
PreferNimbleRule.self,
PreferSelfInStaticReferencesRule.self,
PreferSelfTypeOverTypeOfSelfRule.self,
Expand Down
12 changes: 3 additions & 9 deletions Source/SwiftLintFramework/Rules/Lint/CommentSpacingRule.swift
Original file line number Diff line number Diff line change
Expand Up @@ -122,15 +122,9 @@ struct CommentSpacingRule: SourceKitFreeRule, ConfigurationProviderRule, Substit
func violationRanges(in file: SwiftLintFile) -> [NSRange] {
// Find all comment tokens in the file and regex search them for violations
file.syntaxClassifications
.compactMap { (classifiedRange: SyntaxClassifiedRange) -> [NSRange]? in
switch classifiedRange.kind {
case .blockComment, .docBlockComment, .lineComment, .docLineComment:
break
default:
return nil
}

let range = classifiedRange.range.toSourceKittenByteRange()
.filter(\.kind.isComment)
.map { $0.range.toSourceKittenByteRange() }
Copy link
Collaborator

Choose a reason for hiding this comment

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

The isComment seems to be missing here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's why I get for doing manual copy&paste.
Thank you so much for the thorough review!

I'm kinda surprised by the lack of false positives with this mistake. I mean, the list of examples is not exhaustive but the rule was also run on all those other repos right?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, the rule is run on all the repositories listed above. The violations are truncated to 135 while there is a total of 531, though. So I guess we either just don't see the violations in comment_spacing or the regex used in the rule is already precise enough.

If you have an idea for a test case that would exactly trigger in such a case, do not hesitate to add it. 😉

.compactMap { (range: ByteRange) -> [NSRange]? in
return file.stringView
.substringWithByteRange(range)
.map(StringView.init)
Expand Down
94 changes: 94 additions & 0 deletions Source/SwiftLintFramework/Rules/Lint/PeriodSpacingRule.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,94 @@
import Foundation
import IDEUtils
import SourceKittenFramework

struct PeriodSpacingRule: SourceKitFreeRule, ConfigurationProviderRule, OptInRule, SubstitutionCorrectableRule {
var configuration = SeverityConfiguration(.warning)

init() {}

static let description = RuleDescription(
identifier: "period_spacing",
name: "Period Spacing",
description: "Periods should not be followed by more than one space.",
kind: .style,
nonTriggeringExamples: [
Example("let pi = 3.2"),
Example("let pi = Double.pi"),
Example("let pi = Double. pi"),
Example("let pi = Double. pi"),
Example("// A. Single."),
Example("/// - code: Identifier of the error. Integer."),
Example("""
// value: Multiline.
// Comment.
"""),
Example("""
/**
Sentence ended in period.

- Sentence 2 new line characters after.
**/
""")
],
triggeringExamples: [
Example("/* Only god knows why. ↓ This symbol does nothing. */", testWrappingInComment: false),
Example("// Only god knows why. ↓ This symbol does nothing.", testWrappingInComment: false),
Example("// Single. Double. ↓ End.", testWrappingInComment: false),
Example("// Single. Double. ↓ Triple. ↓ End.", testWrappingInComment: false),
Example("// Triple. ↓ Quad. ↓ End.", testWrappingInComment: false),
Example("/// - code: Identifier of the error. ↓ Integer.", testWrappingInComment: false)
],
corrections: [
Example("/* Why. ↓ Symbol does nothing. */"): Example("/* Why. Symbol does nothing. */"),
Example("// Why. ↓ Symbol does nothing."): Example("// Why. Symbol does nothing."),
Example("// Single. Double. ↓ End."): Example("// Single. Double. End."),
Example("// Single. Double. ↓ Triple. ↓ End."): Example("// Single. Double. Triple. End."),
Example("// Triple. ↓ Quad. ↓ End."): Example("// Triple. Quad. End."),
Example("/// - code: Identifier. ↓ Integer."): Example("/// - code: Identifier. Integer.")
]
)

func violationRanges(in file: SwiftLintFile) -> [NSRange] {
// Find all comment tokens in the file and regex search them for violations
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd love to see a rule enforcing periods to finalize comments. 😉 Can we have a period after each sentence to be consistent?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Uhhhh! I like that!
Should it be a separate rule though?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Well, I don't think such a rule would be feasible. It's hard to recognize full sentences. A comment can also be a link, a code snippet or a few words which don't result into a sentence.

My comment was rather meant as a joke referring to your comments in the new rule ... 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm glad that's the case, I had a mini crisis trying to figure out the rule for that 😅

file.syntaxClassifications
.filter(\.kind.isComment)
.map { $0.range.toSourceKittenByteRange() }
.compactMap { (range: ByteRange) -> [NSRange]? in
return file.stringView
.substringWithByteRange(range)
.map(StringView.init)
.map { commentBody in
// Look for a period followed by two or more whitespaces but not new line or carriage returns
return regex(#"\.[^\S\r\n]{2,}"#)
.matches(in: commentBody)
.compactMap { result in
// Set the location to start from the second whitespace till the last one.
return file.stringView.byteRangeToNSRange(
ByteRange(
// Safe to mix NSRange offsets with byte offsets here because the
// regex can't contain multi-byte characters
location: ByteCount(range.lowerBound.value + result.range.lowerBound + 2),
length: ByteCount(result.range.length.advanced(by: -2))
)
)
}
}
}
.flatMap { $0 }
}

func validate(file: SwiftLintFile) -> [StyleViolation] {
return violationRanges(in: file).map { range in
StyleViolation(
ruleDescription: Self.description,
severity: configuration.severity,
location: Location(file: file, characterOffset: range.location)
)
}
}

func substitution(for violationRange: NSRange, in file: SwiftLintFile) -> (NSRange, String)? {
return (violationRange, "")
}
}
6 changes: 6 additions & 0 deletions Tests/GeneratedTests/GeneratedTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -811,6 +811,12 @@ class PatternMatchingKeywordsRuleGeneratedTests: XCTestCase {
}
}

class PeriodSpacingRuleGeneratedTests: XCTestCase {
func testWithDefaultConfiguration() {
verifyRule(PeriodSpacingRule.description)
}
}

class PreferNimbleRuleGeneratedTests: XCTestCase {
func testWithDefaultConfiguration() {
verifyRule(PreferNimbleRule.description)
Expand Down