DRAFT - Add Trivia.commentValues for sanitised comments
#2578
Conversation
This holds an array of comments made from the trivia's pieces, with one line per element of the array
adammcarter
force-pushed
the
adammcarter/trivia-piece-comment-value
branch
2 times, most recently
from
b6d106d
to
4844ec7
Compare
Blocks of comments are now concatenated with their related lines within the same block comment and added to the commentValues property as one stripped, concatenated string If there are n code blocks, there will be n stripped, concatenated elements in the commentValues (plus x line comments)
adammcarter
force-pushed
the
adammcarter/trivia-piece-comment-value
branch
from
4844ec7
to
e8cd9f9
Compare
This also keeps comment lines within comment blocks with their leading comment markers in tact
adammcarter
force-pushed
the
adammcarter/trivia-piece-comment-value
branch
from
e5718e0
to
36df0fc
Compare
There was a problem hiding this comment.
Thanks for setting up the PR @adammcarter. I have some initial review comments inline.
| /// 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] { |
There was a problem hiding this comment.
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?
|
|
||
| for piece in pieces { | ||
| switch piece { | ||
| case .blockComment(let text), .docBlockComment(let text): |
There was a problem hiding this comment.
If I’m not mistaking completely a blockComment and docBlockComment will always span from the opening /* to the closing */. So I don’t think we need all the foundStartOfCodeBlock/foundEndOfCodeBlock/isInCodeBlock handling.
| /** | ||
| * Some doc block comment | ||
| * // spread on many lines | ||
| * with a line comment | ||
| */ | ||
| """ |
There was a problem hiding this comment.
I think it’s quite common for the * to be indented by a single space to align with the first * of /*. Could you also add a test case for something like
/**
* Some doc block comment
* with a line comment
*/
"""
| let sanitized = | ||
| text | ||
| .split(separator: "\n") | ||
| .map { $0.trimmingAnyCharacters(in: "/*").trimmingAnyCharacters(in: " ") } |
There was a problem hiding this comment.
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:
- At the start of the comment:
/*or/** - Inside the block comment: Any number of spaces followed by an optional
* - At the end of the comment:
*/
| text | ||
| .split(separator: "\n") | ||
| .map { $0.trimmingAnyCharacters(in: "/*").trimmingAnyCharacters(in: " ") } | ||
| .filter { !$0.isEmpty } |
There was a problem hiding this comment.
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
*/| .split(separator: "\n") | ||
| .map { $0.trimmingAnyCharacters(in: "/*").trimmingAnyCharacters(in: " ") } | ||
| .filter { !$0.isEmpty } | ||
| .joined(separator: " ") |
There was a problem hiding this comment.
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.
| @@ -69,4 +69,325 @@ class TriviaTests: XCTestCase { | |||
| XCTAssertNotEqual(TriviaPiece.unexpectedText("e"), .unexpectedText("f")) | |||
| XCTAssertNotEqual(TriviaPiece.unexpectedText("e"), .lineComment("e")) | |||
| } | |||
|
|
|||
| func testTriviaCommentValues() { | |||
There was a problem hiding this comment.
I think it’s fine if we tailor commentValue to the kind of trivia that the parser produces. If we do that we can construct the Trivia from a string literal and have something like the following:
func assertCommentValue(
_ trivia: Trivia,
commentValue: String,
file: StaticString = #filePath,
line: UInt = #line
) {
XCTAssertEqual(trivia.commentValue, commentValue, file: file, line: line)
}
func testExample() {
assertCommentValue(
"// line comment",
commentValue: "line comment"
)
}
Motivation
Original issue here: #1890
Currently when pulling comments out of
TriviaorTriviaPieces we get the comment delimiters such as//,/* */etc. then we have to manually remove these to get the underlying contents of the comments.Additionally, when extracting multiple
TriviaPieces we might want to then concatenate the values in to one string, stripping any new lines too.Proposed solution
As proposed in the original issue, this PR introduces a
commentValuesproperty onTriviawhich does all of the above for us to produce a nice looking comment string, regardless of whether we span multiple lines orTriviaPiecesThere are some edge cases where, for example you might have...
*on new lines in between start/end delimiters. Here we strip the delimiters and new lines to concatenate the string//within the inner comment and just concatenate the comment within the block commentFor other real life use cases they should be covered in the unit tests inside of
TriviaTests.swift, but if there's any I've missed that you can think of feel free to let me know and I can add them in too