feat(amazonq): add commands to command palette #4965
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
justinmk3
approved these changes
May 9, 2024
Commands Added - Amazon Q: Sign Out - Amazon Q: Open Code Reference Log - Amazon Q: Select Customization
ivikash
force-pushed
the
feat/amazon-q-commands
branch
from
47c4bbb
to
4095ecd
Compare
TODO / Fixes
Metric Details
|
justinmk3
reviewed
May 12, 2024
Comment on lines
+639
to
+645
| "command": "aws.amazonq.resumeAutoScans", | ||
| "title": "%AWS.amazonq.resumeAutoScans%", | ||
| "category": "%AWS.amazonq.title%", | ||
| "enablement": "!aws.codewhisperer.isScanEnabled" | ||
| }, | ||
| { | ||
| "command": "aws.amazonq.pauseAutoScans", |
There was a problem hiding this comment.
can these be one "toggle" command instead of 2 separate commands? that allows users to define a single keybinding
justinmk3
reviewed
May 12, 2024
Comment on lines
+627
to
+633
| "command": "aws.amazonq.resumeAutoSuggestions", | ||
| "title": "%AWS.amazonq.resumeAutoSuggestions%", | ||
| "category": "%AWS.amazonq.title%", | ||
| "enablement": "!aws.codewhisperer.isSuggestionsEnabled" | ||
| }, | ||
| { | ||
| "command": "aws.amazonq.pauseAutoSuggestions", |
There was a problem hiding this comment.
same question for these 2
justinmk3
reviewed
May 12, 2024
| "category": "%AWS.amazonq.title%" | ||
| }, | ||
| { | ||
| "command": "aws.amazonq.gettingStarted", |
There was a problem hiding this comment.
why does this need to be the command palette? vscode already exposes all walkthroughs via Welcome: Open Walkthrough...
justinmk3
reviewed
May 12, 2024
Comment on lines
+55
to
+59
| const isSuggestionsEnabled = CodeSuggestionsState.instance.isSuggestionsEnabled() | ||
| await vscode.commands.executeCommand('setContext', 'aws.codewhisperer.isSuggestionsEnabled', isSuggestionsEnabled) | ||
|
|
||
| const isScansEnabled = CodeScansState.instance.isScansEnabled() | ||
| await vscode.commands.executeCommand('setContext', 'aws.codewhisperer.isScanEnabled', isScansEnabled) |
There was a problem hiding this comment.
wouldn't need these if we had "toggle" command instead
justinmk3
reviewed
May 12, 2024
| @@ -84,13 +90,15 @@ export const toggleCodeScans = Commands.declare( | |||
| }) | |||
|
|
|||
| const isScansEnabled = await scansState.toggleScans() | |||
| await vscode.commands.executeCommand('setContext', 'aws.codewhisperer.isScanEnabled', isScansEnabled) | |||
There was a problem hiding this comment.
wouldn't need this either, if we have the toggle command
justinmk3
reviewed
May 12, 2024
| @@ -16,6 +16,8 @@ export const amazonQChatSource = 'amazonQChat' | |||
| export const firstStartUpSource = ExtStartUpSources.firstStartUp | |||
| /** Indicates a CodeWhisperer command was executed as a result of selecting an ellipses menu item */ | |||
| export const cwEllipsesMenu = 'ellipsesMenu' | |||
| /** Indicates a CodeWhisperer command was executed from the command palette */ | |||
| export const commandPalette = 'CommandPalette' | |||
There was a problem hiding this comment.
please, always use the existing code as a hint about conventions. none of the other ids above use PascalCase.
is this actually needed? it is assumed that all commands are from the command palette, unless they have another source id.
Suggested change
| export const commandPalette = 'CommandPalette' | |
| export const commandPalette = 'commandPalette' |
There was a problem hiding this comment.
Certainly. To clarify, I initially preferred to use camelCase in this context. However, upon reviewing the existing code, I noticed and believed that the CommandPalette convention is already implemented as seen in here -
I will update it in the next revision.
There was a problem hiding this comment.
the
CommandPaletteconvention is already implemented as seen in here
Great, thank you for noting that. In that case, yes we should keep the existing name, but please update that code to use the constant you defined here.
There was a problem hiding this comment.
I saw another place relying on commandPalette so I am erring on the side of using lowecase and updating codecatalyst/commands.ts to use the same with smaller case.
I am making sure all instances use the same variable.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Problem
Add commands in Command Palette for Amazon Q to improve feature discoverability
Solution
Commands Added
TODO:
FromCommandPalettepattern. -- Discussed offline, this is the right approach. Going forward use the same prefix and add.commandPaletteas prefix.telemetry/metricsLicense
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.