first cut - migrate settings marked with INTERNAL and adopt to advanced tag #1717
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR migrates internal configuration settings from the chat.advanced.* namespace to a simpler chat.* namespace, making them public/experimental settings accessible to users. The migration includes automatic configuration migration logic to transfer user values from old keys to new keys.
Key changes:
- Created a configuration migration registry system to handle automatic migration of setting values
- Moved 40+ settings from
chat.advanced.*tochat.*namespace and changed them from internal-only to public settings - Updated tests to reflect new setting IDs and public visibility
- Added localization strings and package.json schema entries for newly public settings
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/platform/configuration/common/configurationService.ts | Adds configuration migration infrastructure and updates 40+ setting definitions to use new namespace with migration |
| src/platform/configuration/test/common/configurationService.spec.ts | Updates test assertions to verify new setting IDs and public visibility |
| src/extension/configuration/vscode-node/configurationMigration.ts | Moves migration types to platform layer, imports them instead |
| package.nls.json | Adds localization strings for newly public settings |
| package.json | Adds schema definitions for newly public experimental settings |
| function defineAndMigrateSetting<T>(oldKey: string, newKey: string, defaultValue: T | DefaultValueWithTeamValue<T> | DefaultValueWithTeamAndInternalValue<T>, options?: ConfigOptions): Config<T> { | ||
| const value = defineSetting(newKey, defaultValue, options); | ||
| ConfigurationMigrationRegistry.registerConfigurationMigrations([{ | ||
| key: `${CopilotConfigPrefix}.${oldKey}`, | ||
| migrateFn: async (migrationValue: any) => { | ||
| return [ | ||
| [`${CopilotConfigPrefix}.${newKey}`, { value: migrationValue }], | ||
| [`${CopilotConfigPrefix}.${oldKey}`, { value: undefined }] | ||
| ]; | ||
| } | ||
| }]); | ||
| return value; | ||
| } |
There was a problem hiding this comment.
The defineAndMigrateSetting function lacks documentation. Add a JSDoc comment explaining its purpose, parameters, and the migration behavior (that it automatically migrates values from oldKey to newKey and removes the old setting).
| export function defineAndMigrateExpSetting<T extends ExperimentBasedConfigType>(oldKey: string, newKey: string, defaultValue: T | DefaultValueWithTeamValue<T> | DefaultValueWithTeamAndInternalValue<T>, options?: ConfigOptions, expOptions?: { experimentName?: string }): ExperimentBasedConfig<T> { | ||
| const value = defineExpSetting(newKey, defaultValue, options, expOptions); | ||
| ConfigurationMigrationRegistry.registerConfigurationMigrations([{ | ||
| key: `${CopilotConfigPrefix}.${oldKey}`, | ||
| migrateFn: async (migrationValue: any) => { | ||
| return [ | ||
| [`${CopilotConfigPrefix}.${newKey}`, { value: migrationValue }], | ||
| [`${CopilotConfigPrefix}.${oldKey}`, { value: undefined }] | ||
| ]; | ||
| } | ||
| }]); | ||
| return value; | ||
| } |
There was a problem hiding this comment.
The defineAndMigrateExpSetting function lacks documentation. Add a JSDoc comment explaining its purpose, parameters, and how it differs from defineAndMigrateSetting (it creates an experiment-based setting with migration).
| export type ConfigurationValue = { value: any | undefined /* Remove */ }; | ||
| export type ConfigurationKeyValuePairs = [string, ConfigurationValue][]; | ||
| export type ConfigurationMigrationFn = (value: any) => ConfigurationValue | ConfigurationKeyValuePairs | Promise<ConfigurationValue | ConfigurationKeyValuePairs>; |
There was a problem hiding this comment.
The configuration migration types lack documentation. Add JSDoc comments explaining that ConfigurationValue with undefined marks settings for removal, ConfigurationKeyValuePairs allows multi-key migrations, and ConfigurationMigrationFn handles the migration logic.
| export type ConfigurationValue = { value: any | undefined /* Remove */ }; | |
| export type ConfigurationKeyValuePairs = [string, ConfigurationValue][]; | |
| export type ConfigurationMigrationFn = (value: any) => ConfigurationValue | ConfigurationKeyValuePairs | Promise<ConfigurationValue | ConfigurationKeyValuePairs>; | |
| /** | |
| * Represents a migrated configuration value. | |
| * If `value` is `undefined`, the setting will be removed. | |
| */ | |
| export type ConfigurationValue = { value: any | undefined }; | |
| /** | |
| * Allows migration of multiple configuration keys at once. | |
| * Each tuple contains a key and its corresponding migrated value. | |
| */ | |
| export type ConfigurationKeyValuePairs = [string, ConfigurationValue][]; | |
| /** | |
| * Handles the migration logic for a configuration value. | |
| * Returns either a single migrated value, multiple key-value pairs, or a Promise resolving to one of these. | |
| */ | |
| export type ConfigurationMigrationFn = (value: any) => ConfigurationValue | ConfigurationKeyValuePairs | Promise<ConfigurationValue | ConfigurationKeyValuePairs>; |
|
|
||
| export interface IConfigurationMigrationRegistry { |
There was a problem hiding this comment.
The IConfigurationMigrationRegistry interface lacks documentation. Add a JSDoc comment explaining its purpose as a registry for configuration setting migrations that are automatically executed when the extension loads.
| export interface IConfigurationMigrationRegistry { | |
| /** | |
| * Registry for configuration setting migrations. | |
| * | |
| * This interface allows registration of configuration migrations, which are functions | |
| * that transform or update configuration values. These migrations are automatically | |
| * executed when the extension loads, ensuring that user settings are kept up to date | |
| * with the latest schema or requirements. | |
| */ | |
| export interface IConfigurationMigrationRegistry { | |
| /** | |
| * Registers one or more configuration migrations to be executed automatically on extension load. | |
| * @param configurationMigrations Array of configuration migration objects, each specifying a key and a migration function. | |
| */ |
No description provided.