Please add a `TriviaPiece.commentValue` property

[grynspan]

Description

There are four kinds of comment tracked by TriviaPiece. It'd be a nice convenience to be able to extract the string values of those comments. May I suggest something like:

extension TriviaPiece {
  public var commentValue: String? { get }
}

[ahoppen]

Tracked in Apple’s issue tracker as rdar://111922531

[Matejkob]

Using commentValue do we wanna extract the whole string from these four kinds, or only the documentation content? I mean for the following case:

let triviaPiece = TriviaPiece.docLineComment("/// Text")

commentValue should return /// Text or just Text?

[grynspan]

Well, the latter case gets hard for multi-line comments. What's the content of:

/**
 * Hello, folks!
 * I am a banana!
 */

I think just returning the string verbatim is fine—anybody who cares about processing it can do so. Or, if it is easy enough, you could provide rawCommentValue and commentValue, the latter doing the processing?

[ahoppen]

I think stripping off the /// makes sense because if you wanted to get the entire text of the TriviaPiece you could do Trivia(pieces: [piece]).description (and we could even add description to TriviaPiece but I’m not sure if that’s super useful).

Now the question is: What should we do with the space after ///? My proposal is the following: We implement commentValue on Trivia instead of TriviaPiece. What it will do is the following: Check if all line comments have a space right after // or ///. If that’s the case, also remove that space from the commentValue. If not, we only remove the // or ///.

We could do the same for block comments and use the same logic whether to remove a space after /* or before */.

[Matejkob]

I think stripping off the /// makes sense because if you wanted to get the entire text of the TriviaPiece you could do Trivia(pieces: [piece]).description (and we could even add description to TriviaPiece but I’m not sure if that’s super useful).

Now the question is: What should we do with the space after ///? My proposal is the following: We implement commentValue on Trivia instead of TriviaPiece. What it will do is the following: Check if all line comments have a space right after // or ///. If that’s the case, also remove that space from the commentValue. If not, we only remove the // or ///.

We could do the same for block comments and use the same logic whether to remove a space after /* or before */.

Sounds good to me. I had the same idea in mind.

I can try implement it 🤗

[Matejkob]

I think stripping off the /// makes sense because if you wanted to get the entire text of the TriviaPiece you could do Trivia(pieces: [piece]).description (and we could even add description to TriviaPiece but I’m not sure if that’s super useful).

Now the question is: What should we do with the space after ///? My proposal is the following: We implement commentValue on Trivia instead of TriviaPiece. What it will do is the following: Check if all line comments have a space right after // or ///. If that’s the case, also remove that space from the commentValue. If not, we only remove the // or ///.

We could do the same for block comments and use the same logic whether to remove a space after /* or before */.

One more edge case jumped into my mind. Do we care about multi comments in one Trivia? What do wanna achieve for such a case:

Trivia(
  pieces: [
    .docLineComment("/// docLineComment_1.1"),
    .docLineComment("/// docLineComment_1.2"),
    .newlines(2),
    .docBlockComment("""
    /**
      docBlockComment_1.1
      docBlockComment_1.2
    /*
    """),
    .docLineComment("/// docLineComment_2.1"),
    .docLineComment("/// docLineComment_2.2"),
  ]
)

[grynspan]

Well, I expect these properties to be on TriviaPiece, not Trivia, so each piece has its own comment value. I'm explicitly asking for a convenience on TriviaPiece here because it has four cases that carry equivalent data. I don't think it's necessary to overdesign a solution.

[grynspan]

We could have the convenience property on TriviaPiece, and then something "smarter" on Trivia that does the sort of work you're describing.

[allevato]

FWIW, I just implemented logic in swift-format that tries to mimic what the compiler's Markup component does to extract doc comments from Trivia, which includes handling multiple adjacent pieces and stripping leading * off block comment lines: https://github.com/apple/swift-format/blob/main/Sources/SwiftFormatCore/DocumentationCommentText.swift

swift-syntax would be a better home for this, especially since I imagine it'll happen eventually anyway as more of the compiler gets moved from C++ to Swift.

[adammcarter]

Hi all, is this issue still up for grabs? 👨🏻‍💻

@allevato sounds like you’ve done some great work with Swift format related to this. Do you have plans to bring it in to this issue? Or am I good to look at this?

[ahoppen]

Hi all, is this issue still up for grabs? 👨🏻‍💻

Yes, it is.

[allevato]

@allevato sounds like you’ve done some great work with Swift format related to this. Do you have plans to bring it in to this issue? Or am I good to look at this?

I personally don't have the cycles right now, but if you are interested in using the DocumentationCommentText logic in swift-format as a starting point to provide this in swift-syntax, I'd be supportive of it.

[adammcarter]

Thanks both!

@ahoppen Are you able to assign me please?

[adammcarter]

Hi all, I'm looking at this issue now and got a nice basic implementation working but would love some input on a particular use case.

For the source code below, what would you expect the output to be of trivia.commentValues?

/*
 * Hello, folks!
 * // Apples and oranges
 * I am a banana!
 */

I have an idea of what I'd expect (and have implemented it that way at the moment) but would be good to get other's opinions before I move further.

(Equally this would apply for doc comment blocks containing a doc comment line too or a variant of the two)

Option A 🍎

["Hello, folks! Apples and oranges I am a banana!"]

Option B 🍌

["Hello, folks! // Apples and oranges I am a banana!"]

Option C 🥕

["Hello, folks!", "Apples and oranges", "I am a banana!"]

Option D 🍊

["Hello, folks! I am a banana!", "Apples and oranges"]

Option E 🤔

Or something else entirely?

[allevato]

I would expect either ["Hello, folks!", "// Apples and oranges", "I am a banana!"] or "Hello, folks!\n// Apples and oranges\nI am a banana!", depending on whether you're returning an array of lines or a newline-delimited string. The prefix that gets stripped off the comment would be determined solely by the opening delimiter of the comment and you wouldn't attempt to process other things that look like comments inside of one.

[adammcarter]

you wouldn't attempt to process other things that look like comments inside of one

Agreed! I'd expect option B from the examples above if I were to access that value so I'll go down that path. Thanks!

[zyadtaha]

Hi @DougGregor @ahoppen, I'm interested in this issue, is it still open? Also, do you have any other recommendations for good first issues?

[DougGregor]

@zyadtaha it doesn't look like anyone is actively working on it. The draft PR is almost a year old and hasn't received any updates based on the code review in the pull request.

[adammcarter]

@zyadtaha Feel free to take over my draft PR and address the issues on there if you'd like.

Happy to help out if you have any questions.