Clean up dead code and other low-hanging tech debt#5515
Open
andyleejordan wants to merge 2 commits into
Open
Conversation
A few low-risk cleanups found while combing through the extension: - Remove the unused `getTimestampString()` from `src/utils.ts`. It's been dead since 2017 (`1b424000`), when "Add timestamps and log levels to all extension-side log messages" moved timestamping into the `Logger` and deleted its three call sites. It also had a latent bug: no zero-padding, so it rendered times like `[9:5:3]`. - Remove the unused `getWindowsSystemPowerShellPath()` from `src/platform.ts`. It's been dead since #2238 (2019), which introduced `PowerShellExeFinder` and deleted the `System32PowerShellPath` / `SysnativePowerShellPath` / `SysWow64PowerShellPath` constants that called it. The same path is now built inline in `findWinPS()`, with proper bitness handling in `getSystem32Path()`. - Replace `console.log` with `this.logger.writeDebug` in `writePidIfInDevMode` (`src/session.ts`), per our `ILogger` convention. While here, `await` the `fs.delete` it sits next to so the "Deleted PID file" message is actually true — previously it was a floating promise and the log ran before the delete resolved. - Point the PSScriptAnalyzer rule docs link at `learn.microsoft.com` instead of `docs.microsoft.com` (`src/features/CodeActions.ts`); the old URL just 301s there anyway. I left the Command Explorer's dead code alone since #5508 already rewrites that file. Compile, lint, and format all pass; the touched code has no test coverage so risk is low. Drafted by Copilot (Claude Opus 4.8). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR performs small “tech debt” cleanups in the VS Code PowerShell extension by removing long-dead helpers, aligning logging with ILogger, and updating an external documentation link.
Changes:
- Removed unused legacy helpers:
getTimestampString()(src/utils.ts) andgetWindowsSystemPowerShellPath()(src/platform.ts). - Updated
writePidIfInDevModecleanup logging to useILoggerand attempted to make PID-file deletion logging reflect completion (src/session.ts). - Updated the PSScriptAnalyzer rules documentation base URL to
learn.microsoft.com(src/features/CodeActions.ts).
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/utils.ts | Removes unused timestamp helper. |
| src/session.ts | Replaces console.log with ILogger and adjusts PID-file deletion behavior on process exit. |
| src/platform.ts | Removes unused Windows PowerShell path helper. |
| src/features/CodeActions.ts | Updates PSScriptAnalyzer documentation link base URL. |
The `onExited` listener in `writePidIfInDevMode` is `async`, but VS Code `Event` listeners are fire-and-forget: the returned promise is never awaited. As Copilot noted on #5515, that means a rejection from `fs.delete(...)` would surface as an unhandled promise rejection, and the debug log would be skipped on failure. I kept the `async`/`await` form for readability and wrapped the body in `try`/`catch` so nothing escapes: a fully-caught async body resolves normally, and any failure is now logged via `this.logger.writeError` instead of going unhandled. The original `main` version floated the delete promise and logged success unconditionally, swallowing errors. Drafted by Copilot (Claude Opus 4.8). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
PR Summary
A few low-risk cleanups found while combing through the extension:
getTimestampString()(src/utils.ts) — dead since 2017 (1b424000), when "Add timestamps and log levels to all extension-side log messages" moved timestamping into theLoggerand deleted its three call sites. It also had a latent bug: no zero-padding, so it rendered times like[9:5:3].getWindowsSystemPowerShellPath()(src/platform.ts) — dead since Discover new PowerShell installations, fix startup issue with Windows PowerShell #2238 (2019), which introducedPowerShellExeFinderand deleted theSystem32PowerShellPath/SysnativePowerShellPath/SysWow64PowerShellPathconstants that called it. The same path is now built inline infindWinPS(), with proper bitness handling ingetSystem32Path().ILoggerinstead ofconsole.loginwritePidIfInDevMode(src/session.ts), per our convention. While here,awaitthe adjacentfs.deleteso the "Deleted PID file" message is actually true — previously it was a floating promise and the log ran before the delete resolved.learn.microsoft.comfromdocs.microsoft.com(src/features/CodeActions.ts); the old URL just 301s there anyway.I left the Command Explorer's dead code alone since #5508 already rewrites that file.
compile,lint, andformatall pass; the touched code has no test coverage so risk is low.Drafted by Copilot (Claude Opus 4.8).
PR Checklist