Improve lsp hover definition comment formatting #3411
Conversation
hirasawayuki
changed the title
hirasawayuki
changed the title
hirasawayuki
force-pushed
the
feat/format-comment-docs
branch
from
18a54b8
to
20f7a11
Compare
private/buf/buflsp/symbol.go
Outdated
| // 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 { |
There was a problem hiding this comment.
I think a better name for this is to rename it to something like commentToMarkdown, since the important thing is that this is emitting a Markdown string.
private/buf/buflsp/symbol.go
Outdated
| lines := strings.Split(strings.TrimSpace(comment), "\n") | ||
| for i, line := range lines { | ||
| line = strings.TrimSpace(line) | ||
| line = strings.TrimLeft(line, "*") | ||
| line = strings.TrimPrefix(line, " ") | ||
| lines[i] = line | ||
| } |
There was a problem hiding this comment.
I would prefer if this only triggered if the comment starts with /** Otherwise, the following will be mangled:
/*
my things:
* foo
* bar
* baz
*/There was a problem hiding this comment.
I would also like to see a comment that explains that what this is processing is a Doxygen-style comment, with an inline example of what we expect the comment to look like.
private/buf/buflsp/symbol.go
Outdated
|
|
||
| if strings.HasPrefix(comment, "/*") && strings.HasSuffix(comment, "*/") { | ||
| comment = strings.Trim(comment, "/*") | ||
| // single-line |
There was a problem hiding this comment.
Comments should be complete sentences.
private/buf/buflsp/symbol_test.go
Outdated
| "github.com/stretchr/testify/assert" | ||
| ) | ||
|
|
||
| func Test_formatComment(t *testing.T) { |
There was a problem hiding this comment.
Don't use underscores in test names. Just name it TestFormatComment or equivalent.
| @@ -0,0 +1,58 @@ | |||
| package buflsp | |||
private/buf/buflsp/symbol_test.go
Outdated
| }, | ||
| { | ||
| name: "Multi-line comment with mixed indentation", | ||
| input: "/*\n * First line\n * - Second line\n * - Third line\n */", |
There was a problem hiding this comment.
This test case is incorrect. You should not trim asterisks if the comment does not begin with /**. Alternatively, you could instead only trim asterisks if every line starts with one.
| }, | ||
| { | ||
| name: "Empty comment", | ||
| input: "/**/", |
There was a problem hiding this comment.
Can you add a test case for there being a space in this?
private/buf/buflsp/symbol_test.go
Outdated
| expected string | ||
| }{ | ||
| { | ||
| name: "Single-line comment", |
There was a problem hiding this comment.
can you make the test names be all-lowercase, separated by dashes?
private/buf/buflsp/symbol_test.go
Outdated
| }, | ||
| } | ||
|
|
||
| for _, tt := range tests { |
private/buf/buflsp/symbol_test.go
Outdated
| result := formatComment(tt.input) | ||
| if result != tt.expected { | ||
| assert.Equal(t, tt.input, result) | ||
| } |
There was a problem hiding this comment.
Rewrite this to
assert.Equal(t, text.expected, formatComment(test.input))
|
@mcy |
This PR introduces a new
formatCommentfunction to enhance the formatting of comments displayed in hover definitions. The implementation handles both single-line and multi-line comments, ensuring proper formatting while maintaining Markdown compatibility.Before
As shown above, hover definitions currently display unnecessary asterisks from the proto file's comment syntax, making the documentation harder to read.
After
The new implementation removes the redundant asterisks while preserving the comment's structure, resulting in cleaner and more readable documentation in hover definitions.