Add the commandRunner by aeisenberg · Pull Request #590 · github/vscode-codeql

aeisenberg · 2020-09-25T17:08:04Z

The commandRunner wraps all vscode command registrations. It provides
uniform error handling and an optional progress monitor.

In general, progress monitors should only be created by the
commandRunner and passed through to the locations that use it.

Resolves #464.
Resolves #534.

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-dsp has been cc'd in all issues for UI or other user-facing changes made by this pull request.

aeisenberg · 2020-09-25T17:10:00Z

This is still a draft, but it is good enough to have a look and get some feedback. I need to do some more manual testing as well as add some unit tests.

aeisenberg · 2020-09-25T20:02:13Z

  Uri,
-  ProgressOptions,
-  ProgressLocation,
+  CancellationToken,


This file is mostly about disentangling all the error handling and progress reporting.

adityasharad · 2020-10-07T23:26:07Z

-      KeyType.DefinitionQuery,
-      (src, _dest) => src === uriString
-    );
+    return await withProgress({


Here and elsewhere: This return await can just be a return right? Otherwise we'll await the outcome of the promise, then wrap up the result in a second promise anyway, and return that.

It's redundant. I can remove. Though, I don't think the behaviour is any different due to hoisting of promises. It is essentially this snippet:

async function a() { throw new Error('1') } async function b1() { return await a() } async function b2() { return a() } async function c() { try { await b1() } catch (e) { console.log(e) } try { await b2() } catch (e) { console.log(e) } } c()

Both errors are properly captured.

adityasharad · 2020-10-07T23:43:52Z

+ * If there is no progress monitor, then only extra command arguments are passed in.
+ *
+ * @param commandId The ID of the command to register.
+ * @param task The task to run. If passing taskOptions, then this task must be a `ProgressTask`.


Suggested change

* @param task The task to run. If passing taskOptions, then this task must be a `ProgressTask`.

* @param task The task to run. If passing `progressOptions`, then this task must be a `ProgressTask`.

adityasharad · 2020-10-07T23:45:38Z

+      } else if (e instanceof Error) {
+        showAndLogErrorMessage(e.message);
+      } else {
+        throw e;


Should we always handle this (show and log) to avoid uncaught promise rejections at the top level?

Our code should always be throwing errors. If a non-error is thrown, then that is just...strange and something unexpected is happening. I don't feel strongly about this since it shouldn't happen. I can remove.

Need to handle the case where e.message is undefined.

adityasharad · 2020-10-07T23:47:45Z

+  task: ProgressTask<R> | NoProgressTask,
+  progressOptions?: ProgressOptions


I wonder whether it would be safer to split this into two variants:

accept task: ProgressTask<R> and progressOptions

accept task: NoProgressTask and no progressOptions.

That way it's obvious whenever we attempt to write new code that uses this.

So, commandRunner and commandRunnerWithProgress? I guess this would allow us to ensure we send the proper number of arguments for each style.

That sounds fine. I don't know whether TS overloading rules will let you have the same name with different types and arities.

It's actually a good thing that I changed to different names. It uncovered one instance when I was calling a command handler with the wrong arguments.

aeisenberg · 2020-10-08T01:27:25Z

Thanks for the review. I rebased and didn't address any of your comments yet. I will tomorrow.

The commandRunner wraps all vscode command registrations. It provides uniform error handling and an optional progress monitor. In general, progress monitors should only be created by the commandRunner and passed through to the locations that use it.

When a user runs multiple queries on a non-upgraded database, ensure that only one dialog appears for upgrade. This commit also migrates the upgrades.ts file to using the passed-in cancellation token and progress monitor. This ensures that cancelling a database upgrade command will also cancel out of any wrapper operations. Fixes github#534

Co-authored-by: Shati Patel <42641846+shati-patel@users.noreply.github.com>

adityasharad · 2020-10-08T23:22:27Z

+ * There are two ways to invoke the command task: with or without a progress monitor
+ * If progressOptions are passed in, then the command task will run with a progress monitor
+ * Otherwise, no progress monitor will be used.
+ *
+ * If a task is run with a progress monitor, the first two arguments to the task are always
+ * the progress callback, and the cancellation token. And this is followed by any extra command arguments
+ * (eg- selection, multiselection, ...).
+ *
+ * If there is no progress monitor, then only extra command arguments are passed in.


Split this across the two methods, so each documents the specific case it handles.

Split commandRunner into two functions: commandRunner and commandRunnerWithProgress. Also, take advantage of default arguments for ProgressOptions. And updates changelog.

aeisenberg requested a review from jcreedcmu September 25, 2020 17:08

aeisenberg marked this pull request as draft September 25, 2020 17:08

aeisenberg commented Sep 25, 2020

View reviewed changes

aeisenberg force-pushed the aeisenberg/command-runner branch 2 times, most recently from 28c710c to 81d51bb Compare September 29, 2020 16:49

aeisenberg marked this pull request as ready for review September 29, 2020 17:05

shati-patel reviewed Sep 29, 2020

View reviewed changes

Comment thread extensions/ql-vscode/CHANGELOG.md Outdated

aeisenberg force-pushed the aeisenberg/command-runner branch 2 times, most recently from f40f349 to 59ef027 Compare September 30, 2020 23:51

adityasharad reviewed Oct 7, 2020

View reviewed changes

aeisenberg force-pushed the aeisenberg/command-runner branch from 59ef027 to 4ab5a03 Compare October 8, 2020 01:24

aeisenberg force-pushed the aeisenberg/command-runner branch from c44581f to 7cde605 Compare October 8, 2020 19:01

aeisenberg and others added 3 commits October 8, 2020 12:33

Add the commandRunner

264dba4

The commandRunner wraps all vscode command registrations. It provides uniform error handling and an optional progress monitor. In general, progress monitors should only be created by the commandRunner and passed through to the locations that use it.

Update changelog

7e7623b

Co-authored-by: Shati Patel <42641846+shati-patel@users.noreply.github.com>

aeisenberg force-pushed the aeisenberg/command-runner branch from 7cde605 to e9ed1e5 Compare October 8, 2020 19:34

adityasharad reviewed Oct 8, 2020

View reviewed changes

Refactor the commandRunner

ad06780

Split commandRunner into two functions: commandRunner and commandRunnerWithProgress. Also, take advantage of default arguments for ProgressOptions. And updates changelog.

aeisenberg force-pushed the aeisenberg/command-runner branch from e9ed1e5 to ad06780 Compare October 9, 2020 15:37

adityasharad approved these changes Oct 9, 2020

View reviewed changes

aeisenberg merged commit 8e817ee into github:main Oct 9, 2020

aeisenberg deleted the aeisenberg/command-runner branch November 24, 2020 22:31

	* @param task The task to run. If passing taskOptions, then this task must be a `ProgressTask`.
	* @param task The task to run. If passing `progressOptions`, then this task must be a `ProgressTask`.

		task: ProgressTask<R> \| NoProgressTask,
		progressOptions?: ProgressOptions

Conversation

aeisenberg commented Sep 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

aeisenberg commented Sep 25, 2020

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aeisenberg commented Oct 8, 2020

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

aeisenberg commented Sep 25, 2020 •

edited

Loading