Skip to content

Improve symbol provider#622

Merged
DonJayamanne merged 2 commits intoDonJayamanne:masterfrom
patrys:improve-symbol-provider
Jan 16, 2017
Merged

Improve symbol provider#622
DonJayamanne merged 2 commits intoDonJayamanne:masterfrom
patrys:improve-symbol-provider

Conversation

@patrys
Copy link
Contributor

@patrys patrys commented Jan 5, 2017

  • Ignore symbols from external files (this confuses other extensions)
  • Provide parent scope name where available
  • Provide correct ranges for symbols (including entire scopes for classes, methods and functions - this makes the GitLens extension work much better)

}

public startKernel(kernelSpec: KernelspecMetadata, language: string): Promise<Kernel> {
public async startKernel(kernelSpec: KernelspecMetadata, language: string): Promise<Kernel> {
Copy link
Owner

Choose a reason for hiding this comment

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

Sorry still away on holidays. I can see that you have also attempted to clean up some of the code by replacing promises with async, in totally unrelated code. Wouldn't it be better to introduce such clean up in a separate PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It would! The thing is I started with changes to symbol provider but soon discovered that the test suite no longer passes even on master. I'll try to split my changes into two PRs.

Copy link
Owner

Choose a reason for hiding this comment

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

Yes, I need to fix some test issues, looks like the factoring isn't working anymore. Similar issues have Been reported by users too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've rebased this on top of #627 that fixes the tests but all of the changes live in commit 79f3368

@DonJayamanne DonJayamanne merged commit 48ea4e4 into DonJayamanne:master Jan 16, 2017
DonJayamanne added a commit that referenced this pull request Jan 25, 2018
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