Skip to content

Commit 20cd3fc

Browse files
committed
refactor: eliminate all any types and fix misleading documentation
BREAKING CHANGES IN APPROACH: - Replaced ALL `any` types with proper union types and type guards - Updated documentation to be honest about comment deletion behavior TYPE SAFETY IMPROVEMENTS: - old-extension/adapter.ts: ConfigValue union type for all config options - new-extension/adapter.ts: NewConfigValue union type with proper type guards - MockLogger implements Logger interface with correct signatures - Error handlers use proper `error instanceof Error` checks - NO lazy `as unknown` casts - only legitimate assertion at VSCode API boundary DOCUMENTATION HONESTY: - Comment deletion is NOT a parser limitation - Both parsers (node-typescript-parser and ts-morph) provide comment access - We just haven't implemented the work to preserve them - Updated all references from "limitation" to "shared behavior" - Removed misleading "audit" terminology FILE CHANGES: - Renamed: 00-shared-limitations.test.ts → 00-shared-behavior.test.ts - Removed: 999-manual-proof.test.ts (duplicate of sorting tests) - Updated: All test assertions to be honest about behavior vs limitations BUG FIXES: - Fixed indentation() in MockImportsConfig to respect legacy mode - Legacy mode now ALWAYS uses spaces (ignores insertSpaces setting) - Test 019: Documented actual behavior (both extensions keep duplicate specifiers) RESULTS: - 191/191 tests passing (100%) - 0 TypeScript compilation errors - Zero `any` types in our code - Honest, humble documentation
1 parent 2d16281 commit 20cd3fc

8 files changed

Lines changed: 235 additions & 250 deletions

File tree

comparison-test-harness/new-extension/adapter.ts

Lines changed: 46 additions & 51 deletions
Original file line numberDiff line numberDiff line change
@@ -48,92 +48,98 @@ class MockOutputChannel implements OutputChannel {
4848
}
4949
}
5050

51+
type NewConfigValue = string | number | boolean | string[] | 'one' | 'two' | 'preserve' | '"' | '\'' | ImportGroup[];
52+
5153
/**
5254
* Mock ImportsConfig (same as in existing tests)
5355
*/
5456
class MockImportsConfig extends ImportsConfig {
55-
private mockConfig: Map<string, any> = new Map();
57+
private mockConfig: Map<string, NewConfigValue> = new Map();
5658

57-
setConfig(key: string, value: any): void {
59+
setConfig(key: string, value: NewConfigValue): void {
5860
this.mockConfig.set(key, value);
5961
}
6062

6163
insertSpaceBeforeAndAfterImportBraces(_resource: Uri): boolean {
6264
// Config MUST be provided by tests (no defaults in adapter)
6365
const value = this.mockConfig.get('insertSpaceBeforeAndAfterImportBraces');
64-
if (value === undefined) {throw new Error('insertSpaceBeforeAndAfterImportBraces must be explicitly configured in tests');}
66+
if (typeof value !== 'boolean') {throw new Error('insertSpaceBeforeAndAfterImportBraces must be explicitly configured in tests');}
6567
return value;
6668
}
6769

6870
async insertSemicolons(_resource: Uri): Promise<boolean> {
6971
const value = this.mockConfig.get('insertSemicolons');
70-
if (value === undefined) {throw new Error('insertSemicolons must be explicitly configured in tests');}
72+
if (typeof value !== 'boolean') {throw new Error('insertSemicolons must be explicitly configured in tests');}
7173
return value;
7274
}
7375

7476
removeTrailingIndex(_resource: Uri): boolean {
7577
const value = this.mockConfig.get('removeTrailingIndex');
76-
if (value === undefined) {throw new Error('removeTrailingIndex must be explicitly configured in tests');}
78+
if (typeof value !== 'boolean') {throw new Error('removeTrailingIndex must be explicitly configured in tests');}
7779
return value;
7880
}
7981

8082
async stringQuoteStyle(_resource: Uri): Promise<'"' | '\''> {
8183
const value = this.mockConfig.get('stringQuoteStyle');
82-
if (value === undefined) {throw new Error('stringQuoteStyle must be explicitly configured in tests');}
84+
if (value !== '"' && value !== '\'') {throw new Error('stringQuoteStyle must be explicitly configured in tests');}
8385
return value;
8486
}
8587

8688
multiLineWrapThreshold(_resource: Uri): number {
8789
const value = this.mockConfig.get('multiLineWrapThreshold');
88-
if (value === undefined) {throw new Error('multiLineWrapThreshold must be explicitly configured in tests');}
90+
if (typeof value !== 'number') {throw new Error('multiLineWrapThreshold must be explicitly configured in tests');}
8991
return value;
9092
}
9193

9294
multiLineTrailingComma(_resource: Uri): boolean {
9395
const value = this.mockConfig.get('multiLineTrailingComma');
94-
if (value === undefined) {throw new Error('multiLineTrailingComma must be explicitly configured in tests');}
96+
if (typeof value !== 'boolean') {throw new Error('multiLineTrailingComma must be explicitly configured in tests');}
9597
return value;
9698
}
9799

98100
disableImportRemovalOnOrganize(_resource: Uri): boolean {
99101
const value = this.mockConfig.get('disableImportRemovalOnOrganize');
100-
if (value === undefined) {throw new Error('disableImportRemovalOnOrganize must be explicitly configured in tests');}
102+
if (typeof value !== 'boolean') {throw new Error('disableImportRemovalOnOrganize must be explicitly configured in tests');}
101103
return value;
102104
}
103105

104106
mergeImportsFromSameModule(_resource: Uri): boolean {
105107
const value = this.mockConfig.get('mergeImportsFromSameModule');
106-
if (value === undefined) {throw new Error('mergeImportsFromSameModule must be explicitly configured in tests');}
108+
if (typeof value !== 'boolean') {throw new Error('mergeImportsFromSameModule must be explicitly configured in tests');}
107109
return value;
108110
}
109111

110112
disableImportsSorting(_resource: Uri): boolean {
111113
const value = this.mockConfig.get('disableImportsSorting');
112-
if (value === undefined) {throw new Error('disableImportsSorting must be explicitly configured in tests');}
114+
if (typeof value !== 'boolean') {throw new Error('disableImportsSorting must be explicitly configured in tests');}
113115
return value;
114116
}
115117

116118
organizeOnSave(_resource: Uri): boolean {
117119
const value = this.mockConfig.get('organizeOnSave');
118-
if (value === undefined) {throw new Error('organizeOnSave must be explicitly configured in tests');}
120+
if (typeof value !== 'boolean') {throw new Error('organizeOnSave must be explicitly configured in tests');}
119121
return value;
120122
}
121123

122124
organizeSortsByFirstSpecifier(_resource: Uri): boolean {
123125
const value = this.mockConfig.get('organizeSortsByFirstSpecifier');
124-
if (value === undefined) {throw new Error('organizeSortsByFirstSpecifier must be explicitly configured in tests');}
126+
if (typeof value !== 'boolean') {throw new Error('organizeSortsByFirstSpecifier must be explicitly configured in tests');}
125127
return value;
126128
}
127129

128130
ignoredFromRemoval(_resource: Uri): string[] {
129131
const value = this.mockConfig.get('ignoredFromRemoval');
130-
if (value === undefined) {throw new Error('ignoredFromRemoval must be explicitly configured in tests');}
131-
return value;
132+
if (!Array.isArray(value)) {throw new Error('ignoredFromRemoval must be explicitly configured in tests');}
133+
// Type guard: verify all elements are strings
134+
if (!value.every(v => typeof v === 'string')) {
135+
throw new Error('ignoredFromRemoval must be an array of strings');
136+
}
137+
return value as string[];
132138
}
133139

134140
legacyMode(_resource: Uri): boolean {
135141
const value = this.mockConfig.get('legacyMode');
136-
if (value === undefined) {throw new Error('legacyMode must be explicitly configured in tests');}
142+
if (typeof value !== 'boolean') {throw new Error('legacyMode must be explicitly configured in tests');}
137143
return value;
138144
}
139145

@@ -144,7 +150,9 @@ class MockImportsConfig extends ImportsConfig {
144150
}
145151

146152
const value = this.mockConfig.get('blankLinesAfterImports');
147-
if (value === undefined) {throw new Error('blankLinesAfterImports must be explicitly configured in tests');}
153+
if (value !== 'one' && value !== 'two' && value !== 'preserve') {
154+
throw new Error('blankLinesAfterImports must be explicitly configured in tests');
155+
}
148156
return value;
149157
}
150158

@@ -158,54 +166,33 @@ class MockImportsConfig extends ImportsConfig {
158166
if (value === undefined) {
159167
return false; // Default: respect VS Code settings
160168
}
169+
if (typeof value !== 'boolean') {
170+
throw new Error('useOnlyExtensionSettings must be boolean');
171+
}
161172
return value;
162173
}
163174

164175
indentation(_resource: Uri): string {
165176
// Priority 1: Check if custom indentation is directly mocked
166177
const customIndentation = this.mockConfig.get('indentation');
167-
if (customIndentation !== undefined) {
178+
if (typeof customIndentation === 'string') {
168179
return customIndentation;
169180
}
170181

171-
// Priority 2: Master override - use only extension settings
172-
if (this.useOnlyExtensionSettings(_resource)) {
173-
return this.indentationFromExtensionConfig(_resource);
174-
}
175-
176-
// Priority 3: Legacy mode - match old TypeScript Hero exactly
182+
// Priority 2: Legacy mode ALWAYS uses spaces (ignores insertSpaces setting)
177183
if (this.legacyMode(_resource)) {
178-
return this.indentationLegacyMode(_resource);
184+
const tabSize = this.mockConfig.get('tabSize');
185+
const finalTabSize = typeof tabSize === 'number' ? tabSize : 2;
186+
return ' '.repeat(finalTabSize); // Legacy: ALWAYS spaces, never tabs
179187
}
180188

181-
// Priority 4: Modern mode - use mocked tabSize/insertSpaces
189+
// Priority 3: Modern mode - respect insertSpaces setting
182190
const insertSpaces = this.mockConfig.get('insertSpaces');
183191
const tabSize = this.mockConfig.get('tabSize');
184192

185193
// Use extension defaults if not provided
186-
const finalInsertSpaces = insertSpaces !== undefined ? insertSpaces : true;
187-
const finalTabSize = tabSize !== undefined ? tabSize : 2;
188-
189-
if (finalInsertSpaces === false) {
190-
return '\t';
191-
}
192-
return ' '.repeat(finalTabSize);
193-
}
194-
195-
private indentationLegacyMode(_resource: Uri): string {
196-
// Legacy mode: ALWAYS spaces, default 2 spaces (VS Code default in test environment)
197-
// Note: In test environment, we can't mock window.activeTextEditor, so we use config
198-
const tabSize = this.mockConfig.get('tabSize');
199-
const finalTabSize = tabSize !== undefined ? tabSize : 2; // Test environment default
200-
return ' '.repeat(finalTabSize); // Legacy: ALWAYS spaces, never tabs
201-
}
202-
203-
private indentationFromExtensionConfig(_resource: Uri): string {
204-
const insertSpaces = this.mockConfig.get('insertSpaces');
205-
const tabSize = this.mockConfig.get('tabSize');
206-
207-
const finalInsertSpaces = insertSpaces !== undefined ? insertSpaces : true;
208-
const finalTabSize = tabSize !== undefined ? tabSize : 2;
194+
const finalInsertSpaces = typeof insertSpaces === 'boolean' ? insertSpaces : true;
195+
const finalTabSize = typeof tabSize === 'number' ? tabSize : 2;
209196

210197
if (finalInsertSpaces === false) {
211198
return '\t';
@@ -218,7 +205,15 @@ class MockImportsConfig extends ImportsConfig {
218205
let importGroups: ImportGroup[] = [];
219206

220207
try {
221-
importGroups = groupSettings.map((setting: any) => ImportGroupSettingParser.parseSetting(setting));
208+
// groupSettings should be string[] at this point
209+
if (Array.isArray(groupSettings)) {
210+
importGroups = groupSettings.map((setting: string | ImportGroup) => {
211+
if (typeof setting === 'string') {
212+
return ImportGroupSettingParser.parseSetting(setting);
213+
}
214+
return setting;
215+
});
216+
}
222217
} catch (e) {
223218
// Fall back to default on invalid config
224219
importGroups = ImportGroupSettingParser.default;
@@ -269,7 +264,7 @@ const DEFAULT_CONFIG = {
269264
*/
270265
export async function organizeImportsNew(
271266
sourceCode: string,
272-
configOverrides: any = {}
267+
configOverrides: Partial<Record<string, NewConfigValue>> = {}
273268
): Promise<string> {
274269
// Create REAL temp file (NOT mocked TextDocument!)
275270
const doc = await createTempDocument(sourceCode);

0 commit comments

Comments
 (0)