From d92edfb058c2f0a6c140b3162978ced09ee9d924 Mon Sep 17 00:00:00 2001 From: Jason Reed Date: Tue, 19 May 2020 09:10:10 -0400 Subject: [PATCH 1/4] Remove database panel icon commands from command palette This corrects what is an unfortunately common accidental antipattern, where creating a command meant just to be the handler of a user interface button ends up in the command palette unless you explicitly set `"when": "false"` in the command palette section of the configuration. Also enforce the naming convention that commands prefixed with `codeQLDatabases.` are those meant for the databases panel only, while prefixing `codeQL.` means that it's meant to be directly accessible through the command palette. --- extensions/ql-vscode/package.json | 30 +++++++++++++++++------- extensions/ql-vscode/src/databases-ui.ts | 6 ++--- 2 files changed, 24 insertions(+), 12 deletions(-) diff --git a/extensions/ql-vscode/package.json b/extensions/ql-vscode/package.json index 09335770591..a39bce1091b 100644 --- a/extensions/ql-vscode/package.json +++ b/extensions/ql-vscode/package.json @@ -27,9 +27,9 @@ "onView:codeQLQueryHistory", "onView:test-explorer", "onCommand:codeQL.checkForUpdatesToCLI", - "onCommand:codeQL.chooseDatabaseFolder", - "onCommand:codeQL.chooseDatabaseArchive", - "onCommand:codeQL.chooseDatabaseInternet", + "onCommand:codeQLDatabases.chooseDatabaseFolder", + "onCommand:codeQLDatabases.chooseDatabaseArchive", + "onCommand:codeQLDatabases.chooseDatabaseInternet", "onCommand:codeQL.setCurrentDatabase", "onCommand:codeQL.downloadDatabase", "onCommand:codeQLDatabases.chooseDatabase", @@ -175,7 +175,7 @@ "title": "CodeQL: Quick Query" }, { - "command": "codeQL.chooseDatabaseFolder", + "command": "codeQLDatabases.chooseDatabaseFolder", "title": "Choose Database from Folder", "icon": { "light": "media/light/folder-opened-plus.svg", @@ -183,7 +183,7 @@ } }, { - "command": "codeQL.chooseDatabaseArchive", + "command": "codeQLDatabases.chooseDatabaseArchive", "title": "Choose Database from Archive", "icon": { "light": "media/light/archive-plus.svg", @@ -191,7 +191,7 @@ } }, { - "command": "codeQL.chooseDatabaseInternet", + "command": "codeQLDatabases.chooseDatabaseInternet", "title": "Download database", "icon": { "light": "media/light/cloud-download.svg", @@ -312,17 +312,17 @@ "group": "navigation" }, { - "command": "codeQL.chooseDatabaseFolder", + "command": "codeQLDatabases.chooseDatabaseFolder", "when": "view == codeQLDatabases", "group": "navigation" }, { - "command": "codeQL.chooseDatabaseArchive", + "command": "codeQLDatabases.chooseDatabaseArchive", "when": "view == codeQLDatabases", "group": "navigation" }, { - "command": "codeQL.chooseDatabaseInternet", + "command": "codeQLDatabases.chooseDatabaseInternet", "when": "view == codeQLDatabases", "group": "navigation" } @@ -442,6 +442,18 @@ "command": "codeQLDatabases.removeDatabase", "when": "false" }, + { + "command": "codeQLDatabases.chooseDatabaseFolder", + "when": "false" + }, + { + "command": "codeQLDatabases.chooseDatabaseArchive", + "when": "false" + }, + { + "command": "codeQLDatabases.chooseDatabaseInternet", + "when": "false" + }, { "command": "codeQLQueryHistory.openQuery", "when": "false" diff --git a/extensions/ql-vscode/src/databases-ui.ts b/extensions/ql-vscode/src/databases-ui.ts index 0314089c315..5f00b301d6f 100644 --- a/extensions/ql-vscode/src/databases-ui.ts +++ b/extensions/ql-vscode/src/databases-ui.ts @@ -174,9 +174,9 @@ export class DatabaseUI extends DisposableObject { this.treeDataProvider = this.push(new DatabaseTreeDataProvider(ctx, databaseManager)); this.push(window.createTreeView('codeQLDatabases', { treeDataProvider: this.treeDataProvider })); - ctx.subscriptions.push(commands.registerCommand('codeQL.chooseDatabaseFolder', this.handleChooseDatabaseFolder)); - ctx.subscriptions.push(commands.registerCommand('codeQL.chooseDatabaseArchive', this.handleChooseDatabaseArchive)); - ctx.subscriptions.push(commands.registerCommand('codeQL.chooseDatabaseInternet', this.handleChooseDatabaseInternet)); + ctx.subscriptions.push(commands.registerCommand('codeQLDatabases.chooseDatabaseFolder', this.handleChooseDatabaseFolder)); + ctx.subscriptions.push(commands.registerCommand('codeQLDatabases.chooseDatabaseArchive', this.handleChooseDatabaseArchive)); + ctx.subscriptions.push(commands.registerCommand('codeQLDatabases.chooseDatabaseInternet', this.handleChooseDatabaseInternet)); ctx.subscriptions.push(commands.registerCommand('codeQL.setCurrentDatabase', this.handleSetCurrentDatabase)); ctx.subscriptions.push(commands.registerCommand('codeQL.upgradeCurrentDatabase', this.handleUpgradeCurrentDatabase)); ctx.subscriptions.push(commands.registerCommand('codeQL.clearCache', this.handleClearCache)); From ab09cdb66d2748a11648fad92aab5c528d2cd4be Mon Sep 17 00:00:00 2001 From: Jason Reed Date: Tue, 19 May 2020 11:02:32 -0400 Subject: [PATCH 2/4] Make capitalization consistent --- extensions/ql-vscode/package.json | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/extensions/ql-vscode/package.json b/extensions/ql-vscode/package.json index a39bce1091b..7cdc8e3a7c2 100644 --- a/extensions/ql-vscode/package.json +++ b/extensions/ql-vscode/package.json @@ -192,7 +192,7 @@ }, { "command": "codeQLDatabases.chooseDatabaseInternet", - "title": "Download database", + "title": "Download Database", "icon": { "light": "media/light/cloud-download.svg", "dark": "media/dark/cloud-download.svg" @@ -232,7 +232,7 @@ }, { "command": "codeQL.downloadDatabase", - "title": "CodeQL: Download database" + "title": "CodeQL: Download Database" }, { "command": "codeQLDatabases.sortByName", From c01772848c325c6b5e328e79e4de92f6d42f4bf0 Mon Sep 17 00:00:00 2001 From: Jason Reed Date: Tue, 19 May 2020 11:32:54 -0400 Subject: [PATCH 3/4] Add all db-getting commands (dl, folder, zip) to command palette --- extensions/ql-vscode/package.json | 18 ++++++++++++------ extensions/ql-vscode/src/databases-ui.ts | 6 +++--- extensions/ql-vscode/src/extension.ts | 5 +++-- 3 files changed, 18 insertions(+), 11 deletions(-) diff --git a/extensions/ql-vscode/package.json b/extensions/ql-vscode/package.json index 7cdc8e3a7c2..1860e00c500 100644 --- a/extensions/ql-vscode/package.json +++ b/extensions/ql-vscode/package.json @@ -31,7 +31,9 @@ "onCommand:codeQLDatabases.chooseDatabaseArchive", "onCommand:codeQLDatabases.chooseDatabaseInternet", "onCommand:codeQL.setCurrentDatabase", - "onCommand:codeQL.downloadDatabase", + "onCommand:codeQL.chooseDatabaseFolder", + "onCommand:codeQL.chooseDatabaseArchive", + "onCommand:codeQL.chooseDatabaseInternet", "onCommand:codeQLDatabases.chooseDatabase", "onCommand:codeQLDatabases.setCurrentDatabase", "onCommand:codeQL.quickQuery", @@ -231,7 +233,15 @@ "title": "Show Database Directory" }, { - "command": "codeQL.downloadDatabase", + "command": "codeQL.chooseDatabaseFolder", + "title": "CodeQL: Choose Database from Folder" + }, + { + "command": "codeQL.chooseDatabaseArchive", + "title": "CodeQL: Choose Database from Archive" + }, + { + "command": "codeQL.chooseDatabaseInternet", "title": "CodeQL: Download Database" }, { @@ -406,10 +416,6 @@ "command": "codeQL.runQuery", "when": "resourceLangId == ql && resourceExtname == .ql" }, - { - "command": "codeQL.downloadDatabase", - "when": "true" - }, { "command": "codeQL.quickEval", "when": "editorLangId == ql" diff --git a/extensions/ql-vscode/src/databases-ui.ts b/extensions/ql-vscode/src/databases-ui.ts index 5f00b301d6f..62087189ceb 100644 --- a/extensions/ql-vscode/src/databases-ui.ts +++ b/extensions/ql-vscode/src/databases-ui.ts @@ -193,7 +193,7 @@ export class DatabaseUI extends DisposableObject { await this.databaseManager.setCurrentDatabaseItem(databaseItem); } - private handleChooseDatabaseFolder = async (): Promise => { + handleChooseDatabaseFolder = async (): Promise => { try { return await this.chooseAndSetDatabase(true); } catch (e) { @@ -202,7 +202,7 @@ export class DatabaseUI extends DisposableObject { } } - private handleChooseDatabaseArchive = async (): Promise => { + handleChooseDatabaseArchive = async (): Promise => { try { return await this.chooseAndSetDatabase(false); } catch (e) { @@ -211,7 +211,7 @@ export class DatabaseUI extends DisposableObject { } } - private handleChooseDatabaseInternet = async (): Promise => { + handleChooseDatabaseInternet = async (): Promise => { return await promptImportInternetDatabase(this.databaseManager, this.storagePath); } diff --git a/extensions/ql-vscode/src/extension.ts b/extensions/ql-vscode/src/extension.ts index 152b2a0ef8c..461bc71b493 100644 --- a/extensions/ql-vscode/src/extension.ts +++ b/extensions/ql-vscode/src/extension.ts @@ -20,7 +20,6 @@ import { displayQuickQuery } from './quick-query'; import { compileAndRunQueryAgainstDatabase, tmpDirDisposal, UserCancellationException } from './run-queries'; import { QLTestAdapterFactory } from './test-adapter'; import { TestUIService } from './test-ui'; -import { promptImportInternetDatabase } from './databaseFetcher'; /** * extension.ts @@ -335,7 +334,9 @@ async function activateWithInstalledDistribution(ctx: ExtensionContext, distribu await qs.restartQueryServer(); helpers.showAndLogInformationMessage('CodeQL Query Server restarted.', { outputLogger: queryServerLogger }); })); - ctx.subscriptions.push(commands.registerCommand('codeQL.downloadDatabase', () => promptImportInternetDatabase(dbm, getContextStoragePath(ctx)))); + ctx.subscriptions.push(commands.registerCommand('codeQL.chooseDatabaseFolder', () => databaseUI.handleChooseDatabaseFolder())); + ctx.subscriptions.push(commands.registerCommand('codeQL.chooseDatabaseArchive', () => databaseUI.handleChooseDatabaseArchive())); + ctx.subscriptions.push(commands.registerCommand('codeQL.chooseDatabaseInternet', () => databaseUI.handleChooseDatabaseInternet())); ctx.subscriptions.push(client.start()); From dd1bdf54bbe413328445f39ae65b235a6d0cd035 Mon Sep 17 00:00:00 2001 From: Jason Reed Date: Tue, 19 May 2020 12:16:52 -0400 Subject: [PATCH 4/4] Add integrity check for commands in package.json Attempt to enforce some regularity in how we name commands, and fix one command that was showing up improperly in the command palette. --- extensions/ql-vscode/package.json | 4 + .../test/pure-tests/command-lint.test.ts | 101 ++++++++++++++++++ 2 files changed, 105 insertions(+) create mode 100644 extensions/ql-vscode/test/pure-tests/command-lint.test.ts diff --git a/extensions/ql-vscode/package.json b/extensions/ql-vscode/package.json index 1860e00c500..52ac7d45b88 100644 --- a/extensions/ql-vscode/package.json +++ b/extensions/ql-vscode/package.json @@ -460,6 +460,10 @@ "command": "codeQLDatabases.chooseDatabaseInternet", "when": "false" }, + { + "command": "codeQLDatabases.upgradeDatabase", + "when": "false" + }, { "command": "codeQLQueryHistory.openQuery", "when": "false" diff --git a/extensions/ql-vscode/test/pure-tests/command-lint.test.ts b/extensions/ql-vscode/test/pure-tests/command-lint.test.ts new file mode 100644 index 00000000000..45c3b445369 --- /dev/null +++ b/extensions/ql-vscode/test/pure-tests/command-lint.test.ts @@ -0,0 +1,101 @@ +import { expect } from 'chai'; +import * as path from 'path'; +import * as fs from 'fs-extra'; + +type CmdDecl = { + command: string; + when?: string; + title?: string; +} + +describe('commands declared in package.json', function() { + const manifest = fs.readJsonSync(path.join(__dirname, '../../package.json')); + const commands = manifest.contributes.commands; + const menus = manifest.contributes.menus; + + const disabledInPalette: Set = new Set(); + + // These commands should appear in the command palette, and so + // should be prefixed with 'CodeQL: '. + const paletteCmds: Set = new Set(); + + // These commands arising on context menus in non-CodeQL controlled + // panels, (e.g. file browser) and so should be prefixed with 'CodeQL: '. + const contribContextMenuCmds: Set = new Set(); + + // These are commands used in CodeQL controlled panels, and so don't need any prefixing in their title. + const scopedCmds: Set = new Set(); + const commandTitles: { [cmd: string]: string } = {}; + + commands.forEach((commandDecl: CmdDecl) => { + const { command, title } = commandDecl; + if (command.match(/^codeQL\./) + || command.match(/^codeQLQueryResults\./) + || command.match(/^codeQLTests\./)) { + paletteCmds.add(command); + expect(title).not.to.be.undefined; + commandTitles[command] = title!; + } + else if (command.match(/^codeQLDatabases\./) + || command.match(/^codeQLQueryHistory\./)) { + scopedCmds.add(command); + expect(title).not.to.be.undefined; + commandTitles[command] = title!; + } + else { + expect.fail(`Unexpected command name ${command}`); + } + }); + + menus['explorer/context'].forEach((commandDecl: CmdDecl) => { + const { command } = commandDecl; + paletteCmds.delete(command); + contribContextMenuCmds.add(command); + }); + + menus['editor/context'].forEach((commandDecl: CmdDecl) => { + const { command } = commandDecl; + paletteCmds.delete(command); + contribContextMenuCmds.add(command); + }); + + menus.commandPalette.forEach((commandDecl: CmdDecl) => { + if (commandDecl.when === 'false') + disabledInPalette.add(commandDecl.command); + }); + + + + it('should have commands appropriately prefixed', function() { + paletteCmds.forEach(command => { + expect(commandTitles[command], `command ${command} should be prefixed with 'CodeQL: ', since it is accessible from the command palette`).to.match(/^CodeQL: /); + }); + + contribContextMenuCmds.forEach(command => { + expect(commandTitles[command], `command ${command} should be prefixed with 'CodeQL: ', since it is accessible from a context menu in a non-extension-controlled context`).to.match(/^CodeQL: /); + }); + + scopedCmds.forEach(command => { + expect(commandTitles[command], `command ${command} should not be prefixed with 'CodeQL: ', since it is accessible from an extension-controlled context`).not.to.match(/^CodeQL: /); + }); + }); + + it('should have the right commands accessible from the command palette', function() { + paletteCmds.forEach(command => { + expect(disabledInPalette.has(command), `command ${command} should be enabled in the command palette`).to.be.false; + }); + + // Commands in contribContextMenuCmds may reasonbly be enabled or + // disabled in the command palette; for example, codeQL.runQuery + // is available there, since we heuristically figure out which + // query to run, but codeQL.setCurrentDatabase is not. + + scopedCmds.forEach(command => { + expect(disabledInPalette.has(command), `command ${command} should be disabled in the command palette`).to.be.true; + }); + }); + + +}); + +