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

Improve lsp hover definition comment formatting #3411

Merged
merged 3 commits into from
Dec 16, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
49 changes: 36 additions & 13 deletions private/buf/buflsp/symbol.go
Original file line number Diff line number Diff line change
Expand Up @@ -493,24 +493,12 @@ func (s *symbol) FormatDocs(ctx context.Context) string {
var printed bool
for _, comments := range allComments {
for i := 0; i < comments.Len(); i++ {
comment := comments.Index(i).RawText()

// The compiler does not currently provide comments without their
// delimited removed, so we have to do this ourselves.
if strings.HasPrefix(comment, "//") {
// NOTE: We do not trim the space here, because indentation is
// significant for Markdown code fences, and if every line
// starts with a space, Markdown will trim it for us, even off
// of code blocks.
comment = strings.TrimPrefix(comment, "//")
} else {
comment = strings.TrimSuffix(strings.TrimPrefix(comment, "/*"), "*/")
}

comment := formatComment(comments.Index(i).RawText())
if comment != "" {
printed = true
}

// No need to process Markdown in comment; this Just Works!
fmt.Fprintln(&tooltip, comment)
}
Expand All @@ -523,6 +511,41 @@ func (s *symbol) FormatDocs(ctx context.Context) string {
return tooltip.String()
}

// formatComment takes a raw comment string and formats it by removing comment
// delimiters and unnecessary whitespace. It handles both single-line (//) and
// multi-line (/* */) comments.
func formatComment(comment string) string {
hirasawayuki marked this conversation as resolved.
Show resolved Hide resolved
comment = strings.TrimSpace(comment)

if strings.HasPrefix(comment, "//") {
// NOTE: We do not trim the space here, because indentation is
// significant for Markdown code fences, and if every line
// starts with a space, Markdown will trim it for us, even off
// of code blocks.
return strings.TrimPrefix(comment, "//")
}

if strings.HasPrefix(comment, "/*") && strings.HasSuffix(comment, "*/") {
comment = strings.Trim(comment, "/*")
Copy link
Member

Choose a reason for hiding this comment

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

This will mangle some comments, such as /*This is *important**/. Please include a test case to handle this.

// single-line
hirasawayuki marked this conversation as resolved.
Show resolved Hide resolved
if !strings.Contains(comment, "\n") {
return strings.TrimSpace(comment)
}

lines := strings.Split(strings.TrimSpace(comment), "\n")
for i, line := range lines {
line = strings.TrimSpace(line)
line = strings.TrimLeft(line, "*")
Copy link
Member

Choose a reason for hiding this comment

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

This should be TrimPrefix.

line = strings.TrimPrefix(line, " ")
Copy link
Member

Choose a reason for hiding this comment

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

This seems incorrect for the same reason as stated in my //NOTE comment.

lines[i] = line
}
hirasawayuki marked this conversation as resolved.
Show resolved Hide resolved

return strings.Join(lines, "\n")
}

return comment
}

// symbolWalker is an AST walker that generates the symbol table for a file in IndexSymbols().
type symbolWalker struct {
file *file
Expand Down
58 changes: 58 additions & 0 deletions private/buf/buflsp/symbol_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
package buflsp
hirasawayuki marked this conversation as resolved.
Show resolved Hide resolved

import (
"testing"

"github.com/stretchr/testify/assert"
)

func Test_formatComment(t *testing.T) {
hirasawayuki marked this conversation as resolved.
Show resolved Hide resolved
t.Parallel()

tests := []struct {
name string
input string
expected string
}{
{
name: "Single-line comment",
hirasawayuki marked this conversation as resolved.
Show resolved Hide resolved
input: "// This is a single-line comment",
expected: " This is a single-line comment",
},
{
name: "Multi-line comment",
input: "/*\n This is a\n multi-line comment\n */",
Copy link
Member

Choose a reason for hiding this comment

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

Use go raw strings `xyz` instead, to make this easier to read.

expected: "This is a\nmulti-line comment",
},
{
name: "Multi-line comment with mixed indentation",
input: "/*\n * First line\n * - Second line\n * - Third line\n */",
hirasawayuki marked this conversation as resolved.
Show resolved Hide resolved
expected: "First line\n- Second line\n - Third line",
},
{
name: "Multi-line comment with JavaDoc convention",
input: "/** This is a\n * multi-line comment\n * with multi-asterisks */",
expected: "This is a\nmulti-line comment\nwith multi-asterisks",
},
{
name: "Single-line multi-line comment",
input: "/* Single-line multi-line comment */",
expected: "Single-line multi-line comment",
},
{
name: "Empty comment",
input: "/**/",
hirasawayuki marked this conversation as resolved.
Show resolved Hide resolved
expected: "",
},
}

for _, tt := range tests {
hirasawayuki marked this conversation as resolved.
Show resolved Hide resolved
t.Run(tt.name, func(t *testing.T) {
t.Parallel()
result := formatComment(tt.input)
if result != tt.expected {
assert.Equal(t, tt.input, result)
}
hirasawayuki marked this conversation as resolved.
Show resolved Hide resolved
})
}
}