refactor(copilot-rule): reuse toPosixPath and switch sameRelativePath to object params#1644
Conversation
… to object params Addresses the review findings tracked in dyoshikawa#1603: - Replace the local backslash regex with the shared toPosixPath utility, keeping the consecutive-slash collapse and explaining the WSL edge case it covers - Rename the parameter from path to p to avoid shadowing the node:path import - Move sameRelativePath from four positional strings to {dir, file} objects per the coding guidelines and update both call sites - Add forDeletion tests that pin down root detection with a trailing backslash separator and confirm non-root paths with mid-path backslashes stay non-root Refs dyoshikawa#1603.
dyoshikawa
left a comment
dyoshikawa
left a comment
There was a problem hiding this comment.
Nice, focused refactor — it matches the coding guidelines (object params, shared toPosixPath) cleanly, and the new forDeletion tests cover the mixed-separator edge case well. One small thing about the comment accuracy flagged inline.
| path.replace(/\\/g, "/").replace(/\/+/g, "/"); | ||
| // toPosixPath converts backslashes to forward slashes so paths compare equally | ||
| // on Windows and POSIX. The extra slash collapse covers a WSL/mixed-separator | ||
| // edge case where `node:path/posix.join` keeps a literal backslash inside an |
There was a problem hiding this comment.
Minor: this references node:path/posix.join, but the actual import at the top of the file is node:path (import { join } from "node:path"). On POSIX, node:path.join(".github\\", "x.md") keeps the backslash literal, which is what the slash-collapse handles. The PR description has the accurate phrasing — consider updating the comment to say node:path.join on POSIX so future readers do not get confused.
| // edge case where `node:path/posix.join` keeps a literal backslash inside an | ||
| // input segment (e.g. ".github\\") and produces ".github//instructions/x.md" | ||
| // after the backslash is rewritten. | ||
| const normalizeRelativePath = (p: string): string => toPosixPath(p).replace(/\/+/g, "/"); |
There was a problem hiding this comment.
Nit: p works for avoiding the node:path shadow, but relativePath would communicate intent a bit better. Feel free to ignore — toPosixPath in utils/file.ts already uses p, so this is at least consistent with the existing helper.
dyoshikawa
left a comment
dyoshikawa
left a comment
There was a problem hiding this comment.
Clean little refactor. The DRY consolidation onto toPosixPath, the path → p rename, and the {dir, file} object form on sameRelativePath all match the project's coding guidelines, and the slash-collapse stays put with a justification — that part is exactly right. CI is fully green across all four platforms.
Two things worth a second look before merge. The new comment block in copilot-rule.ts describes node:path/posix.join, but the file actually imports from node:path, and the example path it shows doesn't match the string that's actually joined at the call sites. And the new forDeletion tests are labeled "mixed separators" but don't actually mix / and \. Both are non-blocking nits, but worth tightening for future readers. Optionally, a symmetric fromFile case for the mid-path backslash would close the coverage gap that #1603 explicitly called out.
| path.replace(/\\/g, "/").replace(/\/+/g, "/"); | ||
| // toPosixPath converts backslashes to forward slashes so paths compare equally | ||
| // on Windows and POSIX. The extra slash collapse covers a WSL/mixed-separator | ||
| // edge case where `node:path/posix.join` keeps a literal backslash inside an |
There was a problem hiding this comment.
The comment refers to node:path/posix.join, but the import at the top of this file is node:path (the platform-aware variant), not node:path/posix. The behavior it describes — node:path.join(".github\\", "x.md") leaving the backslash literal — happens on POSIX where \ isn't a separator, and that's actually what we rely on here. Also the example output mentions .github//instructions/x.md, but the join at the call sites in this file produces .github//copilot-instructions.md for the root case. Mind tightening the comment so it matches the real import and the real string under test? Otherwise the next reader may go hunting for a path/posix import that doesn't exist.
| }); | ||
|
|
||
| describe("forDeletion", () => { | ||
| it("should mark root deletion target when separators are mixed", () => { |
There was a problem hiding this comment.
Small naming nit: both new cases say "separators are mixed" / "mixed separators", but neither input actually mixes / and \ in the same string — they use backslashes only (trailing here, mid-path below). Maybe rename to something like "should mark root deletion target when the dir uses a trailing backslash" and "should treat non-root paths with backslash separators as non-root"? Or, if you'd like to keep the phrasing, add one truly mixed case like relativeDirPath: ".github\\instructions/sub" — that matches what #1603 explicitly asked for.
| }); | ||
| }); | ||
|
|
||
| describe("forDeletion", () => { |
There was a problem hiding this comment.
Nice that forDeletion now has coverage for the backslash path. A symmetric test for fromFile with relativeDirPath: ".github\\instructions" would close the other half of the issue's coverage ask, since fromFile also routes through sameRelativePath. Not blocking — the helper is shared — but adding it would make the safety net explicit at both call sites.
… mid-path test Addresses dyoshikawa's review on dyoshikawa#1644: - Comment block on normalizeRelativePath previously said 'node:path/posix.join' and showed a multi-segment example, but the file imports node:path and sameRelativePath joins dir + single filename. Rewrote the comment so the module reference matches the import line and the example matches the actual call shape ('.github\\' + 'copilot-instructions.md'). - forDeletion tests renamed: 'when separators are mixed' → 'when relativeDirPath has a trailing backslash' / 'when relativeDirPath has a mid-path backslash'. They never actually mixed / and \\; the new labels describe what's really exercised. - Same rename applied to the existing fromFile 'should normalize separators…' test. - Added the optional symmetric fromFile mid-path backslash test the review flagged as closing the coverage gap dyoshikawa#1603 originally identified. Materializes the fixture directory at the literal '.github\\instructions' name since fromFile reads from the raw (un-normalized) relativeDirPath after the root check.
|
Thanks for the thorough read — pushed
CI green; 1062 → 1063 tests on the copilot-rule file (65 → 66 in the suite). |
|
@Chen17-sq Thank you! |
2 participants
Summary
Addresses the refactor checklist in #1603 for
src/features/rules/copilot-rule.ts:path.replace(/\\/g, "/")with the sharedtoPosixPathutility fromsrc/utils/file.ts, in line with the coding guideline that points totoPosixPathfor non-filesystem path normalization.replace(/\/+/g, "/")is still needed: on POSIX,node:path.join(".github\\", "x.md")keeps the trailing backslash literal, so aftertoPosixPathruns we get.github//x.md. A short comment now spells this out so future readers don't think the collapse is dead code.pathparameter name — Renamepath→pso it no longer shadows thenode:pathimport in the same file.sameRelativePath— Switch the four positional strings to{dir, file}objects per the project's coding guidelines, and update both call sites (fromFile,forDeletion).Test plan
pnpm cicheck(fmt + oxlint + eslint + tsgo + vitest 5552 tests + cspell + secretlint) passes locally on Node 22.forDeletiontests:relativeDirPathis".github\\".".github\\instructions") stay non-root..github\\continues to pass — behavior is preserved, only the helper implementation changes.Refs #1603.