-
Notifications
You must be signed in to change notification settings - Fork 419
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
DRAFT - Add Trivia.commentValues
for sanitised comments
#2578
base: main
Are you sure you want to change the base?
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 |
---|---|---|
|
@@ -42,6 +42,68 @@ public struct Trivia: Sendable { | |
pieces.isEmpty | ||
} | ||
|
||
/// The string contents of all the comment pieces with any comments tokens trimmed. | ||
/// | ||
/// Each element in the array is the trimmed contents of a line comment, or, in the case of a multi-line comment a trimmed, concatenated single string. | ||
public var commentValues: [String] { | ||
var comments = [String]() | ||
var partialComments = [String]() | ||
|
||
var foundStartOfCodeBlock = false | ||
var foundEndOfCodeBlock = false | ||
var isInCodeBlock: Bool { foundStartOfCodeBlock && !foundEndOfCodeBlock } | ||
|
||
for piece in pieces { | ||
switch piece { | ||
case .blockComment(let text), .docBlockComment(let text): | ||
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. If I’m not mistaking completely a |
||
let text = text.trimmingCharacters(in: "\n") | ||
|
||
foundStartOfCodeBlock = text.hasPrefix("/*") | ||
foundEndOfCodeBlock = text.hasSuffix("*/") | ||
|
||
let sanitized = | ||
text | ||
.split(separator: "\n") | ||
.map { $0.trimmingAnyCharacters(in: "/*").trimmingAnyCharacters(in: " ") } | ||
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 think this would also trim any slashes at the start of a line. And I don’t think we want that. Eg. in /*
/abc
*/ I would expect the comment value to be I would just trim the following:
|
||
.filter { !$0.isEmpty } | ||
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. Does this drop empty lines in the comment? Eg. in the following? Because I don’t think we want to. For example if you interpret the comment as markdown, empty lines have meaning. /*
Paragraph 1
Paragraph 2
*/ |
||
.joined(separator: " ") | ||
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. Similarly here, I don’t think we should concatenate lines using a space. Again, if interpreting a doc comment as markdown, the newlines have meaning. |
||
|
||
appendPartialCommentIfPossible(sanitized) | ||
|
||
case .lineComment(let text), .docLineComment(let text): | ||
if isInCodeBlock { | ||
appendPartialCommentIfPossible(text) | ||
} else { | ||
comments.append(String(text.trimmingPrefix("/ "))) | ||
} | ||
|
||
default: | ||
break | ||
} | ||
|
||
if foundEndOfCodeBlock, !partialComments.isEmpty { | ||
appendSubstringsToLines() | ||
partialComments.removeAll() | ||
} | ||
} | ||
|
||
if !partialComments.isEmpty { | ||
appendSubstringsToLines() | ||
} | ||
|
||
func appendPartialCommentIfPossible(_ text: String) { | ||
guard partialComments.isEmpty || !text.isEmpty else { return } | ||
|
||
partialComments.append(text) | ||
} | ||
|
||
func appendSubstringsToLines() { | ||
comments.append(partialComments.joined(separator: " ")) | ||
} | ||
|
||
return comments | ||
} | ||
|
||
/// The length of all the pieces in this ``Trivia``. | ||
public var sourceLength: SourceLength { | ||
return pieces.map({ $0.sourceLength }).reduce(.zero, +) | ||
|
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.
I think I would prefer if this returned a single string instead of an array of strings because I would expect that’s what most clients would want to work with. It’s always possible to create the array by splitting the string on
\n
.Could you also add an entry to
Release Notes/600.md
?