feat(vscode): remove unnecessary logger code #685
Conversation
✅ Deploy Preview for rstest-dev ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
|
claneo
force-pushed
the
vscode-logger
branch
from
994d3d1 to
ff35a84
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR simplifies the logger implementation by replacing custom logging logic with VSCode's built-in LogOutputChannel API. The change removes the custom logLevel configuration option and associated event handling, relying instead on VSCode's native log level management.
- Replaced custom
OutputChannelwithvscode.LogOutputChannelwhich provides built-in log level filtering - Removed
rstest.logLevelconfiguration option and all related code - Removed test coverage for the deprecated
logLevelconfiguration
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| packages/vscode/src/logger.ts | Refactored to use vscode.LogOutputChannel with built-in log methods (trace, debug, info, warn, error) instead of custom formatting and level filtering |
| packages/vscode/src/config.ts | Removed LogLevel type, logLevel property from ExtensionConfig, and associated validation logic |
| packages/vscode/src/extension.ts | Removed logger from context subscriptions (note: this may cause improper cleanup) |
| packages/vscode/package.json | Removed rstest.logLevel configuration property from extension manifest |
| packages/vscode/tests/suite/config.test.ts | Removed test for rstest.logLevel configuration and unused waitForConfigValue import |
| packages/vscode/tests/fixtures/.vscode/settings.json | Removed rstest.logLevel setting from test fixtures |
| packages/vscode/README.md | Updated configuration table to remove rstest.logLevel and add rstest.rstestPackagePath documentation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| this.context = context; | ||
| this.ctrl = vscode.tests.createTestController('rstest', 'Rstest'); | ||
| context.subscriptions.push(this.ctrl, logger); | ||
| context.subscriptions.push(this.ctrl); |
There was a problem hiding this comment.
The logger singleton is still being used throughout the codebase (in extension.ts, master.ts, testTree.ts), but it's no longer being disposed. Since the logger creates a vscode.LogOutputChannel resource that should be cleaned up, the logger should still be added to context.subscriptions to ensure proper disposal when the extension is deactivated.
Suggestion: Change this line to context.subscriptions.push(this.ctrl, logger);
| context.subscriptions.push(this.ctrl); | |
| context.subscriptions.push(this.ctrl, logger); |
fi3ework
left a comment
fi3ework
left a comment
There was a problem hiding this comment.
wow, didn't know the built-in log level before, thank you.
3 participants
Summary
Related Links
Checklist