add remove option on parser failed notification#181
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds functionality to allow users to remove corrupted parser installations when loading fails. Previously, users could only retry or dismiss error messages when parser installation failed. Now, users can choose to remove a broken parser, with different options presented based on whether the failure was during download or loading.
Key changes:
- Added "Remove" option to error notifications when parser loading fails
- Implemented
removeLanguageandaskRemoveLanguagefunctions to handle parser removal - Removed unused
parserFinishedInitpromise that was previously used for initialization synchronization - Enhanced error handling with structured error types that distinguish between download and load failures
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Installer.ts | Added GetLanguageError type, removeLanguage and askRemoveLanguage functions; updated getLanguage to return structured errors; removed parserFinishedInit references |
| src/extension.ts | Updated error handling to use new askRemoveLanguage function; added conditional logic to handle download vs load failures; removed parserFinishedInit export |
| src/editor/CodeBlocksEditorProvider.ts | Updated error dialog to show "Remove" option for load failures and use askRemoveLanguage; improved error message formatting |
| src/FileTree.ts | Removed parserFinishedInit reference |
| src/test/suite/Installer.test.ts | Updated error message to use JSON.stringify for consistency |
| CHANGELOG.md | Documented the new "Remove" option feature |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| const languageId = activeDocument.languageId; | ||
| const language = await Installer.getLanguage(parsersDir, languageId); | ||
|
|
||
| // sup-optimal conditional to make tsc happy |
There was a problem hiding this comment.
Corrected spelling of 'sup-optimal' to 'sub-optimal'.
| // sup-optimal conditional to make tsc happy | |
| // sub-optimal conditional to make tsc happy |
| ); | ||
|
|
||
| if (doRemove === "Remove") { | ||
| removeLanguage(parsersDir, languageId); |
There was a problem hiding this comment.
Missing await keyword. The removeLanguage function performs file system operations (rmSync) that could throw errors. While rmSync is synchronous, the call should be awaited or the function should handle potential errors, especially since this is called from an async context where error handling would be expected.
| removeLanguage(parsersDir, languageId); | |
| try { | |
| removeLanguage(parsersDir, languageId); | |
| } catch (err) { | |
| const logger = getLogger(); | |
| logger.log(`Failed to remove parser for ${languageId}: ${err}`); | |
| vscode.window.showErrorMessage(`Failed to remove parser for ${languageId}: ${err}`); | |
| } |
| if (choice !== "Retry") { | ||
|
|
||
| if (choice === "Remove") { | ||
| Installer.removeLanguage(this.extensionParsersDirPath, languageId); |
There was a problem hiding this comment.
The removeLanguage function can throw errors during file system operations, but this call is not wrapped in error handling. Consider adding try-catch or propagating errors to inform the user if removal fails.
| Installer.removeLanguage(this.extensionParsersDirPath, languageId); | |
| try { | |
| Installer.removeLanguage(this.extensionParsersDirPath, languageId); | |
| } catch (err) { | |
| await vscode.window.showErrorMessage( | |
| `Failed to remove language '${languageId}': ${err instanceof Error ? err.message : String(err)}` | |
| ); | |
| } |
2d469e9 to
4c746a7
Compare
No description provided.