feat: implement documentSymbol support for JSON objects and arrays - #64
feat: implement documentSymbol support for JSON objects and arrays#64Suyog241005 wants to merge 4 commits into
Conversation
79fca58 to
1acd7ab
Compare
jdesrosiers
left a comment
There was a problem hiding this comment.
I'm not 100% sure how this feature is supposed to work, but this is not what I expected for object properties. I expect that each property in an object would have two entries. The first would be the key and a SymbolKind.Property. The second would be the value and have a SymbolKind reflecting the type of the value.
| const jsonDocument = this.jsonDocuments.get(params.textDocument.uri); | ||
| if (!jsonDocument) { | ||
| return []; | ||
| } |
There was a problem hiding this comment.
We can assume that if the client send us this request, it's for a document that exists.
| private getDocumentSymbols(jsonDocument: JsonDocument): DocumentSymbol[] { | ||
| const ast = jsonDocument.findNodeAtPointer(""); | ||
| if (!ast) { | ||
| return []; | ||
| } | ||
|
|
||
| return this.collectDocumentSymbols(jsonDocument, ast); | ||
| } |
There was a problem hiding this comment.
Let's inline this. There's no reason for this to be a function.
| private collectDocumentSymbols(jsonDocument: JsonDocument, node: Node): DocumentSymbol[] { | ||
| const symbols: DocumentSymbol[] = []; | ||
|
|
||
| if (node.type === "object" && node.children) { |
There was a problem hiding this comment.
Objects always have children.
|
|
||
| if (node.type === "object" && node.children) { | ||
| for (const child of node.children) { | ||
| if (child.type === "property" && child.children) { |
There was a problem hiding this comment.
The children of object nodes are always property nodes and property nodes always have children.
| if (!keyNode) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
There will always be a key node.
| symbols.push(symbol); | ||
| } | ||
| } | ||
| } else if (node.type === "array" && node.children) { |
There was a problem hiding this comment.
Array nodes will always have children.
| default: | ||
| return SymbolKind.Property; |
There was a problem hiding this comment.
This technically works because the only other possible result you could get is "property", but it's not very intentional.
There was a problem hiding this comment.
Will add explicit case "property": return SymbolKind.Property; to make all switch branches intentional
1acd7ab to
d4ce8e8
Compare
|
Hi @jdesrosiers! Updated the PR to address all comments. Regarding object properties: I double-checked standard LSP and VS Code JSON Language Service (
On |
jdesrosiers
left a comment
There was a problem hiding this comment.
You still haven't addressed,
I'm not 100% sure how this feature is supposed to work, but this is not what I expected for object properties. I expect that each property in an object would have two entries. The first would be the key and a
SymbolKind.Property. The second would be the value and have aSymbolKindreflecting the type of the value.
d066393 to
baa0afb
Compare
|
Hi @jdesrosiers! Updated |
Preference has nothing to do with it. Like I said, I'm not confident that I know how it's supposed to work. I expect you to do the research and tell me what you learned. I need to know that you actually thought about this and you're not just doing what you think I want. |
|
Hi @jdesrosiers! Apologies for the confusion, when I said "as per your preference", I shouldn't have temporarily switched the code to 2 entries just because you brought it up! My original research stands: 1 entry per property ( My primary approach from the start (which I mentioned in my previous comment below) remains based on this standard:
This provides the cleanest UX by giving clear breadcrumbs ( |
Implements LSP Document Symbol support to provide a hierarchical document outline for JSON files in VS Code's Outline panel and top breadcrumb navigation.
Fixes #49
Changes
DocumentSymbols.ts: Recursively builds symbol trees for JSON objects, arrays, properties, and primitive values, mappingSymbolKindby value type.DocumentSymbols.test.ts: Tests covering flat objects, nested objects, arrays, and empty