feat: add configurable file indexing logic #967
Conversation
...s-lsp-codewhisperer/src/language-server/localProjectContext/localProjectContextController.ts
Outdated
Show resolved
Hide resolved
This reverts commit 66f0fd4.
This reverts commit a2ee818.
| const localGitIgnoreFiles: string[] = [] | ||
|
|
||
| const crawler = new fdir() | ||
| .withSymlinks({ resolvePaths: !includeSymLinks }) |
There was a problem hiding this comment.
symlinks can cause loops, or just really poor performance on complex directories. I assume includeSymLinks decides whether symlinks are followed. It should probably be false by default.
There was a problem hiding this comment.
Yup it's false by default. Not sure if we want to surface this setting to the user yet, but loops and poor performance are definitely a possible risk. We have an upper bound on the index size though, so we wouldn't explicitly break anything.
| this.fileExtensions = Object.keys(languageByExtension) | ||
| private ignoreFilePatterns?: string[] | ||
| private includeSymlinks?: boolean | ||
| private maxFileSizeMb?: number |
There was a problem hiding this comment.
should this by MB? or is are they actually representing bits?
There was a problem hiding this comment.
Oh I see, yup it should be MB. I'll make the change.
There was a problem hiding this comment.
@jpinkney-aws fixed this!
| private readonly DEFAULT_MAX_FILE_SIZE = 10 | ||
| private readonly MB_TO_BYTES = 1024 * 1024 | ||
|
|
||
| constructor(clientName: string, workspaceFolders: WorkspaceFolder[], logging?: Logging) { |
There was a problem hiding this comment.
why has the logger now become optional?
There was a problem hiding this comment.
I made this change for testing on my local. I'll revert this change before merging.
There was a problem hiding this comment.
@jpinkney-aws fixed this as well!
| const ignore = require('ignore') | ||
| const { fdir } = require('fdir') | ||
| const fs = require('fs') |
There was a problem hiding this comment.
are these necessary to be required or can they be import as well?
There was a problem hiding this comment.
I believe these should be importable as well. I can make the change.
| @@ -43,13 +73,28 @@ export class LocalProjectContextController { | |||
| return this.instance | |||
| } | |||
|
|
|||
| public async init(vectorLib?: any): Promise<void> { | |||
| public async init({ | |||
There was a problem hiding this comment.
Do we have a way to deliver a no-op version of the entire workspace indexing that has no dependencies on os and fs and vectorLib? We need to make sure that this server can be loaded in a browser environment eventually, with local indexing either disabled or replaced by something suitable for the browser. This is not urgent but we'll need to come up with a solution eventually.
There was a problem hiding this comment.
I believe if the vector lib cannot be resolved (in the browser environment for instance), all calls to the controller are effectively no-ops. But @breedloj would be able to answer that better.
Problem
When the LSP is initialized, local project context needs to crawl the workspaces, build a repo map, and create an index with it that will be used to provide additional context to Q. The workspace need to be crawled and indexed and what files to include in the index needs to be configurable.
Solution
File indexing options are exposed to the user through the
workspace/configurationcall. This information is used to determine what files to include in the index by theprocessWorkspaceFoldersmethod inlocalProjectContextController.Justification for newly introduced libraries
.gitignoreconvention. These rules are configurable by different IDEs and are passed in through theworkspace/configurationcall. It is also used to parse the users.gitignorefiles for further filtering.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.