New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(amazonq): add commands to command palette #4965
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"toggle auto-suggestions" would be another good one to have in the palette
Commands Added - Amazon Q: Sign Out - Amazon Q: Open Code Reference Log - Amazon Q: Select Customization
47c4bbb
to
4095ecd
Compare
TODO / Fixes
Metric Details
|
"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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can these be one "toggle" command instead of 2 separate commands? that allows users to define a single keybinding
"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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same question for these 2
"category": "%AWS.amazonq.title%" | ||
}, | ||
{ | ||
"command": "aws.amazonq.gettingStarted", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why does this need to be the command palette? vscode already exposes all walkthroughs via Welcome: Open Walkthrough...
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wouldn't need these if we had "toggle" command instead
@@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wouldn't need this either, if we have the toggle command
@@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
export const commandPalette = 'CommandPalette' | |
export const commandPalette = 'commandPalette' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 -
telemetry.record({ source: 'CommandPalette' }) |
I will update it in the next revision.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the
CommandPalette
convention 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Problem
Add commands in Command Palette for Amazon Q to improve feature discoverability
Solution
Commands Added
TODO:
FromCommandPalette
pattern. -- Discussed offline, this is the right approach. Going forward use the same prefix and add.commandPalette
as prefix.telemetry
/metrics
License
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.