Symbols using textDocument/documentSymbols only #6
Conversation
reader/unmarshal.go
Outdated
| Start _position `json:"start"` | ||
| End _position `json:"end"` | ||
| } | ||
| var payload Range |
There was a problem hiding this comment.
This may have been previously split for a performance optimization. I'll run this after a round of feedback to see if it still makes a big difference in reading.
There was a problem hiding this comment.
Ah got it, happy to switch back. Is there a benchmark we could add that tracks this?
There was a problem hiding this comment.
I went ahead and inlined the unmarshalling types in this function to try to save you some trouble. However, I wasn't sure how much of the performance benefit was due to inlining the types vs. not importing anything. For instance, I used an inline field type of Tags []protocol.SymbolTag instead of Tags []int, because the latter would have required creating a new slice and copying over the integer vales one by one. PTAL to see if this satisfies the concern.
I also added a test for unmarshalling a range with a tag (necessary now to test the conversion from inlined type value to exported type value).
symbol.go
Outdated
| } | ||
| } | ||
|
|
||
| type SymbolData struct { |
There was a problem hiding this comment.
Can we get comments on each of these fields? Looks like these are still non-standard.
There was a problem hiding this comment.
I'm also unsure what in the spec this corresponds to.
There was a problem hiding this comment.
Added a docstring. It's not an extension, it's just the symbol-related embedded fields of the RangeTag struct, which corresponds to the DeclarationTag described in the spec: https://microsoft.github.io/language-server-protocol/specifications/lsif/0.5.0/specification/#documentSymbol
There was a problem hiding this comment.
Looks like the Tags field is missing from the spec, though? Is that part a non-standard extension?
There was a problem hiding this comment.
Yes, I just updated the docstring to mention https://github.com/microsoft/language-server-protocol/issues/1209, which proposes adding SymbolTag[] as a field, so that it mirrors the information present in lsp.DocumentSymbol.
|
@efritz is there any other feedback I should address? |
|
closing as these changes were merged in sourcegraph/sourcegraph#18655 |
Subsumes #4. The main difference is that this PR no longer introduces a
workspace/symbolvertex and special symbol type that are outside the LSIF spec. It keeps the exported/unexported tags that do extend the LSIF spec.