Implement report issue command for prerelease extension #5837

dibarbet · 2023-06-20T19:17:50Z

Resolves #5747

dibarbet · 2023-06-20T19:18:42Z

src/lsptoolshost/dotnetRuntimeExtensionResolver.ts

+/**
+ * Resolves the dotnet runtime from given options and the dotnet runtime VSCode extension.
+ */
+export class DotnetRuntimeExtensionResolver implements IHostExecutableResolver {


moved the runtime resolution into the IHostExecutableResolver so we can get this information in other places (and share more code with O#).

this is all essentially just moved

dibarbet · 2023-06-20T19:19:34Z

src/lsptoolshost/roslynLanguageServer.ts

@@ -641,6 +602,39 @@ export async function activateRoslynLanguageServer(context: vscode.ExtensionCont
    _languageServer.start();
 }

+export function getServerPath(options: Options, platformInfo: PlatformInformation) {


moved to re-use

dibarbet · 2023-06-20T19:20:07Z

src/observers/OpenURLObserver.ts

-    constructor(private vscode: vscode) {
-    }
-
-    public post = (event: BaseEvent) => {


this seemed entirely unnecessary - an event + observer just for a single location to open a URL

dibarbet · 2023-06-20T19:20:26Z

src/shared/reportIssue.ts

+    ${monoVersion}</details>`;
+}
+
+function getLogInfo(useOmnisharp: boolean): string {


this is a new bit

dibarbet · 2023-06-20T19:21:10Z

src/shared/reportIssue.ts

+    }
+
+    const tableHeader = `|Extension|Author|Version|Folder Name|\n|---|---|---|---|`;
+    const table = extensions.map((e) => `|${e.packageJSON.name}|${e.packageJSON.publisher}|${e.packageJSON.version}|${basename(e.extensionPath)}|`).join("\n");


this is all the same, except I added the extension folder name. This is the only place where it will show which architecture the extension comes from. Useful because we've had issues with the marketplace installing the wrong architecture version.

Is that pulling in just the immediate parent path or the full path? That may include the username which they might not want to share.

Just the name of the immediate parent - I didn't include the entire path for that exact reason :)

dibarbet · 2023-06-20T19:22:02Z

src/shared/utils/getDotnetInfo.ts

-                };
-                return _dotnetInfo;
-            }
+        if (version !== undefined) {


dotnet info on a runtime-only dotnet.exe does not output any runtimeid

jasonmalinowski · 2023-06-21T20:39:08Z

src/shared/reportIssue.ts

+    }
+
+    const tableHeader = `|Extension|Author|Version|Folder Name|\n|---|---|---|---|`;
+    const table = extensions.map((e) => `|${e.packageJSON.name}|${e.packageJSON.publisher}|${e.packageJSON.version}|${basename(e.extensionPath)}|`).join("\n");


Is that pulling in just the immediate parent path or the full path? That may include the username which they might not want to share.

jasonmalinowski · 2023-06-21T20:43:25Z

test/unitTests/features/reportIssue.test.ts

    let options: Options;
    let issueBody: string;

    setup(() => {
        vscode = getFakeVsCode();
+        // @ts-ignore


Why the @ts-ignore here?

It needs to return a vscode.Uri, but the only way I know how to get one is to call vscode.Uri.Parse... which we're implementing here. Since the result of this call doesn't matter for the unit test anyway, I return the same input value. Since it isn't a vscode.Uri type though I need to suppress the TS warnings.

When we get integration tests I want to delete these unit tests in favor of integration tests anyway.

@dibarbet The VSCode URI package is referencable separately: https://github.com/microsoft/vscode-uri. So it's pretty easy to add a reference to that and then have a "real" parse method in play.

jasonmalinowski · 2023-06-21T21:34:33Z

src/shared/reportIssue.ts

+`;
+
+    const queryStringPrefix: string = "?";
+    const issueDefault = "Please paste the output from your clipboard";


Do I dare ask why we don't just fill in the body directly since we have a way to do that?

I think it is because of potential URI escaping issues (the body gets put into the actual URI). Might be possible still, I can investigate it later. For now I'll leave it since this is the existing behavior.

Yeah, I don't know why we can't just call an escape method here. :-/

its not as simple as doing encodeURIComponent and passing it along, I did try that, but you get this:

So will dig into this more later.

I saw in the python extension that you could open the vscode Report Issue dialog with a templated issue body. Might be something to look into. https://github.com/microsoft/vscode-python/blob/acd12995eacfd52577a5aedf494e1cd163433476/src/client/common/application/commands/reportIssueCommand.ts#L100-L109

Great suggestion. I'm using that. Fills out the template nicely, includes additional system info, shows duplicates. Works great.

src/shared/reportIssue.ts

src/lsptoolshost/dotnetRuntimeExtensionResolver.ts

dibarbet · 2023-06-21T22:58:42Z

@WardenGnaw mind taking a quick look at the debugger change?

package.json

src/coreclr-debug/util.ts

test/unitTests/features/reportIssue.test.ts

WardenGnaw

src/lsptoolshost/dotnetRuntimeExtensionResolver.ts

jaredpar · 2023-06-23T17:01:00Z

src/lsptoolshost/dotnetRuntimeExtensionResolver.ts

+
+    async getHostExecutableInfo(options: Options): Promise<HostExecutableInformation> {
+        let dotnetRuntimePath = options.commonOptions.dotnetPath;
+        const serverPath = getServerPath(options, this.platformInfo);


This seems wrong in the current form. This is a generic .NET resolver but it's calling into roslyn's specific getServerPath method. Shouldn't we make this fully generic and have the paths / file namse passed in so we can share with razor?

Yeah - when I originally wrote it, I hadn't thought about sharing it with Razor yet. I'll update this.

…olver

dibarbet · 2023-06-23T17:24:48Z

src/lsptoolshost/dotnetRuntimeExtensionResolver.ts

+        /**
+         * This is a function instead of a string because the server path can change while the extension is active (when the option changes).
+         */
+        private getServerPath: (options: Options, platform: PlatformInformation) => string) { }


Updating to eventually share this with Razor - #5855
A couple reasons for doing it this way

As mentioned in the comment it needs to be a function for when the server path changes (from the option). One IHostExecutableResolver is created during activation for roslyn, and re-used across server restarts.

It didn't seem appropriate to update getHostExecutableInfo to take in the path because not all implementations of IHostExecutableResolver require a server path (for example used in O# with dotnet and mono as well). Its more of a property of this specific resolver.

jasonmalinowski · 2023-06-23T21:25:33Z

src/shared/reportIssue.ts

+    await vscode.commands.executeCommand("workbench.action.openIssueReporter", {
+        extensionId: CSharpExtensionId,
+        issueBody: body
+    });


Implement report issue command for prerelease extension

40c292a

dibarbet requested a review from genlu June 20, 2023 19:17

dibarbet requested review from a team as code owners June 20, 2023 19:17

dibarbet commented Jun 20, 2023

View reviewed changes

Merge remote-tracking branch 'upstream/main' into report_issue

2d8dadc

jasonmalinowski approved these changes Jun 21, 2023

View reviewed changes

dibarbet added 2 commits June 21, 2023 15:05

review feedback

06460ea

use vscode-uri

d5362bf

WardenGnaw reviewed Jun 22, 2023

View reviewed changes

package.json Outdated Show resolved Hide resolved

src/coreclr-debug/util.ts Outdated Show resolved Hide resolved

test/unitTests/features/reportIssue.test.ts Outdated Show resolved Hide resolved

dibarbet added 4 commits June 22, 2023 11:03

Fix options schema

b8d485e

Explicitly throw if runtimeId is missing

9a2ff73

Use real looking runtimeId in fake

153f19c

Use issue reporter command

0bc35c0

dibarbet requested review from WardenGnaw and jasonmalinowski June 22, 2023 19:25

WardenGnaw approved these changes Jun 22, 2023

View reviewed changes

dibarbet mentioned this pull request Jun 23, 2023

Allow rzls to launch using standard dotnet #5855

Open

jaredpar reviewed Jun 23, 2023

View reviewed changes

dibarbet added 2 commits June 23, 2023 10:21

Add server path as an option to allow Razor to re-use the runtime res…

5050d46

…olver

Fix brace

949bbc9

dibarbet commented Jun 23, 2023

View reviewed changes

jasonmalinowski approved these changes Jun 23, 2023

View reviewed changes

dibarbet merged commit 32917c2 into dotnet:main Jun 27, 2023

dibarbet deleted the report_issue branch June 27, 2023 23:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement report issue command for prerelease extension #5837

Implement report issue command for prerelease extension #5837

dibarbet commented Jun 20, 2023 •

edited

Loading

dibarbet Jun 20, 2023

dibarbet Jun 20, 2023

dibarbet Jun 20, 2023

dibarbet Jun 20, 2023

dibarbet Jun 20, 2023

dibarbet Jun 20, 2023

jasonmalinowski Jun 21, 2023

dibarbet Jun 21, 2023

dibarbet Jun 20, 2023

jasonmalinowski Jun 21, 2023

jasonmalinowski Jun 21, 2023

dibarbet Jun 21, 2023

jasonmalinowski Jun 21, 2023

jasonmalinowski Jun 21, 2023

dibarbet Jun 21, 2023

jasonmalinowski Jun 21, 2023

dibarbet Jun 21, 2023

JoeRobich Jun 22, 2023

dibarbet Jun 22, 2023 •

edited

Loading

dibarbet commented Jun 21, 2023

WardenGnaw left a comment

jaredpar Jun 23, 2023

dibarbet Jun 23, 2023

dibarbet Jun 23, 2023 •

edited

Loading

jasonmalinowski Jun 23, 2023

Implement report issue command for prerelease extension #5837

Implement report issue command for prerelease extension #5837

Conversation

dibarbet commented Jun 20, 2023 • 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

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

dibarbet Jun 22, 2023 • edited Loading

Choose a reason for hiding this comment

dibarbet commented Jun 21, 2023

WardenGnaw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dibarbet Jun 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dibarbet commented Jun 20, 2023 •

edited

Loading

dibarbet Jun 22, 2023 •

edited

Loading

dibarbet Jun 23, 2023 •

edited

Loading