Fix the "CodeQL: Open Referenced File" command for windows paths#979
Uh oh!
There was an error while loading. Please reload this page.
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 😅)
Uh oh!
There was an error while loading. Please reload this page.
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.
| letresolved; | ||
| try{ | ||
| // replaces leading '/' in the query path if followed by a windows drive letter | ||
| resolved=awaitcliServer.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:
letqueryPath: string=selectedQuery.path;if(process.platform==='win32'){queryPath=queryPath.replace(/^\/([a-zA-Z]:)/,'$1');}resolved=awaitcliServer.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.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
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) | ||
| letqueryPath: 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=awaitcliServer.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?
| letqueryPath: 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=awaitcliServer.resolveQlref(queryPath); | |
| resolved=awaitcliServer.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.