Port isInComment
#1397
Port isInComment
#1397
Conversation
| // `precedingToken` and `tokenAtPosition`. | ||
| // It is the caller's responsibility to call `astnav.GetTokenAtPosition` to compute a default `tokenAtPosition`, | ||
| // or `astnav.FindPrecedingToken` to compute a default `precedingToken`. | ||
| func getRangeOfEnclosingComment( |
There was a problem hiding this comment.
This function in Strada used to differentiate between a null precedingToken (which would be left undefined) and an undefined precedingToken (which would get replaced with a default computed value). We can't easily do this in Go, so, I'm simply not doing anything to the parameters here.
There was a problem hiding this comment.
Maybe a separate shouldFindPrecedingToken bool?
There was a problem hiding this comment.
I kinda dislike the original behavior, and there was only one place which needed it in Strada, so I'd rather leave it like this.
| // `precedingToken` and `tokenAtPosition`. | ||
| // It is the caller's responsibility to call `astnav.GetTokenAtPosition` to compute a default `tokenAtPosition`, | ||
| // or `astnav.FindPrecedingToken` to compute a default `precedingToken`. | ||
| func getRangeOfEnclosingComment( |
There was a problem hiding this comment.
Maybe a separate shouldFindPrecedingToken bool?
internal/ls/format.go
Outdated
| tokenAtPosition *ast.Node, | ||
| ) *ast.CommentRange { | ||
| jsdoc := ast.FindAncestor(tokenAtPosition, func(n *ast.Node) bool { | ||
| return n.Kind == ast.KindJSDoc |
There was a problem hiding this comment.
We're probably missing an IsJSDoc method, which you could just pass here as (*ast.Node).IsJSDoc`
There was a problem hiding this comment.
I could have sworn we had that already...
There was a problem hiding this comment.
I thought we could be avoiding adding it on purpose, to avoid confusion with isJSDocNode, but I'll add it.
| if node.Kind == ast.KindJsxText { | ||
| return nil | ||
| } | ||
| return scanner.GetLeadingCommentRanges(&ast.NodeFactory{}, file.Text(), node.Pos()) |
There was a problem hiding this comment.
It's so weird that this needs a factory because we only ever really need to look at a single comment range at a time.
| @@ -11,7 +11,7 @@ import ( | |||
|
|
|||
| func TestCompletionsJSDocNoCrash3(t *testing.T) { | |||
| t.Parallel() | |||
|
|
|||
| t.Skip() | |||
There was a problem hiding this comment.
do we have an easy way to eyeball these Skip tests for the future? Do we just assume every Skip in fourslash is a bug?
There was a problem hiding this comment.
Some are bugs, some are missing features, e.g. JSDoc tests. The way to find out why they fail is to re-generate the tests without Skip and look at the error messages.
Co-authored-by: Daniel Rosenwasser <[email protected]>
Co-authored-by: Daniel Rosenwasser <[email protected]>
|
Hopefully this will be the last reapproval needed. 🙁 |
Port
isInComment/getRangeOfEnclosingComment.This fixes some completion tests, but some tests now fail because JSDoc completions are unimplemented, and we always return
nilfor completions in a comment position.