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

Small improvements to `workspace/symbol` #5443

Merged
merged 7 commits into from Nov 30, 2018

Conversation

Projects
None yet
2 participants
@Duhemm
Copy link
Contributor

Duhemm commented Nov 14, 2018

  • Inspect only source trees
  • Exclude local symbols

@Duhemm Duhemm added the area:ide label Nov 14, 2018

Duhemm added some commits Nov 14, 2018

Look only in source trees in `workspace/symbol`
We were inspecting all trees on the classpath for every project, which
was not efficient and lead to duplicate results. This commit changes the
implementation so that the language server inspects only the source
trees of every project.

@Duhemm Duhemm force-pushed the dotty-staging:ide-workspace-symbols branch from ca776a7 to 7fd0c1d Nov 20, 2018

@Duhemm

This comment has been minimized.

Copy link
Contributor

Duhemm commented Nov 20, 2018

Rebased

@Duhemm

This comment has been minimized.

Copy link
Contributor

Duhemm commented Nov 28, 2018

@smarter can you take a look?

@Duhemm Duhemm requested a review from smarter Nov 28, 2018

@@ -466,12 +471,14 @@ class DottyLanguageServer extends LanguageServer

override def symbol(params: WorkspaceSymbolParams) = computeAsync { cancelToken =>
val query = params.getQuery
def predicate(implicit ctx: Context): NameTree => Boolean =
tree => tree.symbol.exists && !tree.symbol.isLocal && tree.name.toString.contains(query)

This comment has been minimized.

@smarter

smarter Nov 28, 2018

Member

Shouldn't symbol and documentSymbol have similar predicates ? I.e., should we also exclude primary constructors and synthetic symbols here ?

This comment has been minimized.

@Duhemm

Duhemm Nov 29, 2018

Contributor

Yes, that makes a lot of sense. I've removed checks that were redundant with those happening in namedTrees.

I'm also thinking that namedTrees should be the one excluding primary constructors. I can't think of situations where they should be included, but I may very well be missing something. What do you think?

Simplify predicates in `symbol`, `documentSymbol`
Some checks were redundant with the checks that are performed in
`namedTrees`, and can be removed. Also, exclude primary constructors in
`symbol`.
@smarter

This comment has been minimized.

Copy link
Member

smarter commented Nov 29, 2018

Agreed, as long as go to definition on a new call still goes to the correct place

Exclude primary constructors in `namedTrees`
We never want to match them, so it's easier to exclude them here rather
than always add a check in the predicate function.
Add `Include.local`
This include flag means that local symbols and trees should be
inspected.

@Duhemm Duhemm force-pushed the dotty-staging:ide-workspace-symbols branch from 6f141db to 8213555 Nov 29, 2018

@Duhemm

This comment has been minimized.

Copy link
Contributor

Duhemm commented Nov 29, 2018

_ => true could be the default value of this parameter

👍

@Duhemm Duhemm merged commit 2b84d7c into lampepfl:master Nov 30, 2018

3 checks passed

CLA User signed CLA
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/drone/pr the build was successful
Details

@Duhemm Duhemm deleted the dotty-staging:ide-workspace-symbols branch Nov 30, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment