Validate mainClass and projectName before launch by testforstephen · Pull Request #368 · microsoft/vscode-java-debug

Jinbo Wang (testforstephen) · 2018-08-15T01:44:11Z

Signed-off-by: Jinbo Wang jinbwan@microsoft.com

See issue #355 for the requirement.

The validation work is done on java code side, see the peer PR: microsoft/java-debug#205

Signed-off-by: Jinbo Wang <jinbwan@microsoft.com>

Yaohai Zheng (yaohaizh) · 2018-08-15T01:55:20Z


-    private async chooseMainClass(folder: vscode.WorkspaceFolder | undefined): Promise<IMainClassOption | undefined> {
-        const res = await resolveMainClass(folder ? folder.uri : undefined);
+    private async quickFixLaunchConfig(folder: vscode.Uri | undefined, config: vscode.DebugConfiguration):


I don't think this function is to fix the launch config; fix is only part of the functionality after failed to choose mainclass. More like populate the launchConfig.

IMO, when the mainClass and projectName is empty, and we prompt users to choose one, this is also one kind of fix. And the difference is we don't persist them in launch.json.

Fix means something is wrong and you want to correct it. For user configuration, the mainClass and projectName is correct and doesn't need fix them after validation.

Yaohai Zheng (yaohaizh) · 2018-08-15T02:13:11Z

+
+            if (validationResponse.proposals && validationResponse.proposals.length) {
+                const answer = await utility.showErrorMessageWithTroubleshooting({
+                    message: errors.join("\n"),


Use platform dependent separator

Yaohai Zheng (yaohaizh) · 2018-08-15T02:19:26Z

+        }
+    }
+
+    private findDebugConfig(rawConfigs: vscode.DebugConfiguration[], config: vscode.DebugConfiguration): number {


Since we are Java debugger, use the type to filter out unnecessary DebugConfig.

Use lodash utility to search the debug config instead.

Yaohai Zheng (yaohaizh) · 2018-08-15T02:26:28Z

+        return -1;
+    }
+
+    private compareDebugConfig(config1: object, config2: object): boolean {


The convention of semantic for compare result is -1, 0, 1 but not boolean value. Please keep this convention as possible.

Use equals*** for boolean result.

this method is discarded.

Yaohai Zheng (yaohaizh) · 2018-08-15T02:30:57Z

+
+interface IValidationResult {
+    readonly passed: boolean;
+    readonly message?: string;


message ==> errorMessage?

after discussion, we think the neutral message name is more appropriate, because message could be for both valid and invalid case.

Yaohai Zheng (yaohaizh)

Chose wrong result...

Rome Li (akaroml) · 2018-08-15T02:51:07Z


-    private async chooseMainClass(folder: vscode.WorkspaceFolder | undefined): Promise<IMainClassOption | undefined> {
-        const res = await resolveMainClass(folder ? folder.uri : undefined);
+    private async quickFixLaunchConfig(folder: vscode.Uri | undefined, config: vscode.DebugConfiguration):


I would suggest 'resolveLaunchConfig' because a fix is not happening in every case.

Rome Li (akaroml) · 2018-08-15T03:31:05Z

-        const selection = pickItems.length > 1 ?
-            await vscode.window.showQuickPick(pickItems, { placeHolder: "Select main class<project name>" })
+
+        const selection = alwaysShowQuickPickBox || pickItems.length > 1 ?


Does alwaysShowQuickPickBox mean we need to keep the box visible when focus moves?
If that's the case then the QuickPickOptions.ignoreFocusOut needs to be set to true to achieve that.
I'm confused by the variable name and its usage here.

The flag is not about focus thing. Just to indicate whether to show the QuickPick box when only one available pick item.

In previous logic, when there is only one mainClass available, then pick it silently. But in the Fix prompt, even only one available mainClass, i want to show the QuickPick box to keep user consent about the ongoing fix replacement.

Rome Li (akaroml) · 2018-08-15T03:32:16Z

 }
+
+interface IValidationResult {
+    readonly passed: boolean;


passed -> isValid

Rome Li (akaroml) · 2018-08-15T03:35:27Z

+        return true;
+    }
+
+    private async chooseMainClass(folder: vscode.Uri | undefined): Promise<IMainClassOption | undefined> {


chooseMainClass -> promptMainClass

Rome Li (akaroml) · 2018-08-15T04:16:29Z

+                const answer = await utility.showErrorMessageWithTroubleshooting({
+                    message: errors.join("\n"),
+                    type: Type.USAGEERROR,
+                }, "Quick Fix");


'Quick Fix' -> 'Fix'

Rome Li (akaroml) · 2018-08-15T04:17:41Z

+                    type: Type.USAGEERROR,
+                }, "Quick Fix");
+                if (answer === "Quick Fix") {
+                    const quickFix: IMainClassOption = await this.showMainClassQuickPick(validationResponse.proposals,


What if user cancels and returns nothing?

If the user chooses to cancel the fix QuickPick box, then the debugger exits.

Rome Li (akaroml) · 2018-08-15T04:21:28Z

+        }
+    }
+
+    private persistLaunchConfig(folder: vscode.Uri | undefined, config: vscode.DebugConfiguration, change: IMainClassOption): void {


The function looks generic from its name and signature. But the data type of chagne is restricted to IMainClassOption which makes it only accept main class changes.
To workaround this, the signature should look like (folder, oldConfig, newConfig) to me, newConfig is with change applied. oldConfig is for us to look up the actual section in the file.

Rome Li (akaroml) · 2018-08-15T04:25:20Z

+    }
+
+    private findDebugConfig(rawConfigs: vscode.DebugConfiguration[], config: vscode.DebugConfiguration): number {
+        const candidates = [];


lodash.isEqual does deep comparison.
_.findIndex(rawConfigs, _.isEqual) can get you the index.

1. Better naming about validate and fix mainClass and ProjectName. 2. Use lodash library to find the debug configuration index from the workspaceConfiguration list.

Rome Li (akaroml) · 2018-08-17T07:03:36Z

-    readonly passed: boolean;
-    readonly message?: string;
+    readonly isValid: boolean;
+    readonly errorMessage?: string;


The message field can be neutral because there could message for both valid and invalid case.

Agree. Have reverted the field back to message.

Rome Li (akaroml) · 2018-08-17T07:04:39Z

+import * as _ from "lodash";
+import * as os from "os";
 import * as vscode from "vscode";
+


Nice to have extra line to separate 3rd party dependencies and local dependencies.

…scheme

Andy Xu(devdiv) (andxu) · 2018-08-23T06:59:33Z

+        }
+
+        if (validationResponse.proposals && validationResponse.proposals.length) {
+            const answer = await utility.showErrorMessageWithTroubleshooting({


dup with line 257, you can keep the promise of utility.showErrorMessageWithTroubleshooting and await it if validationResponse.proposals have values.

There is a tiny difference between if/else conditions here. For the case of having proposals, the UI shows a Fix button, and the debug session will wait the user to click it. For the case of no having proposals, the UI just shows a Learn More button, no need to wait the user's click behavior, and the debug session will terminate directly.

Rome Li (akaroml) · 2018-08-27T05:05:43Z

-                    config.projectName = userSelection.projectName;
+
+                const mainClassOption = await this.resolveLaunchConfig(folder ? folder.uri : undefined, config);
+                if (!mainClassOption || !mainClassOption.mainClass) {


Use try/catch block to handle the rejected/exception case.

Rome Li (akaroml) · 2018-08-27T05:06:08Z


-    private async chooseMainClass(folder: vscode.WorkspaceFolder | undefined): Promise<IMainClassOption | undefined> {
-        const res = await resolveMainClass(folder ? folder.uri : undefined);
+    private isEmptyArray(configItems: any): boolean {


Use _.isEmpty() instead.

Rome Li (akaroml) · 2018-08-27T05:24:00Z

+        return !Array.isArray(configItems) || !configItems.length;
+    }
+
+    private async resolveLaunchConfig(folder: vscode.Uri | undefined, config: vscode.DebugConfiguration): Promise<IMainClassOption> {


If the first parameter is optional, then we could write it as folder?: vscode.Uri. The question mark after the param name will assign undefined value when the param is not omitted. "default to undefined" is not necessariy by the nature of ts/js.

I suggest that you place config param as the first param because it's required. And folder to the second because it's optional.

Actually the pattern is inherited from VSCode API. See the original VSCode API defintion public resolveDebugConfiguration(folder: vscode.WorkspaceFolder | undefined, config: vscode.DebugConfiguration, token?: vscode.CancellationToken):

It indicates the folder parameter is not optional. Just represent it could be undefined value when no workspace is opened.

Rome Li (akaroml) · 2018-08-27T05:27:17Z

+    }
+
+    private async fixMainClass(folder: vscode.Uri | undefined, config: vscode.DebugConfiguration, validationResponse: ILaunchValidationResponse):
+        Promise<IMainClassOption | undefined> {


A promise should be resolved with real results instead of undefined. In the case that we are not able to resolve some real results, it looks like an exception case.

Here is a case for undefined. when user cancels the fix dropdown, should return undefined instead of an exception. because it's user consent behavior.

Rome Li (akaroml) · 2018-08-27T05:29:21Z

+    private async showMainClassQuickPick(pickItems: IMainClassQuickPickItem[], quickPickHintMessage: string, autoPick: boolean = true):
+        Promise<IMainClassOption | undefined> {
+        if (pickItems.length === 0) {
+            return;


return nothing means resolving with undefined. This does not seem right. I would recommend through an exception because an empty candidate list is an error case.

will remove the empty check logic, because the check has been done in the caller side. No need to duplicate it.

Rome Li (akaroml) · 2018-08-27T05:30:48Z

        }
+
+        const selected = (pickItems.length === 1 && autoPick) ?
+            pickItems[0] : await vscode.window.showQuickPick(pickItems, { placeHolder: quickPickHintMessage });


What if user cancels by pressing Esc?

selected = undefined if user cancels by ESC.

Rome Li (akaroml) · 2018-08-27T05:31:55Z

+        };
+    }
+
+    private async fixMainClass(folder: vscode.Uri | undefined, config: vscode.DebugConfiguration, validationResponse: ILaunchValidationResponse):


Same comment regarding optional params.

Rome Li (akaroml) · 2018-08-27T05:32:23Z

+        return;
+    }
+
+    private async persistMainClassOption(folder: vscode.Uri | undefined, oldConfig: vscode.DebugConfiguration, change: IMainClassOption):


Same comment regarding optional params and sequence.

Rome Li (akaroml) · 2018-08-27T05:32:29Z

+        return this.persistLaunchConfig(folder, oldConfig, newConfig);
+    }
+
+    private async persistLaunchConfig(folder: vscode.Uri | undefined, oldConfig: vscode.DebugConfiguration, newConfig: vscode.DebugConfiguration):


Same comment regarding optional params and sequence.

Rome Li (akaroml) · 2018-08-27T05:32:43Z

+        }
+    }
+
+    private async promptMainClass(folder: vscode.Uri | undefined): Promise<IMainClassOption | undefined> {


Same comment regarding optional params.

Validate mainClass and projectName before launch

043210f

Signed-off-by: Jinbo Wang <jinbwan@microsoft.com>

Jinbo Wang (testforstephen) requested review from Yan Zhang (Eskibear), Rome Li (akaroml), Andy Xu(devdiv) (andxu) and Yaohai Zheng (yaohaizh) August 15, 2018 01:44

Yaohai Zheng (yaohaizh) reviewed Aug 15, 2018

View reviewed changes

Yaohai Zheng (yaohaizh) approved these changes Aug 15, 2018

View reviewed changes

Yaohai Zheng (yaohaizh) suggested changes Aug 15, 2018

View reviewed changes

Rome Li (akaroml) requested changes Aug 15, 2018

View reviewed changes

Fix review comments

2023847

1. Better naming about validate and fix mainClass and ProjectName. 2. Use lodash library to find the debug configuration index from the workspaceConfiguration list.

Rome Li (akaroml) reviewed Aug 17, 2018

View reviewed changes

Jinbo Wang (testforstephen) added 5 commits August 17, 2018 15:26

Revert 'errorMessage' to the neutral 'message' for IValidationResult …

4f96333

…scheme

Pass containsExternalClasspaths flag to the mainClass validator

1470746

Resolve merge conflicts

17cfa32

Correct the learn more link for mainClass configError

c5d7ade

Fix learn more link for mainClass configError

50c3fa7

Andy Xu(devdiv) (andxu) reviewed Aug 23, 2018

View reviewed changes

Jinbo Wang (testforstephen) added 3 commits August 27, 2018 10:55

Resolve merge conflicts

4dc1854

Remove extra empty line

0653261

Resolve merge conflicts

88c0552

Rome Li (akaroml) requested changes Aug 27, 2018

View reviewed changes

Jinbo Wang (testforstephen) added 2 commits August 27, 2018 13:52

Use lodash _.isEmpty to check empty array

a506dcb

Use try/catch to handle the rejected/exceptional case

a0cfffd

Rome Li (akaroml) approved these changes Aug 27, 2018

View reviewed changes

Yaohai Zheng (yaohaizh) approved these changes Aug 28, 2018

View reviewed changes

Jinbo Wang (testforstephen) merged commit 8ac5ca5 into master Aug 28, 2018

Jinbo Wang (testforstephen) deleted the jinbo_validate branch August 28, 2018 03:16

Jinbo Wang (testforstephen) mentioned this pull request Aug 28, 2018

Add validation for configuration items of launch.json #355

Closed

Conversation

Jinbo Wang (testforstephen) commented Aug 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Yaohai Zheng (yaohaizh) Aug 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Yaohai Zheng (yaohaizh) left a comment

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!

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!

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!

Jinbo Wang (testforstephen) Aug 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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

Jinbo Wang (testforstephen) commented Aug 15, 2018 •

edited

Loading

Yaohai Zheng (yaohaizh) Aug 15, 2018 •

edited

Loading

Jinbo Wang (testforstephen) Aug 27, 2018 •

edited

Loading