-
Notifications
You must be signed in to change notification settings - Fork 751
refactor(settings): minimize duplication across prompt settings. #6243
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
Closed
Closed
Changes from all commits
Commits
Show all changes
25 commits
Select commit
Hold shift + click to select a range
018d719
run branch check on PRs only
Hweinstock d03a2da
use event branch name
Hweinstock 4fb8e66
change wording
Hweinstock cdecab6
add extra check
Hweinstock f2900e5
switch to single quotes
Hweinstock 902de77
Merge branch 'aws:master' into master
Hweinstock bf03fd3
Merge branch 'aws:master' into master
Hweinstock 4cd5400
Merge branch 'aws:master' into master
Hweinstock 0234303
Merge branch 'aws:master' into master
Hweinstock f988458
Merge branch 'aws:master' into master
Hweinstock 54be0b3
Merge branch 'aws:master' into master
Hweinstock d4f7dda
Merge branch 'aws:master' into master
Hweinstock 33f900a
Merge branch 'aws:master' into master
Hweinstock e16fe17
factor out to general isEnabled function
Hweinstock 4498a73
avoid using same function for experiment
Hweinstock e9b39f2
factor out other method
Hweinstock df97e33
move code to be adjacent
Hweinstock ea22122
generate class based on prompt
Hweinstock 7c55670
Merge branch 'aws:master' into master
Hweinstock 78dd426
Merge branch 'master' into settingsRefactor
Hweinstock 71fc1d9
fix type problem
Hweinstock 44a7613
export type as well as value
Hweinstock e0747bf
remove unnecessary await
Hweinstock 4d8545e
remove awaits
Hweinstock d363ea6
Merge branch 'master' into settingsRefactor
Hweinstock File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -600,7 +600,7 @@ export function fromExtensionManifest<T extends TypeDescriptor & Partial<Section | |
| * | ||
| * ### Usage: | ||
| * ``` | ||
| * if (await settings.isPromptEnabled('myPromptName')) { | ||
| * if (settings.isPromptEnabled('myPromptName')) { | ||
| * // Show some sort of prompt | ||
| * const userResponse = await promptUser() | ||
| * | ||
|
|
@@ -619,68 +619,44 @@ export function fromExtensionManifest<T extends TypeDescriptor & Partial<Section | |
| * core lib as necessary. | ||
| */ | ||
| export const toolkitPrompts = settingsProps['aws.suppressPrompts'] | ||
| type toolkitPromptName = keyof typeof toolkitPrompts | ||
| export class ToolkitPromptSettings | ||
| extends Settings.define( | ||
| 'aws.suppressPrompts', | ||
| toRecord(keys(toolkitPrompts), () => Boolean) | ||
| ) | ||
| implements PromptSettings | ||
| { | ||
| public async isPromptEnabled(promptName: toolkitPromptName): Promise<boolean> { | ||
| try { | ||
| return !this._getOrThrow(promptName, false) | ||
| } catch (e) { | ||
| this._log('prompt check for "%s" failed: %s', promptName, (e as Error).message) | ||
| await this.reset() | ||
|
|
||
| return true | ||
| } | ||
| } | ||
|
|
||
| public async disablePrompt(promptName: toolkitPromptName): Promise<void> { | ||
| if (await this.isPromptEnabled(promptName)) { | ||
| await this.update(promptName, true) | ||
| } | ||
| } | ||
|
|
||
| static #instance: ToolkitPromptSettings | ||
|
|
||
| public static get instance() { | ||
| return (this.#instance ??= new this()) | ||
| } | ||
| } | ||
| export const ToolkitPromptSettings = getPromptSettings('aws.suppressPrompts') | ||
| export type ToolkitPromptSettings = InstanceType<typeof ToolkitPromptSettings> | ||
|
|
||
| export const amazonQPrompts = settingsProps['amazonQ.suppressPrompts'] | ||
| type amazonQPromptName = keyof typeof amazonQPrompts | ||
| export class AmazonQPromptSettings | ||
| extends Settings.define( | ||
| 'amazonQ.suppressPrompts', | ||
| toRecord(keys(amazonQPrompts), () => Boolean) | ||
| ) | ||
| implements PromptSettings | ||
| { | ||
| public async isPromptEnabled(promptName: amazonQPromptName): Promise<boolean> { | ||
| try { | ||
| return !this._getOrThrow(promptName, false) | ||
| } catch (e) { | ||
| this._log('prompt check for "%s" failed: %s', promptName, (e as Error).message) | ||
| await this.reset() | ||
| export const AmazonQPromptSettings = getPromptSettings('amazonQ.suppressPrompts') | ||
| export type AmazonQPromptSettings = InstanceType<typeof AmazonQPromptSettings> | ||
|
|
||
| function getPromptSettings<P extends 'amazonQ.suppressPrompts' | 'aws.suppressPrompts'>(promptsKey: P) { | ||
| const prompts = settingsProps[promptsKey] | ||
| type promptName = keyof typeof prompts & string | ||
| return class AnonymousPromptSettings extends Settings.define( | ||
| promptsKey, | ||
| toRecord(keys(prompts), () => Boolean) | ||
| ) { | ||
| public isPromptEnabled(promptName: promptName) { | ||
| try { | ||
| return !this._getOrThrow(promptName, false as never) | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Believe there is an issue with how I setup types since I need the cast here. |
||
| } catch (e) { | ||
| this._log('prompt check for "%s" failed: %s', promptName, (e as Error).message) | ||
| this.reset().catch((e) => | ||
| getLogger().error(`failed to reset prompt settings: %O`, (e as Error).message) | ||
| ) | ||
|
|
||
| return true | ||
| return true | ||
| } | ||
| } | ||
| } | ||
|
|
||
| public async disablePrompt(promptName: amazonQPromptName): Promise<void> { | ||
| if (await this.isPromptEnabled(promptName)) { | ||
| await this.update(promptName, true) | ||
| public async disablePrompt(promptName: promptName) { | ||
| if (this.isPromptEnabled(promptName)) { | ||
| await this.update(promptName, true as never) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| static #instance: AmazonQPromptSettings | ||
| static #instance: AnonymousPromptSettings | ||
|
|
||
| public static get instance() { | ||
| return (this.#instance ??= new this()) | ||
| public static get instance() { | ||
| return (this.#instance ??= new this()) | ||
| } | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -689,10 +665,10 @@ export class AmazonQPromptSettings | |
| * which is the intersection of the types (only the values that occur | ||
| * in each are selected), but idk how to do that. | ||
| */ | ||
| type AllPromptNames = amazonQPromptName | toolkitPromptName | ||
| type AllPromptNames = keyof typeof toolkitPrompts | keyof typeof amazonQPrompts | ||
|
|
||
| export interface PromptSettings { | ||
| isPromptEnabled(promptName: AllPromptNames): Promise<boolean> | ||
| isPromptEnabled(promptName: AllPromptNames): boolean | ||
| disablePrompt(promptName: AllPromptNames): Promise<void> | ||
| } | ||
|
|
||
|
|
@@ -708,7 +684,7 @@ type ExperimentName = keyof typeof experiments | |
| * ### Usage: | ||
| * ``` | ||
| * function myExperimentalFeature(): void { | ||
| * if (!(await settings.isExperimentEnabled('myExperimentalFeature'))) { | ||
| * if (!( settings.isExperimentEnabled('myExperimentalFeature'))) { | ||
| * return | ||
| * } | ||
| * | ||
|
|
||
This file contains hidden or 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
This file contains hidden or 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
Oops, something went wrong.
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.
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.
This is a welcome change 👍
Not a strong reason for this function to be async
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.
isolated this change here: https://github.com/aws/aws-toolkit-vscode/pull/6255/files