-
Notifications
You must be signed in to change notification settings - Fork 73
feat: enable inline project context in suggestion requests #983
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
Conversation
d5ccc9a
to
d06dee8
Compare
} | ||
|
||
const controller = LocalProjectContextController.getInstance() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we ever setInstance()
? I couldn't find any code initializing the singleton.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I found it :)
language-servers/server/aws-lsp-codewhisperer/src/shared/localProjectContextController.ts
Line 54 in 12dc8d7
LocalProjectContextController.instance = this |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The instance should be initialized on start up, but I should defensively check the instance exists for extra safety in case of failures. I'll make this change.
const supplementalContextConfig = getSupplementalContextConfig(document.languageId) | ||
|
||
if (supplementalContextConfig === undefined) { | ||
return supplementalContextConfig |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this intentionally returning undefined
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Undefined is indeed intentional here. The caller of this method returns undefined as well if there is no context result.
return supplementalContextConfig | ||
} | ||
|
||
if (supplementalContextConfig === 'OpenTabs_BM25') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This never happens? See
return isCrossFileSupported(languageId) ? 'codemap' : undefined
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, getSupplementalContextConfig
should be enhanced to return different strategies once the others are implemented (others are currently no-ops), which is what this conditional was targeting. I'll remove as its unreachable in the short term.
d06dee8
to
4e881e2
Compare
)" This reverts commit 501d3fe.
Problem
Supplemental Context included in suggestions requests currently does not include inline project context
Solution
Port over vscode toolkit implementation to include project context in supplemental context sent in suggestions request. Leverages vector library and local project context addition to the language server component.
Note: This code is reliant on PR #982 re-enabling the localProjectContextController's ability to find the vector library.
License
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.