NEW (Extension) @W-14001840@: Add ability to cancel graph engine runs #70
Conversation
… change coverage thresholds (#71)
There was a problem hiding this comment.
Have these messages gone through Text Review?
There was a problem hiding this comment.
Yes, I am working with @jshackell-sfdc on those.
| @@ -25,8 +25,13 @@ export const messages = { | |||
| graphEngine: { | |||
| noViolationsFound: "Scan was completed. No violations found.", | |||
There was a problem hiding this comment.
Is this message still used anywhere?
There was a problem hiding this comment.
Yes, this is still used when there are no violations found. I tried this with some empty methods and made sure this is thrown.
src/extension.ts
Outdated
| @@ -94,10 +111,27 @@ export async function activate(context: vscode.ExtensionContext): Promise<vscode | |||
| return Promise.resolve(context); | |||
| } | |||
|
|
|||
| export async function _stopExistingDfaRun(context: vscode.ExtensionContext, outputChannel: vscode.LogOutputChannel) { | |||
There was a problem hiding this comment.
As a matter of style, typescript functions should have return types declared, so this should be : Promise<void>
There was a problem hiding this comment.
Line 128 has void on it... which is why the compiler is happy. It took me a minute on this one as well... so just for readability/consistency we can remove the void on line 128 and add in : Promise as Josh is suggesting.
There was a problem hiding this comment.
I will add Promise. I still want to keep line 128 with void in it. Seems like that's the preference for updating caches (by not blocking the original function by using await).
src/extension.ts
Outdated
| .then(async answer => { | ||
| if (answer === messages.graphEngine.stopDfaRunConfirmationYes) { | ||
| await _stopExistingDfaRun(context, channel); | ||
| return true; |
There was a problem hiding this comment.
Why does this callback return true here? The return value isn't used anywhere as far as I can tell. Also, right now there are cases where it won't return anything, which seems like it's a problem.
There was a problem hiding this comment.
Agreed, this should be here.
src/extension.ts
Outdated
| @@ -94,10 +111,27 @@ export async function activate(context: vscode.ExtensionContext): Promise<vscode | |||
| return Promise.resolve(context); | |||
| } | |||
|
|
|||
| export async function _stopExistingDfaRun(context: vscode.ExtensionContext, outputChannel: vscode.LogOutputChannel) { | |||
There was a problem hiding this comment.
Line 128 has void on it... which is why the compiler is happy. It took me a minute on this one as well... so just for readability/consistency we can remove the void on line 128 and add in : Promise as Josh is suggesting.
src/extension.ts
Outdated
| @@ -94,10 +111,27 @@ export async function activate(context: vscode.ExtensionContext): Promise<vscode | |||
| return Promise.resolve(context); | |||
| } | |||
|
|
|||
| export async function _stopExistingDfaRun(context: vscode.ExtensionContext, outputChannel: vscode.LogOutputChannel) { | |||
| const pid = context.workspaceState.get(Constants.GLOBAL_DFA_PROCESS); | |||
There was a problem hiding this comment.
nitpick. Now that it is a workspace setting and not a global setting. Do we want the name of the constant to still be GLOBAL_DFA_PROCESS?
There was a problem hiding this comment.
Will take care of this.
No description provided.