-
Notifications
You must be signed in to change notification settings - Fork 213
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Handle context file searching using forward slashes on Windows #2215
Conversation
@dominiccooney another windows PR for ya :) |
I've opened #2241 too - not Windows specific, but depends on a function added in this PR to work. I'll rebase/merge that one after this lands (GitHub doesn't have a great way to do dependent PRs :-)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Beautiful.
Given your great comments I suggest we can simplify these conditionals to always replace / with the platform separator for a slight readability win.
If there were other Windows adaptations we were putting in those conditional blocks I would feel better about keeping the conditional blocks around.
if (path.sep === path.win32.sep) { | ||
relativePath = relativePath.replaceAll(path.posix.sep, path.win32.sep) | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (path.sep === path.win32.sep) { | |
relativePath = relativePath.replaceAll(path.posix.sep, path.win32.sep) | |
} | |
relativePath = relativePath.replaceAll(path.posix.sep, path.sep) | |
Co-authored-by: Dominic Cooney <dominic.cooney@sourcegraph.com>
Co-authored-by: Dominic Cooney <dominic.cooney@sourcegraph.com>
Fixes #2198
Test plan
pnpm test:e2e at-file
to run updated test, or: