Add command to run queries on multiple databases#898
Add command to run queries on multiple databases#898shati-patel merged 6 commits intogithub:mainfrom
Conversation
6b32a3e to
541627e
Compare
|
This looks very neat, and is a nice compact PR! Just had a quick read, will actually test it out later and then do a proper review. |
adityasharad
left a comment
There was a problem hiding this comment.
Nice work! Tried it out and this was a good experience to use, and the code is nice and clear. Some suggestions on how we can do error handling - ping me if you'd like more detail.
| commandRunnerWithProgress( | ||
| 'codeQL.runQueryOnMultipleDatabases', | ||
| async ( | ||
| progress: ProgressCallback, |
There was a problem hiding this comment.
Future note: A cool enhancement might be to have the progress bar reflect how many of the selected databases the query has completed on. Dealing with progress monitors is tricky, so let's think about this later, writing it here just to record the idea.
| if (quickpick !== undefined) { | ||
| for (const item of quickpick) { | ||
| try { | ||
| await compileAndRunQuery(false, uri, progress, token, item.databaseItem); |
There was a problem hiding this comment.
We don't know the language of the query at this point do we? It would be very cool to automatically filter out databases that are of the wrong language. But no problem if we can't do that yet.
There was a problem hiding this comment.
We don't know the language of the query at this point do we? It would be very cool to automatically filter out databases that are of the wrong language. But no problem if we can't do that yet.
Indeed 😅 I couldn't work out how to extract that language information before displaying the quick pick, so I stuck with this for now!
541627e to
6c55f19
Compare
adityasharad
left a comment
There was a problem hiding this comment.
This looks good! Add a changelog entry and we're good to go.
| if (qs !== undefined) { | ||
| const dbItem = await databaseUI.getDatabaseItem(progress, token); | ||
| // If no databaseItem is specified, use the database currently selected in the Databases UI | ||
| const dbItem = databaseItem || await databaseUI.getDatabaseItem(progress, token); |
There was a problem hiding this comment.
Minor: you could use only one variable throughout:
databaseItem = databaseItem || await databaseUI.getDatabaseItem(progress, token);and replace dbItem in the two uses below with databaseItem.
Why do this? To avoid accidentally introducing code later that uses databaseItem when we should be using dbItem. (This is a classic mistake I have made often when two variables have similar names but only one variable has the fully initialised value...)
There was a problem hiding this comment.
Changed! Thanks for the tip—I can definitely see myself getting confused by variables otherwise 🙃
Will fix #569.
First pass at adding a new command "CodeQL: Run Query on Multiple Databases", that lets you select multiple databases from a quick pick menu and run a query over all of them. This currently loops over all selected databases and displays results and query history runs for each database separately. If a database is incompatible, we display an error message and skip to the next database.
This might need some design input if we want to do anything fancy with results display 😅
Expand for a (somewhat grainy) demo
Notes
Checklist
@github/docs-content-codeqlhas been cc'd in all issues for UI or other user-facing changes made by this pull request.