Add Generate Qlhelp preview command #988

mgsium · 2021-10-31T23:03:37Z

Adds a "Generate Query Help" command which generates a .md file from a selected .qhelp file and opens it for preview in vscode. Implements #481

Checklist

CHANGELOG.md has been updated to incorporate all user visible changes made by this pull request.
Issues have been created for any UI or other user-facing changes made by this pull request.
@github/docs-content-codeql has been cc'd in all issues for UI or other user-facing changes made by this pull request.

shati-patel

This new command looks exciting! (Not a real review yet, just a small comment about terminology)

extensions/ql-vscode/src/cli.ts

adityasharad

Thanks! This is a good start to a much-awaited feature.

Most comments are about security (this command should only be run in a trusted workspace) and file/path handling. There is one longer suggestion about whether we should share the temporary file mechanism used for DIL and results files, on which I'd like opinions from the others on the team.

extensions/ql-vscode/src/cli.ts

extensions/ql-vscode/package.json

adityasharad · 2021-11-01T21:14:02Z

extensions/ql-vscode/src/extension.ts

+  async function previewQueryHelp(
+    selectedQuery: Uri
+  ): Promise<void> {
+    const path = selectedQuery ? selectedQuery.path : window.activeTextEditor!.document.uri.fsPath;


If this command is called from the context menu on the editor, I would expect selectedQuery to be exactly window.activeTextEditor.document.uri. Is that what you observed while testing this?

Yes, as far as I've observed they are equal. JSON.stringify(selectedQuery) === JSON.stringify(window.activeTextEditor.document.uri) also evaluates to true in the cases I've tested

In that case I think it should be enough to use selectedQuery.fsPath.

As in path = selectedQuery ? selectedQuery.fsPath : window.activeTextEditor.document.uri or just path = selectedQuery.fsPath?

Just path = selectedQuery.fsPath

path = selectedQuery.fsPath fails for me when I run the command from the palette

adityasharad · 2021-11-01T21:32:00Z

extensions/ql-vscode/src/extension.ts

+    const path = selectedQuery ? selectedQuery.path : window.activeTextEditor!.document.uri.fsPath;
+    if(path) {
+      // Create temporary directory
+      const tmpDir = tmp.dirSync();


Using a temporary directory is sensible. However we must be extra careful to clean it up properly, especially on Windows.

Have you seen the other uses of temporary directories in run-queries.ts? I suggest you follow that pattern as closely as possible. In particular, I recommend passing similar options to dirSync, and creating a dispose callback that can be registered with the extension via ctx.subscriptions.push(callback), so that the extension cleans up the temporary directories when it shuts down.

More detailed discussion: We could achieve more elegant code by adding an extra field to the QueryInfo class in run-queries.ts, which currently tracks a dilPath and resultsPath for each completed query. What do others think?

Advantages: It handles all the temporary directories already, associates each query with a number of temporary file paths, and doesn't repeat work if the file has already been generated.

Disadvantages: It only works for queries that have been run, which seems unreasonable for previewing query help. We would also have to track whether the .qhelp file had been modified.

adityasharad · 2021-11-01T21:42:22Z

extensions/ql-vscode/src/extension.ts

+        const errorMessage =  err.message.includes('Generating qhelp in markdown') ? (
+          `Could not generate markdown from ${path}: Bad formatting in .qhelp file.`
+        ) : `Could not open a preview of the generated file (${absolutePathToMd}).`;
+        void helpers.showAndLogErrorMessage(errorMessage);


I think this will suppress the error message from the CLI, which is hopefully useful. Could we include err in the message we construct here?

extensions/ql-vscode/package.json

extensions/ql-vscode/src/extension.ts

extensions/ql-vscode/CHANGELOG.md

shati-patel

Just tried this locally and found one more thing! Looks good otherwise 🎇

extensions/ql-vscode/src/extension.ts

shati-patel

Sorry to leave this hanging! I think this is mostly good to merge, though I've highlighted one of the earlier review comments which looks worth doing 😃

shati-patel · 2021-11-09T14:20:42Z

extensions/ql-vscode/src/extension.ts

+          const errorMessage =  err.message.includes('Generating qhelp in markdown') ? (
+            `Could not generate markdown from ${pathToQhelp}: Bad formatting in .qhelp file.`
+          ) : `Could not open a preview of the generated file (${absolutePathToMd}).`;
+          void helpers.showAndLogErrorMessage(errorMessage);


As @adityasharad mentioned above (in #988 (comment)), it would be nice to print err itself somewhere (perhaps using logger.log(...)?) 📝

adityasharad

Looking good, thank you. Final round of minor suggestions :)

extensions/ql-vscode/CHANGELOG.md

extensions/ql-vscode/src/extension.ts

adityasharad · 2021-11-15T19:20:19Z

extensions/ql-vscode/src/extension.ts

@@ -493,6 +494,32 @@ async function activateWithInstalledDistribution(
    }
  }

+  const tmpDir = tmp.dirSync({ prefix: 'qhelp_', keep: false, unsafeCleanup: true });


Minor: Rename this to qhelpTmpDir (or something similarly meaningful) so we can remember what it's for.

Also I think this pattern does the right thing on Windows (since we use it elsewhere in the extension), but I would not object to a second look from @shati-patel.

Also I think this pattern does the right thing on Windows (since we use it elsewhere in the extension), but I would not object to a second look from @shati-patel.

Looks good to me too! 😊

shati-patel

This looks good to me! Once you've tidied up the changelog this should be ready to merge 🥳

shati-patel · 2021-11-17T14:45:21Z

extensions/ql-vscode/CHANGELOG.md

+- Add a _CodeQL: Preview Query Help_ command to generate Markdown previews of `.qhelp` query help files. This command should only be run in trusted workspaces. See https://codeql.github.com/docs/codeql-cli/testing-query-help-files for more information about query help. [#988](https://github.com/github/vscode-codeql/pull/988)
 - Make "Open Referenced File" command accessible from the active editor menu. [#989](https://github.com/github/vscode-codeql/pull/989)
+- Add a _CodeQL: Preview Query Help File_ command to generate and preview query documentation in markdown format from a .qhelp file. [#988](https://github.com/github/vscode-codeql/pull/988)
+- Add a _CodeQL: Preview Query Help_ command to generate Markdown previews of `.qhelp` query help files. See https://codeql.github.com/docs/codeql-cli/testing-query-help-files for more information about query help. [#988](https://github.com/github/vscode-codeql/pull/988)


There are some outdated copies of the changelog entry here! The top one looks most correct, so you can delete the others :)

shati-patel · 2021-11-18T16:10:22Z

Nice work! Thank you 🎈 🥳

shati-patel reviewed Nov 1, 2021

View reviewed changes

extensions/ql-vscode/src/cli.ts Outdated Show resolved Hide resolved

mgsium force-pushed the qlhelp-preview branch from d03d7a1 to 8a4b6fd Compare November 1, 2021 20:38

mgsium marked this pull request as ready for review November 1, 2021 21:05

mgsium requested a review from a team as a code owner November 1, 2021 21:05

adityasharad reviewed Nov 1, 2021

View reviewed changes

shati-patel reviewed Nov 4, 2021

View reviewed changes

extensions/ql-vscode/src/extension.ts Outdated Show resolved Hide resolved

shati-patel reviewed Nov 9, 2021

View reviewed changes

adityasharad approved these changes Nov 15, 2021

View reviewed changes

mgsium force-pushed the qlhelp-preview branch from 76c63a8 to 3d198e7 Compare November 16, 2021 17:14

shati-patel approved these changes Nov 17, 2021

View reviewed changes

Add "Preview Query Help" command

7f5ef6f

mgsium force-pushed the qlhelp-preview branch from d5b29bb to 7f5ef6f Compare November 17, 2021 16:24

shati-patel merged commit 3743895 into github:main Nov 18, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Generate Qlhelp preview command #988

Add Generate Qlhelp preview command #988

mgsium commented Oct 31, 2021 •

edited

Loading

shati-patel left a comment

adityasharad left a comment •

edited

Loading

adityasharad Nov 1, 2021

mgsium Nov 9, 2021

adityasharad Nov 9, 2021

mgsium Nov 10, 2021

adityasharad Nov 10, 2021

mgsium Nov 10, 2021

adityasharad Nov 1, 2021

adityasharad Nov 1, 2021

shati-patel left a comment

shati-patel left a comment

shati-patel Nov 9, 2021

adityasharad left a comment

adityasharad Nov 15, 2021

adityasharad Nov 15, 2021

shati-patel Nov 17, 2021

shati-patel left a comment

shati-patel Nov 17, 2021

shati-patel commented Nov 18, 2021

Add Generate Qlhelp preview command #988

Add Generate Qlhelp preview command #988

Conversation

mgsium commented Oct 31, 2021 • edited Loading

Checklist

shati-patel left a comment

Choose a reason for hiding this comment

adityasharad left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shati-patel left a comment

Choose a reason for hiding this comment

shati-patel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adityasharad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shati-patel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shati-patel commented Nov 18, 2021

mgsium commented Oct 31, 2021 •

edited

Loading

adityasharad left a comment •

edited

Loading