Modify QueryDiscovery to resolve queries by language#2471
Modify QueryDiscovery to resolve queries by language#2471robertbrignull wants to merge 2 commits intomainfrom
Conversation
shati-patel
left a comment
shati-patel
left a comment
There was a problem hiding this comment.
[as discussed offline as well]
I've tested this locally and, whilst it does work, it's unbearably slow on my (Windows) machine 😅 E.g. in the vscode-codeql-starter workspace, it takes well over a minute to populate/update the queries panel.
This is an issue with the underlying CLI command to resolve queries by language, so we should think about where/how best to solve this. (I'll link an existing internal issue about the slowness on Windows... 🔗)
| */ | ||
| private async discoverQueries( | ||
| workspaceFolders: readonly WorkspaceFolder[], | ||
| workspaceFolders: WorkspaceFolder[], |
There was a problem hiding this comment.
Could we instead change the argument types of the discoverQueriesInWorkspaceFolder function so that it also takes a readonly array? It seems like this should still be readonly.
| workspaceFolders: WorkspaceFolder[], | ||
| ): Promise<FileTreeDirectory[]> { | ||
| const rootDirectories = []; | ||
| const allWorkspceFolderPaths = workspaceFolders.map((f) => f.uri.fsPath); |
There was a problem hiding this comment.
| const allWorkspceFolderPaths = workspaceFolders.map((f) => f.uri.fsPath); | |
| const allWorkspaceFolderPaths = workspaceFolders.map((f) => f.uri.fsPath); |
| const fullPath = workspaceFolder.uri.fsPath; | ||
| const name = workspaceFolder.name; | ||
|
|
||
| // We don't want to log each invocation of resolveQueries, since it clutters up the log. |
There was a problem hiding this comment.
| // We don't want to log each invocation of resolveQueries, since it clutters up the log. | |
| // We don't want to log each invocation of resolveQueryByLanguage, since it clutters up the log. |
| return jest.fn().mockImplementation((queryDir: Uri) => { | ||
| const data = dataFn(queryDir.fsPath); | ||
| const value: QueryInfoByLanguage = { | ||
| byLanguage: Object.keys(data.byLanguage || {}).reduce((result, key) => { |
There was a problem hiding this comment.
Could we use Object.entries here instead of Object.keys to avoid needing to call data.byLanguage![key]?
| result[key] = queriesArrayToRecord(data.byLanguage![key]); | ||
| return result; | ||
| }, {} as QueryInfoByLanguage["byLanguage"]), | ||
| noDeclaredLanguage: queriesArrayToRecord(data.noDeclaredLanguage || []), |
There was a problem hiding this comment.
We could move the || [] into the declaration of queriesArrayToRecord by adding queries: string[] = []. I think this would make this a bit shorter.
|
We're no longer going ahead with this PR. We hit some snags using |
3 participants
Changes
QueryDiscoveryto pass the--format=bylanguagearg tocodeql resolve queries. This makes it indicate which language each query is for, if that information is known. We're not using this new info yet, but that'll come soon so we want to have the data ready.The tests proved to be quite fiddly to get right, and creation of mock data is a big clunky. There might be cleaner ways of doing this, and we could look into that if this solution is too awful.
Checklist
ready-for-doc-reviewlabel there.