fix(amazonq): Use common utility to determine workspaceFolders and fix tests #1353
Conversation
| let localProjectContextController: LocalProjectContextController | ||
| let amazonQServiceManager: AmazonQTokenServiceManager | ||
| let telemetryService: TelemetryService | ||
|
|
||
| let localProjectContextEnabled: boolean = false | ||
|
|
||
| lsp.addInitializer((params: InitializeParams) => { | ||
| const workspaceFolders = getWorkspaceFolders(logging, params) | ||
| const workspaceFolders = workspace.getAllWorkspaceFolders() || params.workspaceFolders |
There was a problem hiding this comment.
Should this be ?? I'm a little rusty...
Similarly in workspaceContextServer.ts
There was a problem hiding this comment.
From what I could find, it seems like it the right hand side would be used as fallback when left-hand side is any falsy value (including false, 0, "", null, undefined, and NaN). That seemed fine to me
| @@ -99,12 +99,16 @@ export function formatListing(entry: Dirent): string { | |||
| } | |||
|
|
|||
| export function getEntryPath(entry: Dirent) { | |||
| if (!entry.parentPath) { | |||
There was a problem hiding this comment.
doesn't typescript satisfy this automatically since parentpath is a non-optional field in Dirent?
The only place where the parentpath was not being set before was in the testing stubs that had custom behaviour and that were doing lots of type casting. Throwing and error here doesn't look really right to me, now with the parentpath changes introduced in the PR do we need to throw this error?
There was a problem hiding this comment.
I will remove the throw and error check in getEntryPath as I was seeing errors locally where parentpath was null with tests. I wasn't sure if this only impacted tests but as long as it does not impact actual logic, I am fine with removing this
| @@ -99,12 +99,16 @@ export function formatListing(entry: Dirent): string { | |||
| } | |||
|
|
|||
| export function getEntryPath(entry: Dirent) { | |||
| if (!entry.parentPath) { | |||
| throw new Error(`Entry is missing parentPath property: ${JSON.stringify(entry)}`) | |||
| } | |||
| return path.join(entry.parentPath, entry.name) | |||
| } | |||
|
|
|||
| // TODO: port this to runtimes? | |||
There was a problem hiding this comment.
I think we can remove this comment as well
Description
This change address a few different things concerning the usage of
workspaceFoldersacross servers.workspace.getAllWorkspaceFolders()which was introduced in the runtimes PR as a common utility(it respectsdidChangeWorkspaceFolderevent alongwith using rootUri of initializeparams in absence ofworkspaceFolder)In addition, this change fixes test failures seen locally on Windows devices associated with
workspaceUtils.test.tsandfileSearch.test.tswhereeverfs.readDircalls were being stubbed out. Errorpath is undefinedwas being thrown for tests wheregetEntryPathwas invoked.License
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.