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

Let documentSymbol return the correct results when mergeable elements are used #77

Conversation

keyboardDrummer
Copy link
Contributor

@keyboardDrummer keyboardDrummer commented Sep 20, 2018

Previously, the documentSymbol results would grow exponentially in size when using multiple elements with the same name such as in some of the examples for declaration merging.

Note that this implementation, although in line with the existing implementation, is different from what VSCode does. An example, given the code:

interface Box {
    height: number;
    width: number;
}

interface Box {
    scale: number;
}

VSCode will show:

Box
  height
  width
  scale

While this implementation shows:

Box
  height
  width
Box
  scale

@gitpod-io-legacy-app
Copy link

Open in Gitpod - starts a development workspace for this pull request in code review mode and opens it in a browser IDE.

@akosyakov
Copy link
Contributor

Can you elaborate what was the issue? I cannot reproduce it.

I've tried the master state and latest VS Code and they both show:

Box
  height
  width
Box
  scale

screen shot 2018-09-20 at 17 59 23

master_document_symbols

@keyboardDrummer
Copy link
Contributor Author

keyboardDrummer commented Sep 20, 2018

Hmm. Curious that both VSCode and.. Theia? still produce correct behavior. However, running the testcases added in the PR in the master code shows that this LSP server had incorrect behavior. You will see that:

interface Box {
    height: number;
    width: number;
}

interface Box {
    scale: number;
}

Returns something like

Box
  height
  width
  scale
Box
  height
  width
  scale

And

interface Box {
  width: number
}
interface Box {
  width: number
}

Returns

Box
  width
  width
Box
  width
  width

Copy link
Contributor

@akosyakov akosyakov left a comment

Choose a reason for hiding this comment

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

@keyboardDrummer thanks for taking time to check, indeed I was able to reproduce it by reverting changes to document-symbol and running new tests

@@ -40,7 +48,7 @@ export function collectDocumentSymbols(parent: tsp.NavigationTree, symbols: lsp.
}

export function collectSymbolInformations(uri: string, current: tsp.NavigationTree, symbols: lsp.SymbolInformation[], containerName?: string): boolean {
Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably do the same here for clients who cannot handle hierarchical symbols, can be a separate PR.

@akosyakov akosyakov merged commit c63307d into typescript-language-server:master Sep 21, 2018
@keyboardDrummer keyboardDrummer deleted the typeFoxDocumentSymbol branch September 21, 2018 08:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants