-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Revert "first cut - migrate settings marked with INTERNAL and adopt to advanced tag" #2021
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
Conversation
…o advanc…" This reverts commit 7098ab5.
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.
Pull Request Overview
This PR reverts a previous change (#1717) that migrated internal configuration settings from the AdvancedExperimental and AdvancedExperimentalExperiments namespaces to the Internal namespace and removed the "advanced" configuration section from package.json.
Key changes:
- Moves configuration definitions back from
ConfigKey.InternaltoConfigKey.AdvancedExperimentalandConfigKey.AdvancedExperimentalExperimentsnamespaces - Restores the "advanced" configuration section in package.json with all previously migrated settings
- Updates all usage sites across the codebase to reference the original configuration key locations
- Reverts configuration service implementation changes related to old key migration support
Reviewed Changes
Copilot reviewed 59 out of 59 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/platform/configuration/common/configurationService.ts | Restores AdvancedExperimental namespaces, removes Internal namespace consolidation, adds back migration support |
| src/platform/configuration/vscode/configurationServiceImpl.ts | Removes unused imports, restores old key fallback logic in configuration resolution |
| src/platform/configuration/test/common/configurationService.spec.ts | Updates test assertions to verify settings under AdvancedExperimental namespaces instead of Internal |
| src/extension/configuration/vscode-node/configurationMigration.ts | Moves ConfigurationMigrationRegistry definition from common to here |
| package.json | Restores "advanced" configuration section with 50+ settings |
| package.nls.json | Removes localization strings for migrated settings (no longer public) |
| src/extension/test/node/configurations.spec.ts | Updates tests to handle AdvancedExperimental settings and validate against package.json |
| Multiple extension files | Updates all ConfigKey references from Internal back to AdvancedExperimental/AdvancedExperimentalExperiments |
| export type ConfigurationValue = { value: any | undefined /* Remove */ }; | ||
| export type ConfigurationKeyValuePairs = [string, ConfigurationValue][]; | ||
| export type ConfigurationMigrationFn = (value: any) => ConfigurationValue | ConfigurationKeyValuePairs | Promise<ConfigurationValue | ConfigurationKeyValuePairs>; | ||
| export type ConfigurationMigration = { key: string; migrateFn: ConfigurationMigrationFn }; | ||
|
|
||
| export interface IConfigurationMigrationRegistry { | ||
| registerConfigurationMigrations(configurationMigrations: ConfigurationMigration[]): void; | ||
| } | ||
|
|
||
| class ConfigurationMigrationRegistryImpl implements IConfigurationMigrationRegistry { | ||
| readonly migrations: ConfigurationMigration[] = []; | ||
|
|
||
| private readonly _onDidRegisterConfigurationMigrations = new Emitter<ConfigurationMigration[]>(); | ||
| readonly onDidRegisterConfigurationMigration = this._onDidRegisterConfigurationMigrations.event; | ||
|
|
||
| registerConfigurationMigrations(configurationMigrations: ConfigurationMigration[]): void { | ||
| this.migrations.push(...configurationMigrations); | ||
| } | ||
| } | ||
|
|
||
| export const ConfigurationMigrationRegistry = new ConfigurationMigrationRegistryImpl(); |
Copilot
AI
Nov 14, 2025
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 event firing logic is missing in the registerConfigurationMigrations method. The original implementation (being removed from configurationService.ts) included this._onDidRegisterConfigurationMigrations.fire(configurationMigrations); after pushing migrations, but the new implementation in this file omits this line. This could cause listeners to not be notified when configuration migrations are registered.
| import { CodeBlockFormattingRules, EXISTING_CODE_MARKER } from '../panel/codeBlockFormattingRules'; | ||
| import { MathIntegrationRules } from '../panel/editorIntegrationRules'; | ||
| import { KeepGoingReminder } from './agentPrompt'; | ||
| import { isHiddenModelB, isHiddenModelC, isHiddenModelD } from '../../../../platform/endpoint/common/chatModelCapabilities'; |
Copilot
AI
Nov 14, 2025
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.
[nitpick] Unnecessary import reordering. The import statement for isHiddenModelB, isHiddenModelC, isHiddenModelD is being moved from line 8 to line 19 without any functional reason. This adds unnecessary diff noise. Consider keeping imports in their original position during a revert to minimize changes.
| import { AnyDiagnosticCompletionItem, AnyDiagnosticCompletionProvider } from './diagnosticsBasedCompletions/anyDiagnosticsCompletionProvider'; | ||
| import { AsyncDiagnosticCompletionProvider } from './diagnosticsBasedCompletions/asyncDiagnosticsCompletionProvider'; | ||
| import { Diagnostic, DiagnosticCompletionItem, DiagnosticInlineEditRequestLogContext, distanceToClosestDiagnostic, IDiagnosticCompletionProvider, log, logList, sortDiagnosticsByDistance } from './diagnosticsBasedCompletions/diagnosticsCompletions'; | ||
| import { ImportDiagnosticCompletionItem, ImportDiagnosticCompletionProvider } from './diagnosticsBasedCompletions/importDiagnosticsCompletionProvider'; | ||
| import { toInternalPosition } from '../utils/translations'; |
Copilot
AI
Nov 14, 2025
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.
[nitpick] Unnecessary import reordering. The import statement for toInternalPosition is being moved from line 36 to line 41 without any functional reason. This adds unnecessary diff noise. Consider keeping imports in their original position during a revert to minimize changes.
| import { AnyDiagnosticCompletionItem, AnyDiagnosticCompletionProvider } from './diagnosticsBasedCompletions/anyDiagnosticsCompletionProvider'; | |
| import { AsyncDiagnosticCompletionProvider } from './diagnosticsBasedCompletions/asyncDiagnosticsCompletionProvider'; | |
| import { Diagnostic, DiagnosticCompletionItem, DiagnosticInlineEditRequestLogContext, distanceToClosestDiagnostic, IDiagnosticCompletionProvider, log, logList, sortDiagnosticsByDistance } from './diagnosticsBasedCompletions/diagnosticsCompletions'; | |
| import { ImportDiagnosticCompletionItem, ImportDiagnosticCompletionProvider } from './diagnosticsBasedCompletions/importDiagnosticsCompletionProvider'; | |
| import { toInternalPosition } from '../utils/translations'; | |
| import { toInternalPosition } from '../utils/translations'; | |
| import { AnyDiagnosticCompletionItem, AnyDiagnosticCompletionProvider } from './diagnosticsBasedCompletions/anyDiagnosticsCompletionProvider'; | |
| import { AsyncDiagnosticCompletionProvider } from './diagnosticsBasedCompletions/asyncDiagnosticsCompletionProvider'; | |
| import { Diagnostic, DiagnosticCompletionItem, DiagnosticInlineEditRequestLogContext, distanceToClosestDiagnostic, IDiagnosticCompletionProvider, log, logList, sortDiagnosticsByDistance } from './diagnosticsBasedCompletions/diagnosticsCompletions'; | |
| import { ImportDiagnosticCompletionItem, ImportDiagnosticCompletionProvider } from './diagnosticsBasedCompletions/importDiagnosticsCompletionProvider'; |
Reverts #1717