-
Notifications
You must be signed in to change notification settings - Fork 247
Ignore sentence terminators inside quotes when applying the 'BeginDocumentationCommentWithOneLineSummary' option. #687
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,6 +11,9 @@ | |
//===----------------------------------------------------------------------===// | ||
|
||
import Foundation | ||
#if os(macOS) | ||
import NaturalLanguage | ||
#endif | ||
import SwiftSyntax | ||
|
||
/// All documentation comments must begin with a one-line summary of the declaration. | ||
|
@@ -125,13 +128,31 @@ public final class BeginDocumentationCommentWithOneLineSummary: SyntaxLintRule { | |
} | ||
|
||
var sentences = [String]() | ||
var tags = [NLTag]() | ||
var tokenRanges = [Range<String.Index>]() | ||
let tags = text.linguisticTags( | ||
|
||
let tagger = NLTagger(tagSchemes: [.lexicalClass]) | ||
tagger.string = text | ||
tagger.enumerateTags( | ||
in: text.startIndex..<text.endIndex, | ||
scheme: NSLinguisticTagScheme.lexicalClass.rawValue, | ||
tokenRanges: &tokenRanges) | ||
unit: .word, | ||
scheme: .lexicalClass | ||
) { tag, range in | ||
if let tag { | ||
tags.append(tag) | ||
tokenRanges.append(range) | ||
} | ||
return true | ||
} | ||
|
||
var isInsideQuotes = false | ||
let sentenceTerminatorIndices = tags.enumerated().filter { | ||
$0.element == "SentenceTerminator" | ||
if $0.element == NLTag.openQuote { | ||
isInsideQuotes = true | ||
} else if $0.element == NLTag.closeQuote { | ||
isInsideQuotes = false | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was going to point out that this won't handle nested quotes correctly, but it looks like the Foundation API itself doesn't categorize them correctly either: func tags(_ text: String) {
var tokenRanges = [Range<String.Index>]()
let tags = text.linguisticTags(in: text.startIndex..<text.endIndex, scheme: NSLinguisticTagScheme.lexicalClass.rawValue, tokenRanges: &tokenRanges)
print(Array(tags))
}
tags("\"Hello 'world'\"")
// ["OpenQuote", "Interjection", "Whitespace", "OpenQuote", "Noun", "CloseQuote", "OpenQuote"] So an attempt to increment/decrement a nested quote count won't work as intended. In other words, I wanted to point it out but you don't need to change anything. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, this is entirely dependent on the behavior of the Foundation API. The replacement, |
||
} | ||
return !isInsideQuotes && $0.element == NLTag.sentenceTerminator | ||
}.map { | ||
tokenRanges[$0.offset].lowerBound | ||
} | ||
|
@@ -152,8 +173,8 @@ public final class BeginDocumentationCommentWithOneLineSummary: SyntaxLintRule { | |
/// Returns the best approximation of sentences in the given text using string splitting around | ||
/// periods that are followed by spaces. | ||
/// | ||
/// This method is a fallback for platforms (like Linux, currently) where `String` does not | ||
/// support `NSLinguisticTagger` and its related APIs. It will fail to catch certain kinds of | ||
/// This method is a fallback for platforms (like Linux, currently) that does not | ||
/// support `NaturalLanguage` and its related APIs. It will fail to catch certain kinds of | ||
/// sentences (such as those containing abbreviations that are followed by a period, like "Dr.") | ||
/// that the more advanced API can handle. | ||
private func nonLinguisticSentenceApproximations(in text: String) -> ( | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -139,4 +139,34 @@ final class BeginDocumentationCommentWithOneLineSummaryTests: LintOrFormatRuleTe | |
) | ||
#endif | ||
} | ||
|
||
func testSentenceTerminationInsideQuotes() { | ||
assertLint( | ||
BeginDocumentationCommentWithOneLineSummary.self, | ||
""" | ||
/// Creates an instance with the same raw value as `x` failing iff `x.kind != Subject.kind`. | ||
struct TestBackTick {} | ||
|
||
/// A set of `Diagnostic` that can answer the question ‘was there an error?’ in O(1). | ||
struct TestSingleSmartQuotes {} | ||
|
||
/// A set of `Diagnostic` that can answer the question 'was there an error?' in O(1). | ||
struct TestSingleStraightQuotes {} | ||
|
||
/// A set of `Diagnostic` that can answer the question “was there an error?” in O(1). | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Both this example and the one below use smart quotes. Can you add one with regular ASCII (straight) double quotes? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure, I'll add it! |
||
struct TestDoubleSmartQuotes {} | ||
|
||
/// A set of `Diagnostic` that can answer the question "was there an error?" in O(1). | ||
struct TestDoubleStraightQuotes {} | ||
|
||
/// A set of `Diagnostic` that can answer the question “was there | ||
/// an error?” in O(1). | ||
struct TestTwoLinesDoubleSmartQuotes {} | ||
|
||
/// A set of `Diagnostic` that can answer the question "was there | ||
/// an error?" in O(1). | ||
struct TestTwoLinesDoubleStraightQuotes {} | ||
""" | ||
) | ||
} | ||
} |
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
#if os(macOS)
around this import to match where it's used, since it won't exist on Linux.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, I missed it.