Skip to content
This repository has been archived by the owner on Feb 22, 2020. It is now read-only.

Activation refactor #117

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from
Draft

Activation refactor #117

wants to merge 5 commits into from

Conversation

efritz
Copy link
Contributor

@efritz efritz commented Jan 24, 2020

This PR cleans up the init code after sourcegraph/code-intel-extensions#198.

This removes all LSIF and search-based logic and instead delegates to the activateCodeIntel function, passing it only the language-server specific logic.

This PR will be followed by a major cleanup.

@efritz efritz force-pushed the activation-refactor branch from 5345af5 to 2a6a60e Compare January 27, 2020 15:08
@efritz efritz force-pushed the activation-refactor branch from 2a6a60e to bd9208b Compare January 27, 2020 16:30
@@ -1,7 +1,7 @@
{
"extends": "./node_modules/@sourcegraph/tsconfig/tsconfig.json",
"compilerOptions": {
"target": "es2016",
"target": "es2019",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need async generators now.

src/lang-go.ts Outdated
doc,
pos,
sendRequest,
}: {
doc: sourcegraph.TextDocument
pos: sourcegraph.Position
sendRequest: SendRequest<any>
}): Observable<lspext.Xreference & { currentDocURI: string }> {
}): AsyncGenerator<lspext.Xreference & { currentDocURI: string }> {
Copy link
Contributor Author

@efritz efritz Jan 27, 2020

Choose a reason for hiding this comment

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

Return async generator instead of observable so we don't have to go from observable -> async generator -> observable later in the code.

src/lang-go.ts Outdated
definition: () => Promise.resolve(undefined),
references: () => Promise.resolve(undefined),
}
// interface MaybeProviders {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will clean up this and additional dead code in a cleanup PR.

Copy link
Contributor

@chrismwendt chrismwendt left a comment

Choose a reason for hiding this comment

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

I verified the following behavior remains unchanged:

  • With go.serverUrl set
    • On a repo missing LSIF
      • Hover: LSIF fails, falls back to LSP
      • Definition: LSIF fails, falls back to LSP
      • References: LSP is used
    • On a repo with LSIF
      • Hover: LSIF succeeds
      • Definition: LSIF succeeds
      • References: LSP is used
  • With go.serverUrl not set: same as Activation refactor code-intel-extensions#198 (review)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants