Create Q Customization Server #462
Conversation
| lsp.extensions.onGetConfigurationFromServer( | ||
| async (params: GetConfigurationFromServerParams, token: CancellationToken) => { | ||
| if (params.section === Q_CUSTOMIZATION_CONFIGURATION_SECTION) { | ||
| const customizations = (await codeWhispererService.listAvailableCustomizations({ maxResults: 10 })) |
There was a problem hiding this comment.
Why we limit to 10 here? Can we return all and let client decide how many to display?
| @@ -260,18 +261,6 @@ const mergeSuggestionsWithRightContext = ( | |||
| }) | |||
| } | |||
|
|
|||
| // Checks if any suggestion in list of suggestions matches with left context of the file | |||
| const hasLeftContextMatch = (suggestions: Suggestion[], leftFileContent: string): boolean => { | |||
There was a problem hiding this comment.
Did you intentionally remove this function?
There was a problem hiding this comment.
Yes, it was unused
| import { getUserAgent } from '../utils' | ||
|
|
||
| // The configuration section that the customization server will register and listen to | ||
| export const Q_CUSTOMIZATION_CONFIGURATION_SECTION = 'aws.q.customization' |
There was a problem hiding this comment.
How many levels of nested sections should we allow servers to register and handle?
Registering this server to 3 levels deep section seems to be too granular - what if we need to fetch other more configs, other that customizations? Will we have to register another aws.q.$sectionname section for this server?
Can we instead make this more generic amazonQConfigurationServer, and let it handle whole aws.q section of configs? Then it can be extended easier in the future without much refactoring.
Additionally, we can still allow clients to request more detailed section, like aws.q.customization and return filtered object for only requested subsection. This filtering can be done in the runtime proxying getConfigurationFromServer request, so logic is uniform for all servers.
ege0zcan
force-pushed
the
qCustomization
branch
from
36294db to
07ff051
Compare
| .catch(reason => logging.log(`Error in GetConfiguration: ${reason}`)) | ||
| const updateConfiguration = async () => { | ||
| try { | ||
| const qConfig = await lsp.workspace.getConfiguration(Q_CONFIGURATION_SECTION) |
There was a problem hiding this comment.
I think here we just need to make sure we send the request with the correct parameters. According to LSP spec, https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#configurationParams an array of items has to be sent.
There was a problem hiding this comment.
I checked locally and it seems internally it somehow sends the request in the expected correct format. Can ignore this comment.
There was a problem hiding this comment.
FYI, under the hood getConfiguration is handled by this implementation, which converts arguments to LSP params
https://github.com/microsoft/vscode-languageserver-node/blob/f58f4dff16ad2760028bb1cb95e882de30a1000f/server/src/common/configuration.ts#L4
There was a problem hiding this comment.
thanks for sharing the link Viktor.
server/aws-lsp-codewhisperer/src/language-server/codeWhispererServer.ts
Outdated
Show resolved
Hide resolved
| logging.log(`Inline completion configuration updated to use ${qConfig.customization}`) | ||
| } | ||
|
|
||
| const config = await lsp.workspace.getConfiguration('aws.codeWhisperer') |
There was a problem hiding this comment.
nit: it should be possible to request multiple configuration sections with one getConfiguration call, instead of 2 https://github.com/microsoft/vscode-languageserver-node/blob/f58f4dff16ad2760028bb1cb95e882de30a1000f/server/src/common/configuration.ts#L18.
Our Workspace.getConfiguration proxy handler only supports section though, so it will need to be extended. Could be done as a follow-up.
server/aws-lsp-codewhisperer/src/language-server/codeWhispererService.ts
Outdated
Show resolved
Hide resolved
| import { getUserAgent } from '../utils' | ||
|
|
||
| // The configuration section that the server will register and listen to | ||
| export const Q_CONFIGURATION_SECTION = 'aws.q' |
There was a problem hiding this comment.
Could you add documentation for QConfigurationServerToken to README with an configuration key it supports and data type of response it provides?
Also you can define a Typescript type for onGetConfigurationFromServer response, and it will serve as a documentation as well.
| @@ -330,6 +339,18 @@ export class ChatController implements ChatHandlers { | |||
| return chatEventParser.getChatResult() | |||
| } | |||
|
|
|||
| updateConfiguration = async () => { | |||
| try { | |||
| const qCustomizationConfig = await this.#features.lsp.workspace.getConfiguration(Q_CONFIGURATION_SECTION) | |||
There was a problem hiding this comment.
The Typescript type for expected configuration object defined in #462 (comment) can be used in calls to read documentation like here.
Description
updateConfigurationlogic in the inline completion server and tidied up the codeLicense
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.