Fix the "CodeQL: Open Referenced File" command for windows paths#979
Fix the "CodeQL: Open Referenced File" command for windows paths#979aeisenberg merged 1 commit intogithub:mainfrom
Conversation
shati-patel
left a comment
shati-patel
left a comment
There was a problem hiding this comment.
This is great! Works for me (on Windows) too 🎉
(I'll let someone else approve/merge too, mainly cause I'm not super confident with regex/TS yet myself 😅)
mgsium
changed the title
adityasharad
left a comment
adityasharad
left a comment
There was a problem hiding this comment.
Looks good. Minor suggestions only, either for now or a follow-up.
| let resolved; | ||
| try { | ||
| // replaces leading '/' in the query path if followed by a windows drive letter | ||
| resolved = await cliServer.resolveQlref(selectedQuery.path.replace(/^\/([a-zA-Z]:)/, '$1')); |
There was a problem hiding this comment.
Should we detect whether the current OS is Windows, and only apply this replacement in that case?
There was a problem hiding this comment.
My thinking was a regex check for the drive letter at the start would be enough, but if not I could change it to something like:
let queryPath: string = selectedQuery.path;
if (process.platform === 'win32') {
queryPath = queryPath.replace(/^\/([a-zA-Z]:)/, '$1');
}
resolved = await cliServer.resolveQlref(queryPath);There was a problem hiding this comment.
That looks nice and clear to me, together with your existing code comment. A file or directory on Unix with a colon is I think unlikely, but it's helpful to avoid that case entirely.
There was a problem hiding this comment.
Am I ok to add https://www.npmjs.com/package/platform as a dependency?
There was a problem hiding this comment.
I don't think you need that. You can use os.type() https://nodejs.dev/learn/the-nodejs-os-module#ostype I think this is sufficient.
There was a problem hiding this comment.
process.platform should work too.
extensions/ql-vscode/CHANGELOG.md
Outdated
| - Remove line about selecting a language from the dropdown when downloading database from LGTM. This makes the download progress visible when the popup is not expanded. [#894](https://github.com/github/vscode-codeql/issues/894) | ||
| - Fixed a bug where copying the version information fails when a CodeQL CLI cannot be found. [#958](https://github.com/github/vscode-codeql/pull/958) | ||
| - Fixed _CodeQL: Open Referenced File_ command for windows systems. [#979](https://github.com/github/vscode-codeql/pull/979) | ||
|
|
There was a problem hiding this comment.
This needs to be deleted.
| let queryPath: string = selectedQuery.path; | ||
| if (os.type() === 'Windows_NT') { | ||
| // replaces leading '/' in the query path if followed by a windows drive letter | ||
| queryPath = queryPath.replace(/^\/([a-zA-Z]:)/, '$1'); | ||
| } | ||
| resolved = await cliServer.resolveQlref(queryPath); |
There was a problem hiding this comment.
I just realized now that it might just work with selectedQuery.fsPath. Could you try this out?
| let queryPath: string = selectedQuery.path; | |
| if (os.type() === 'Windows_NT') { | |
| // replaces leading '/' in the query path if followed by a windows drive letter | |
| queryPath = queryPath.replace(/^\/([a-zA-Z]:)/, '$1'); | |
| } | |
| resolved = await cliServer.resolveQlref(queryPath); | |
| resolved = await cliServer.resolveQlref(selectedQuery.fsPath); |
Sorry for not thinking of this earlier.
There was a problem hiding this comment.
Appears to work well on Windows and MacOS, I'll commit the change but with a const
aeisenberg
left a comment
aeisenberg
left a comment
There was a problem hiding this comment.
Nice and simple. Even though the final change is small, I know there was a lot of work that went into this.
4 participants
Removes the leading '/' in a qlref query path if it's followed by a windows drive letter. Fixes #970
Checklist
@github/docs-content-codeqlhas been cc'd in all issues for UI or other user-facing changes made by this pull request.