Conversation
- Added new `Settings` interface to define all the configuration settings and their typings. - Refactored the `Configuration::getConfigurationValue` method to use the new `Settings` interface and automatically get the correct typing of the specified key from the interface. This also enables intellisense support for the settings keys. It also removes the requirement of hardcoding the type via generic typing when using the method. ie. `getConfigurationValue<string[]>()`. - Removed all generic typings from the `getConfigurationValue` method calls.
- Fixed the typing of `overrideDefaultLanguageMultiLineComments` setting to use the `Record` utility type. The setting is defined in the package.json as an object, but it has always been wrongly typed as an array (`string[]`). The utility type `Record` defines the types for the keys and values, assuming all keys and all values will be of the same typing, ie stringed keys and numeric values, in our case though it's stringed values. It's an easier way of typing it as `{ [ key: string ]: string }`.
- Removed the import for the `SupportUnsupportedLanguages` interface, as this isn't available until 1.2.0. This was wrongly added to this PR while adding it for the 1.2.0 version.
- Removed the generic typing from the `Configuration::getConfigurationValue` method call in extension.ts file.
- Changed the job name in check-ts.yml from `deploy` to `check-ts`, as it's confusing seeing `deploy` on GitHub PR's and actions when it actually isn't the deploy workflow. This workflow only checks ts errors.
- Merge `ExtensionData::createExtensionData` method into `ExtensionData::setExtensionData` method for simplicity. We once had a unified method before until I split them into the 2 aforementioned methods in PR #12 (commit b0a40aa). Not sure why they were split, maybe because I thought too many `this.extensionData.set` was duplication, but it actually isn't. So we now merge them back.
- Added new `ExtensionMetaData` interface for better typings and vscode intellisense. - Changed the `extensionData` property map to use typing of the new `ExtensionMetaData` interface. - Changed `get` and `getAll` methods to use the typing of `ExtensionMetaData` interface. This is instead of using return type of the old `createExtensionData` method, and still allows vscode intellisense of the data properties. It also is a better way to type the properties.
- Refactor `Configuration::setBladeComments` method to remove the duplicated blade and html comment arrays. We now only define the arrays once as a variable and use it in the return statement. - Fix `setBladeComments` method return type to vscode's `CharacterPair` type. Also typed the blade and html comments variables as `CharacterPair`.
- Fixed the type of the `value` parameter of `Configuration::updateConfigurationValue` method to use `unknown` instead of `any`. This is because `any` type just disables TypeScript's error reporting, while `unknown` is a type-safe usage for "anything". Using `unknown` type is generally preferred over `any`.
- Fixed the type for the `config` param to vscode's `LanguageConfiguration` instead of `any` in both `setMultiLineCommentLanguageDefinitions` and `setSingleLineCommentLanguageDefinitions` methods. - Typed the `langArray` variable in `setMultiLineCommentLanguageDefinitions` method as `string[]`. - Fixed `defaultMultiLineConfig` variable's type to vscode's `LanguageConfiguration` instead of `any` in `setLanguageConfiguration`.
Previously, the `lineComment` style check in `setSingleLineCommentLanguageDefinitions` used the string `includes` method to determine if the style "included" a `;` because they could also be `;;`. But since those are the only 2 strings that it can be, it makes more sense to just do a strict equality check for both instead. - Refactored `lineComment` style conditional to remove the `includes` method and use strict equality checks for `;` OR `;;`.
…ons`
- Added a new custom type, `SingleLineCommentStyle`, in the new `commentStyles` file to define 3 union strings for the single line styles.
- Fix `style` variable type in `setSingleLineCommentLanguageDefinitions` to the union type of the new `SingleLineCommentStyle` type and `null`. This gives more info to ts for what `style` should be, either 1 of the 3 strings or `null`. By typing the 3 style options ("//", "#", ";"), we are also enabling vscode's intellisense to auto complete the value. This helps for future development.
- Changed the default value for `style` variable to be `null` instead of an empty string. This is so that we don't need to type the new `SingleLineCommentStyle` as `""` for empty string which looks weird as a union type. It also provides a better understanding of why the variable might be null, ie. an unsupported style.
Also changed the style empty string checks to null checks.
- Fixed typing for the language config `lineComment` property.
The property used to be solely a string, but as of June 2025, VScode changed it to be a string, object or null, in the PR microsoft/vscode#243283, specifically commit microsoft/vscode@d9145a2. However, this has not been updated in the npm package `@types/vscode` by DefinitelyTyped, so ts still thinks it should be a string only. So when we try to access the `comment` property in the object, then ts spits out errors.
While I created an issue for it (DefinitelyTyped/DefinitelyTyped#74372), we need need to manually add the changes until @types/vscode is changed accordingly.
- Added new `LineComment` custom type in the new commentStyles file to be a union of `string`, `LineCommentConfig`, and `null`. This typing is inline with vscode's change, but as a custom type instead of inside an interface. Also typed the `lineComment` variable as this new type and cast the `config.comments.lineComment` as this type too, to avoid any ts errors.
- Added new `LineCommentConfig` interface in the new commentStyles file, to provide info for the object options. This is inline with vscode's change.
- Changed the conditional for the `lineComment` as an object to first check it is of type object and not null before checking if `comment` property exists in the object. This prevents ts erroring.
- Fixed ts type error `Type 'void | CharacterPair' is not assignable to type 'CharacterPair'` while trying to set blade comments in `setLanguageConfiguration` method. This probably happens because the `setBladeComments` method is returning void and being set directly to the lang config `blockComment` property which doesn't work. So to avoid this, we set the blade comments into a new variable instead and then only set the `blockComment` property if the conditional check of `bladeComments` variable is truthy, ie. has any value except null,undefined, etc.
- Removed the unused duplicate `style` variable in `setSingleLineCommentLanguageDefinitions` method.
- Fixed the `data` param in logger's `debug` method to be an optional param, so it no longer is required if we just want the `DEBUG` message in the logs.
- Added an optional `namespace` property in the `ExtensionMetaData` interface. - Move the configuration namespace value of the `name` property in `setExtensionData` to the new `namespace` property as it's value. - Set the `name` property n `setExtensionData` to the name in the packageJson instead of the configuration namespace. - Change all occurrences of getting the `name` property to the new `namespace` property since each occurrence is for the namespace and not the actual name.
- Added new `ExtensionData::setExtensionDiscoveryPaths` method to set the paths to installed extensions, and added it's method call to the constructor. - Moved the setting of the extension paths from `setExtensionData` into the new `setExtensionDiscoveryPaths` method. - Added new `ExtensionPaths` interface and moved all the related properties from the `ExtensionMetaData` interface into the new interface. - Added new `ExtensionData::extensionDiscoveryPaths` property to hold the Map of all the paths, and uses the `ExtensionPaths` interface. - Changed references to the `extensionData` property in `setExtensionDiscoveryPaths` method to use the new `extensionDiscoveryPaths` property. - Added `getExtensionDiscoveryPath` and `getAllExtensionDiscoveryPaths` methods to get a specific extension path by key or get all paths, respectively. - Changed all method calls to `get` extension paths in `Configuration` class to use the `getExtensionDiscoveryPath` method; and added the `getAllExtensionDiscoveryPaths` method call in the constructor.
- Move the creation of the extension ID from the `getExtensionPackageJsonData` method into the `setExtensionData` method. And set the id property of the `extensionData` Map directly as the id variable. This is instead of using the `packageJSON` object as the middleman, and using a custom property which is then used to set the `extensionData` Map. The ID doesn't need to be in the packageJSON object, so we can just create and set the id directly in the `setExtensionData` method.
- Move `settingsNamespace` creation from `getExtensionPackageJsonData` method into the `setExtensionData` method. So that the `namespace` property of the `extensionData` Map can be set directly using the variable instead of using the `packageJSON` object as a middleman again. We only add namespace to `packageJSON.contributes.configuration` as a way to eventually add it to the `extensionData` Map. This is redundant code.
- Moved the check for package.json file existing (`fs.existsSync`) in `Configruation::readExtensionsFromDirectory` method into the `readJsonFile` utility function. This is so that the util function can check if a JSON file exists before trying to read it. If file doesn't exist then it just returns `null`.
- Removed the `fs.existsSync` check and replaced it with a `null` check in the `readExtensionsFromDirectory` method.
- Refactored `readJsonFile` util function to accept a new `throwOnFileMissing` param with a default value of `true`. This param, along with it's accompanying logic, will throw an error if a file is missing (doesn't exist) and when the param is `true, instead of returning `null`.
This allows only specific usages of the util function to return `null` instead of error throwing when the param is set to `false`. Thus being backward compatibile with other usages.
- Added new `extensionPath` private property to the `ExtensionData` class to hold the absolute path of the extension. - Move the `extensionPath` creation from `getExtensionPackageJsonData` method into the `constructor`, and set it to the new class property of the same name. - Split the extension package.json path creation from the `readJsonFile` method call param into a new separate `packageJSONPath` variable in the `getExtensionPackageJsonData` method, and use this new variable as the param of `readJsonFile`. - Added new `extensionPath` property to the `ExtensionMetaData` interface to allow the `extensionData` Map to hold this new property, and added setting it's value into the `setExtensionData` method. - Removed the setting of the `extensionPath` into the `packageJson` object in `getExtensionPackageJsonData`, in favour of the new `extensionData` Map property of the same name.
- Added the `throwOnFileMissing` param of the readJsonFile util function call to as `false` in the `getExtensionPackageJsonData` method to allow the util function to return `null`. - Changed return types of the `getExtensionPackageJsonData` function to allow `null`. - Added a `packageJson` null check in the constructor to only allow running the `setExtensionData` method when `packageJsonData` is not `null`.
- Added `packageJson` property to the `ExtensionMetaData` interface with the type of `IPackageJson`.
- Added a new `ExtensionMetaDataValue` utility type to automatically get all types of the `ExtensionMetaData` and construct them as a union type. This is needed because typing the Map value as `string` will no longer work with the new `packageJson` property since it's not a string.
- Changed the typing of the `extensionData` Map value to use the new `ExtensionMetaDataValue` type.
- Added new `extensionPath` param to the constructor so that the class can be initialised with an optional extension path. The param is `null` be default.
- Added the new `extensionPath` param variable to set the class's `extensionPath` property. Using a nullish coalescing operator (`??`) on the variable, we set the property to the variable if it's a string, otherwise set it to our extension's path.
- Changed setting of the extension namespace property to only be set if the extension is ours - wrapped the code in an if statement in `setExtensionData` method. This is because we only need to know our extension's config namespace.
- Set the new `packageJson` property into the `extensionData` Map with the class's `packageJsonData` property as the value in `setExtensionData`.
- Changed all typings of any variables relating to an array of extensions in `Configuration` class to use the `ExtensionMetaData` interface. Because we have the required properties in place now we don't need to type them as `any`, we can now use the interface for a better developing experience.
In addition, we can also now remove all occurrences of the array object typing of `Array<{ id: string; extensionPath: string; packageJSON: IPackageJson }>` and replace it with the `ExtensionMetaData` interface as the type `ExtensionMetaData[]`. This makes it easier to handle, and removes all duplication.
- Changed `Configuration::readExtensionsFromDirectory` method to use the `ExtensionData` class to `getAll` data of each extension in the directory. This removes all now redundant code dealing with the package.json within the method, since it's handled in the `ExtensionData` class.
- Changed `ExtensionData::getAll` method to return a plain object instead of a readonly Map, this is so that we can use it in the `Configuration::readExtensionsFromDirectory` method without ts errors.
Also added a conditional that will return null if the `extensionData` map has a size of 0, ie. is empty.
The `//TODO: REMOVE THIS BEFORE RELEASE. FOR TESTING ONLY.` comment along with it's testing `userExtensionsPath` variable code was mistakenly added in commit be9d972. This was never supposed have seen the lights of git, and is only used in development to ensure it gets the right extensions path otherwise the development environment would get the wrong path. Fixed by removing the testing code, and uncommenting the real code.
- Added some code comments for clarity. - Improved type safety on a few variables, including the `config` in the `Configuration::setLanguageConfigDefinitions` method and the `event` of the `onDidChangeConfiguration` API in the `activate` method in extension.ts.
By default `trailingComma` option is set to `all` when not defined, and this made changes to extension.ts file by adding unnecessary trailing commas to function and method params. This could be seen in all the `showInformationMessage` method calls. By defining the option with the value `es5`, trailing commas will only be affective on arrays and objects, not functions and methods.
- Added proper typings for json objects.
- `JsonObject` interface to define a generic json object.
- `JsonValue` type to define the possible types of the json values.
- `JsonArray` type to define an array in json.
- Added interfaces to define the single-line and multi-line language definitions, which extend the `JsonObject`: `SingleLineLanguageDefinitions` and `MultiLineLanguageDefinitions`.
- Refactored `readJsonFile` util function to use the new `JsonObject` interface and `JsonValue` type. The function now passes a generic type `T` as it's return value, if the generic type isn't defined when calling the function, then it defaults to `JsonObject`. This ensures that specific types or interfaces can be defined as the return type that naturally is or extends a `JsonObject`. Eg. `readJsonFile<IPackageJson>()`.
- Refactored `writeJsonFile` util function to pass a generic type `T` as it's return value and it's `data` param type as `T`.
- Changed `obj` param type in `reconstructRegex` util function to `unknown.
- Changed `convertMapToReversedObject` util function return type to `JsonObject`.
- Changed `readJsonFile` function call in `ExtensionData::getExtensionPackageJsonData` method to define the generic return type as `IPackageJson`.
- Changed `Configuration::getLanguagesToSkip` method return type to `JsonArray`.
- Refactored `Configuration::writeCommentLanguageDefinitionsToJsonFile` method to properly type the code as the new `MultiLineLanguageDefinitions` and `SingleLineLanguageDefinitions` interfaces.
- Changed `defaultMultiLineConfig` variable in `Configuration::setLanguageConfiguration` to be cast as `vscode.LanguageConfiguration` to ensure the `readJsonFile` return type is of that type.
- Auto-formatting changes., like removal of commas.
- Changed the `readJsonFile` function call return type in `Configuration::logDebugInfo` method to use the `MultiLineLanguageDefinitions` and `SingleLineLanguageDefinitions` interfaces.
- Fixed returns for `writeJsonFile` util function. In the previous commit (a832c7e), the return type was changed from `any` to a passable generic T type. But despite always being typed as returning a value even before the commit, the function doesn't actually return a value. It only performs a side effect by writing to disk. Fixed by removing the return type altogether and the docblock return comment. Also changed it's function calls in `Configuration::writeCommentLanguageDefinitionsToJsonFile` method to remove the passing of the generic return types. - Changed `data` param of `writeJsonFile` util function to be of type `JsonValue` instead of `T`. - Fixed `convertMapToReversedObject` util function return type to a passable generic `T` type, and that defaults to return of `JsonObject` if the type is not defined when calling. This keeps it constraint to `JsonObject` type while also accepting a passable return type that is JSONable. And changed the function calls in `Configuration::writeCommentLanguageDefinitionsToJsonFile` method to pass the `SingleLineLanguageDefinitions` interface as it's generic return type instead of casting it as such. - Removed the type from the `result` variable declaration in `convertMapToReversedObject` util function and we now cast it `as T` at the end when returning to ensure it satisfies the new return typing.
- Fixed `reconstructRegex` return type to explicitly be typed as `RegExp` and added docblock types.
- Fixed `mergeArraysBy` util function call in `Configuration::setLanguageConfiguration` method to pass its generic return type as vscode's `AutoClosingPair` or `OnEnterRule`. The function is declared with a passable generic `T` type, but it wasn't being used, so we might as well use it and pass the return types.
- Added `string` types to these class properties:
- `autoGeneratedDir`
- `singleLineLangDefinitionFilePath`
- `multiLineLangDefinitionFilePath`
- Added return types to these methods:
- `configureCommentBlocks`
- `registerCommands`
- `getOverriddenMultiLineComment`
- `getLanguageConfig`
- Fixed some docblock formatting and comments.
- Added `JsonObject` type to the variables `extensionsPaths` and `env` in `logDebugInfo` method.
- Changed various docblock comments.
- Changed Map key typings to the union type of `"customSupportedLanguages" | "supportedLanguages"` instead of just `string` in the class properties `singleLineBlocksMap` and `multiLineBlocksMap`.
This is to be more verbose for what keys the Map can have. Since it's only these 2 keys, it makes sense to just use them as a union type. It also aids in vscode intellisense.
- Changed typings to use `SingleLineCommentStyle` type instead of just `string` for:
- `singleLineBlocksMap` class property inner Map value.
- `getSingleLineLanguages` method return type.
- `tempMap` variable in `setSingleLineCommentLanguageDefinitions` method.
- Docblock types.
- `singleLineStyle` param of `setLanguageConfiguration` method.
- `style` variable in `handleSingleLineBlock` method.
- Added new `ExtraSingleLineCommentStyles` type to define all the extra single-line comment styles like `///` and `##`.
This is needed to prevent ts errors like: "error TS2322: Type '";;"' is not assignable to type 'SingleLineCommentStyle'."
This was because changing the style types to use `SingleLineCommentStyle`, it affected the `handleSingleLineBlock` method usage of setting the extra styles directly on the `style` variable. Thus ts would rightly error that we're not allowed to set the style to anything other than specified in the union type.
This new type is the union of all the extra styles, and we just use it on the `style` variable as a union of both `SingleLineCommentStyle` and `ExtraSingleLineCommentStyles` to ensure the extra styles are allowed.
- Fixed the docblock param `channel` to the actual name of the param `channelOverride`.
- Added new type `LanguageId` which is just an alias for `string`. By typing relevant strings as `LanguageId`, it becomes more self documenting.
- Changed `languageConfigFilePaths` and `languageConfigs` class properties to define it's types on the property instead of defining the generic type of the `new Map` instance. This is just to keep things looking clean and inline with other code.
- Changed `string` types to use the new `LanguageId` in `Configuration` class for:
- `languageConfigFilePaths`, `languageConfigs`, `singleLineBlocksMap`, and `multiLineBlocksMap` class property Maps.
- Docblock types.
- `isLangIdDisabled`, `isLangIdMultiLineCommentOverridden`, `getOverriddenMultiLineComment` and `getLanguageConfig` method params.
- Various variables in many methods.
- `getMultiLineLanguages` and `getSingleLineLanguages` method returns.
There was a problem hiding this comment.
Pull request overview
This pull request significantly enhances type safety throughout the codebase by introducing comprehensive TypeScript interfaces and replacing generic types with specific, well-defined types. The changes focus on the Configuration class and related components, improving code maintainability and reducing potential runtime errors.
Changes:
- Introduced new interface files (
interfaces/utils.ts,interfaces/settings.ts,interfaces/extensionMetaData.ts,interfaces/commentStyles.ts) defining precise types for JSON values, language IDs, comment styles, settings, and extension metadata - Refactored
ExtensionDataclass to separate extension metadata from discovery paths, using strongly-typed Maps and introducing new helper methods - Updated
Configurationclass to use strongly-typed configuration access patterns, replacing genericstringtypes with specific types likeLanguageId,SingleLineCommentStyle, and interface-based settings access - Enhanced function signatures in utilities (
utils.ts) to support generic typing and better null handling - Updated configuration references from extension
nametonamespaceproperty - Added trailing comma enforcement to Prettier configuration and renamed TypeScript check workflow job for clarity
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| src/interfaces/utils.ts | Defines JSON type hierarchy and language definition structures |
| src/interfaces/settings.ts | Defines Settings interface for type-safe configuration access |
| src/interfaces/extensionMetaData.ts | Defines extension metadata and discovery path types |
| src/interfaces/commentStyles.ts | Defines single-line and multi-line comment style types |
| src/utils.ts | Enhanced readJsonFile with generic typing and null handling, improved function signatures |
| src/logger.ts | Made debug method's data parameter optional with proper documentation |
| src/extensionData.ts | Major refactor separating extension data and discovery paths with strongly-typed Maps |
| src/extension.ts | Updated to use namespace property and properly-typed ConfigurationChangeEvent |
| src/configuration.ts | Comprehensive type improvements with specific types replacing generic strings throughout |
| .prettierrc.json | Added trailing comma configuration for ES5 style |
| .github/workflows/check-ts.yml | Renamed workflow job from "deploy" to "check-ts" |
Comments suppressed due to low confidence (2)
src/configuration.ts:326
- The
getExtensionDiscoveryPathmethod returnsExtensionPaths[K] | undefined, butreadExtensionsFromDirectoryexpects astringparameter. When the path isundefined, it will cause runtime errors when trying to use it infs.readdirSync()at line 493.
Consider adding null/undefined checks before calling readExtensionsFromDirectory or ensuring these paths are always defined.
const windowsUserExtensionsPath = this.extensionData.getExtensionDiscoveryPath("WindowsUserExtensionsPathFromWsl");
const windowsBuiltInExtensionsPath = this.extensionData.getExtensionDiscoveryPath("WindowsBuiltInExtensionsPathFromWsl");
// Read the paths and create arrays of the extensions.
const windowsBuiltInExtensions = this.readExtensionsFromDirectory(windowsBuiltInExtensionsPath);
const windowsUserExtensions = this.readExtensionsFromDirectory(windowsUserExtensionsPath);
src/configuration.ts:337
- The
getExtensionDiscoveryPathmethod returnsExtensionPaths[K] | undefined, butreadExtensionsFromDirectoryexpects astringparameter. When the path isundefined, it will cause runtime errors when trying to use it infs.readdirSync()at line 493.
Consider adding null/undefined checks before calling readExtensionsFromDirectory or ensuring these paths are always defined.
const userExtensionsPath = this.extensionData.getExtensionDiscoveryPath("userExtensionsPath");
const builtInExtensionsPath = this.extensionData.getExtensionDiscoveryPath("builtInExtensionsPath");
// Read the paths and create arrays of the extensions.
const userExtensions = this.readExtensionsFromDirectory(userExtensionsPath);
const builtInExtensions = this.readExtensionsFromDirectory(builtInExtensionsPath);
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const builtInExtensionsPath = this.extensionData.getExtensionDiscoveryPath("builtInExtensionsPath"); | ||
|
|
||
| let extensionsPaths = {}; | ||
| let extensionsPaths: JsonObject = {}; | ||
|
|
||
| if (isWsl) { | ||
| // Get the Windows user and built-in extensions paths. | ||
| const windowsUserExtensionsPath = this.extensionData.get("WindowsUserExtensionsPathFromWsl"); | ||
| const windowsBuiltInExtensionsPath = this.extensionData.get("WindowsBuiltInExtensionsPathFromWsl"); | ||
| const windowsUserExtensionsPath = this.extensionData.getExtensionDiscoveryPath("WindowsUserExtensionsPathFromWsl"); | ||
| const windowsBuiltInExtensionsPath = this.extensionData.getExtensionDiscoveryPath("WindowsBuiltInExtensionsPathFromWsl"); | ||
|
|
||
| extensionsPaths = { | ||
| "Windows-installed Built-in Extensions Path": windowsBuiltInExtensionsPath, | ||
| "Windows-installed User Extensions Path": windowsUserExtensionsPath, | ||
| "WSL-installed Built-in Extensions Path": builtInExtensionsPath, | ||
| "WSL-installed User Extensions Path": this.extensionData.get("userExtensionsPath"), | ||
| "WSL-installed User Extensions Path": this.extensionData.getExtensionDiscoveryPath("userExtensionsPath"), |
There was a problem hiding this comment.
The getExtensionDiscoveryPath method returns ExtensionPaths[K] | undefined, but the returned values are being used directly in object assignment without null/undefined checks. This could result in undefined values being assigned to the object properties.
While this may not cause runtime errors, it could lead to confusing debug output. Consider adding checks or using fallback values to ensure meaningful debug information.
| private getLanguagesToSkip(): JsonArray { | ||
| const json = utils.readJsonFile(`${__dirname}/../../config/skip-languages.jsonc`); | ||
| return json.languages; | ||
| return json.languages as JsonArray; |
There was a problem hiding this comment.
The readJsonFile function can return null when throwOnFileMissing is false (as per the updated signature). However, this call doesn't pass the second parameter, so it defaults to true and will throw on missing files. But if the file is successfully read, attempting to access .languages on the result without checking if it exists could cause a runtime error if the JSON structure doesn't match expectations.
Consider adding a check to ensure the languages property exists on the returned JSON object before accessing it.
| return json.languages as JsonArray; | |
| if (!json || typeof json !== "object") { | |
| logger.error("Failed to read skip-languages.jsonc: invalid or null JSON object."); | |
| return []; | |
| } | |
| const maybeLanguages = (json as { languages?: unknown }).languages; | |
| if (!Array.isArray(maybeLanguages)) { | |
| logger.error("skip-languages.jsonc does not contain a valid 'languages' array."); | |
| return []; | |
| } | |
| return maybeLanguages as JsonArray; |
| disposables.push(...configureCommentBlocksDisposable, ...registerCommandsDisposable); | ||
|
|
||
| const extensionName = extensionData.get("name"); | ||
| const extensionName = extensionData.get("namespace"); |
There was a problem hiding this comment.
The namespace property may be undefined when retrieved from the ExtensionData, as it's optional in the ExtensionMetaData interface and only set for the "automatic-comment-blocks" extension (see line 79 in extensionData.ts). Using it directly in string templates without checking for undefined could result in "undefined" being interpolated into the strings.
Consider adding a null check or using a fallback value when namespace is undefined.
| const extensionName = extensionData.get("namespace"); | |
| const extensionName = String(extensionData.get("namespace")); |
| this.extensionDiscoveryPaths.set("WindowsUserExtensionsPathFromWsl", path.dirname(process.env.VSCODE_WSL_EXT_LOCATION!)); | ||
| this.extensionDiscoveryPaths.set("WindowsBuiltInExtensionsPathFromWsl", path.join(process.env.VSCODE_CWD!, "resources/app/extensions")); |
There was a problem hiding this comment.
Using the non-null assertion operator (!) on process.env.VSCODE_WSL_EXT_LOCATION assumes this environment variable is always defined when running in WSL. If this environment variable is not set, it will result in path.dirname(undefined), which could cause unexpected behavior.
Consider adding a check to ensure the environment variable exists before using it, or provide a fallback value.
| this.extensionDiscoveryPaths.set("WindowsUserExtensionsPathFromWsl", path.dirname(process.env.VSCODE_WSL_EXT_LOCATION!)); | |
| this.extensionDiscoveryPaths.set("WindowsBuiltInExtensionsPathFromWsl", path.join(process.env.VSCODE_CWD!, "resources/app/extensions")); | |
| const wslExtLocation = process.env.VSCODE_WSL_EXT_LOCATION; | |
| if (wslExtLocation) { | |
| this.extensionDiscoveryPaths.set("WindowsUserExtensionsPathFromWsl", path.dirname(wslExtLocation)); | |
| } | |
| const vscodeCwd = process.env.VSCODE_CWD; | |
| if (vscodeCwd) { | |
| this.extensionDiscoveryPaths.set("WindowsBuiltInExtensionsPathFromWsl", path.join(vscodeCwd, "resources/app/extensions")); | |
| } |
| this.extensionDiscoveryPaths.set("WindowsUserExtensionsPathFromWsl", path.dirname(process.env.VSCODE_WSL_EXT_LOCATION!)); | ||
| this.extensionDiscoveryPaths.set("WindowsBuiltInExtensionsPathFromWsl", path.join(process.env.VSCODE_CWD!, "resources/app/extensions")); |
There was a problem hiding this comment.
Using the non-null assertion operator (!) on process.env.VSCODE_CWD assumes this environment variable is always defined when running in WSL. If this environment variable is not set, it will result in path.join(undefined, ...), which could cause unexpected behavior.
Consider adding a check to ensure the environment variable exists before using it, or provide a fallback value.
| this.extensionDiscoveryPaths.set("WindowsUserExtensionsPathFromWsl", path.dirname(process.env.VSCODE_WSL_EXT_LOCATION!)); | |
| this.extensionDiscoveryPaths.set("WindowsBuiltInExtensionsPathFromWsl", path.join(process.env.VSCODE_CWD!, "resources/app/extensions")); | |
| const wslExtLocation = process.env.VSCODE_WSL_EXT_LOCATION; | |
| const vscodeCwd = process.env.VSCODE_CWD; | |
| if (wslExtLocation) { | |
| this.extensionDiscoveryPaths.set( | |
| "WindowsUserExtensionsPathFromWsl", | |
| path.dirname(wslExtLocation) | |
| ); | |
| } | |
| if (vscodeCwd) { | |
| this.extensionDiscoveryPaths.set( | |
| "WindowsBuiltInExtensionsPathFromWsl", | |
| path.join(vscodeCwd, "resources/app/extensions") | |
| ); | |
| } |
| @@ -906,7 +936,7 @@ export class Configuration { | |||
| } | |||
|
|
|||
| var indentedNewLine = "\n" + line.text.substring(0, line.text.search(indentRegex)); | |||
There was a problem hiding this comment.
Using var for variable declarations is inconsistent with modern TypeScript/JavaScript best practices. Throughout the rest of the codebase, let and const are used consistently. Consider using let instead of var for style and indentedNewLine to maintain consistency and avoid potential hoisting issues.
| rules.forEach((rule) => { | ||
| if (rule.action && "indent" in rule.action) { | ||
| // Convert JSON format to API format | ||
| // Usage of the `any` type is necessary here because `rule.action` |
There was a problem hiding this comment.
The comment on line 1061 is incomplete: "Usage of the any type is necessary here because rule.action" - the sentence is cut off and doesn't finish explaining why the any type is necessary. This could confuse future developers trying to understand the reasoning.
Consider completing the comment to fully explain why the any type is needed here.
| // Usage of the `any` type is necessary here because `rule.action` | |
| // Usage of the `any` type is necessary here because `rule.action` is typed as vscode.EnterAction, | |
| // which does not declare an `indent` property; the JSON configuration may still provide this property | |
| // at runtime, so we temporarily treat it as `any` to read and remove `indent` before setting indentAction. |
| */ | ||
| public getConfiguration(): vscode.WorkspaceConfiguration { | ||
| return vscode.workspace.getConfiguration(this.extensionData.get("name"), null); | ||
| return vscode.workspace.getConfiguration(this.extensionData.get("namespace"), null); |
There was a problem hiding this comment.
The namespace property is optional in the ExtensionMetaData interface (line 25) and may be undefined when the extension name is not "automatic-comment-blocks" (line 79). However, this method call requires a non-undefined value as the first parameter to vscode.workspace.getConfiguration().
When namespace is undefined, this will cause the configuration to be retrieved incorrectly or potentially throw an error. Consider handling the undefined case or ensuring namespace is always defined for this extension.
| return vscode.workspace.getConfiguration(this.extensionData.get("namespace"), null); | |
| const namespace = this.extensionData.get("namespace"); | |
| if (namespace) { | |
| return vscode.workspace.getConfiguration(namespace, null); | |
| } | |
| return vscode.workspace.getConfiguration(); |
- Added `addDevEnvVariables` util function to allow the extension to load the local `.env` file from the project root if it exists, using Node's `loadEnvFile` API, and load the environment variables into `process.env`. The `.env` file will only exist on the development machine and is crucial to properly test the extension in vscode's extension testing environment. This fixes the ability to mistakenly commit the dev testing path to git. As seen in commit be9d972 and removed in commit 9ff0857 of PR #20. With the .env file, this will never happen again. - Added the `addDevEnvVariables` function call to the top of the `extension.ts` file so it is the first thing it does and the env variables will be available in all files in development. - Added the usage of the `DEV_USER_EXTENSIONS_PATH` env variable to `ExtensionData::setExtensionDiscoveryPaths` method. So that when its available to use, it will override the `userExtensionsPath` variable with the path set in the env variable.
This pull request refactors and improves type safety in the
Configurationclass by introducing and applying more precise TypeScript types throughout the codebase. It also updates configuration access patterns and improves code readability. The most important changes are grouped below:Type Safety & Refactoring
stringtypes with more specific types such asLanguageId,SingleLineCommentStyle, andExtensionMetaDatafor various Maps, method parameters, and return types throughoutsrc/configuration.ts. This enhances type safety and code clarity.Configuration Access & Usage
namespaceinstead ofname, and updated methods to use strongly-typed keys and values for getting and updating configuration values.ExtensionData.Code Quality & Readability
Miscellaneous
.prettierrc.jsonto enforce trailing commas in ES5 style for improved code formatting consistency.deploytocheck-tsin.github/workflows/check-ts.ymlfor clarity.~ Summary generated by Copilot