Enable multiple download of omnisharp #2065

akshita31 · 2018-02-23T23:02:24Z

Fixes: #1909
Final PR including all the changes.

…ns of omnisharp (#2028) * Enable usage of multiple versions * Either load the server from a path or download the version packages * Tests for the package creator * Added null check and removed semver check in package creator * Test for the experiment omnisharp downloader * Added test for package manager * Code clean up * Added null or empty check for version * Changes * Modified the description Put the clean up logic in the teardown function * Remove comment * Remove unnecessary usage * CR comments * Removed experimental * Modified launcher * Removed experimental * Modified tests * Modified package description to include version information * Renamed launch path * Add more tests * Changed the description in package.json

* Enable usage of multiple versions * Either load the server from a path or download the version packages * Tests for the package creator * Added null check and removed semver check in package creator * Test for the experiment omnisharp downloader * Added test for package manager * Code clean up * Added null or empty check for version * Changes * Modified the description * Put the clean up logic in the teardown function * Remove comment * Remove unnecessary usage * CR comments * Removed experimental * Modified launcher * Removed experimental * Modified tests * Modified package description to include version information * Renamed launch path * Add more tests * Changed the description in package.json * Getting latest version info * Refactored code and added tests * Remove unnecessary using * CR comments * Use common function for latest download * Add new line * Resolve binaries on linux

colombod · 2018-02-23T23:40:29Z

package.json

@@ -39,6 +39,7 @@
    "postinstall": "node ./node_modules/vscode/bin/install"
  },
  "dependencies": {
+    "chai-as-promised": "^7.1.1",


should this be a dev dependency only?

This is automatically added when I npm install the package. What do you think should be changed here?

I would just move this dependency in the devdependecies list and fix it to 7.1.1 instead of compatible versions

TheRealPiotrP · 2018-02-23T23:42:58Z

package.json

@@ -93,7 +94,8 @@
      "architectures": [
        "x86"
      ],
-      "installTestPath": "./.omnisharp/OmniSharp.exe"
+      "installTestPath": "./.omnisharp/OmniSharp.exe",
+      "experimentalPackageId": "win-x86"


let's rename experimentalPackageId to platformId

rchande

Overall looks pretty good. There's probably some stuff we can clean up later. I made a couple of comments--fix those up and I think we're good to go.

rchande · 2018-02-23T23:14:56Z

src/OmnisharpDownload.Helper.ts

+    installationStage = '';
+    logger.appendLine('Finished');
+
+    statusItem.dispose();


Seems weird that this method takes statusItem just to call dispose on it.

rchande · 2018-02-23T23:15:29Z

src/OmnisharpDownload.Helper.ts

+    if (platformInfo.distribution) {
+        telemetryProps['platform.distribution'] = platformInfo.distribution.toTelemetryString();
+    }
+    if (reporter) {


Is reporter ever null?

I dont think so. But I just followed by the pattern followed by CSharpExtDownloader.

Currently this pattern helps because in testing I am conveniently passing this field as null, but in future we might want to test the Telemetry as well.

For the current PR, I think we can retain this pattern. What do you think ?

Sure, we can refactor later.

rchande · 2018-02-23T23:21:43Z

src/omnisharp/OmnisharpDownloader.ts

+
+    public async DownloadAndInstallOmnisharp(version: string, serverUrl: string, installPath: string) {
+        if (!version) {
+            throw new Error('Invalid version');


If this happens, this is basically a bug in our code, right? Since this method is not expecting a null version. "invalid version" makes it sound like a user mistake, which it isn't.

rchande · 2018-02-23T23:24:13Z

src/omnisharp/OmnisharpManager.ts

+            }
+        }
+        else if (omnisharpPath == "latest") {
+            return await this.LatestInstallAndReturnLaunchPath(downloader, useMono, serverUrl, versionFilePathInServer, installPath, extensionPath, platformInfo);


call this method InstallLatestAndReturnLaunchPath

rchande · 2018-02-23T23:25:11Z

src/omnisharp/OmnisharpManager.ts

+            return GetLaunchPathForVersion(platformInfo, version, installPath, extensionPath, useMono);
+        }
+        else {
+            throw new Error('Invalid omnisharp version specified');


Can we include the specified version in this message?

rchande · 2018-02-23T23:25:56Z

src/omnisharp/OmnisharpManager.ts

+
+    let basePath = path.resolve(extensionPath, installPath, version);
+
+    if (platformInfo.isWindows()) {


platformInfo.isWindows() || useMono ?

No, in case of useMono we append an extra "omnisharp" to the path

rchande · 2018-02-23T23:28:22Z

src/omnisharp/OmnisharpPackageCreator.ts

+        throw new Error('Invalid version');
+    }
+
+    let versionPackage = <Package>{


Can you use the ... operator and just change the fields you care about?
{...package, url = zzz, experimentalPackageId = xxx
Means the same thing, but might be easier to tell what fields you're changing.

rchande · 2018-02-23T23:43:01Z

src/omnisharp/server.ts

+        let launchPath: string;
+        if (this._options.path) {
+            try {
+                let serverUrl = "https://roslynomnisharp.blob.core.windows.net";


Maybe move these into some obvious constant definitions somewhere?

Where do you suggest can these be moved? Should we define some object that contains these fields in the package.json and just read from it here ?

You could just move them up to be const fields in server.ts

rchande · 2018-02-23T23:43:36Z

src/packages.ts

@@ -24,6 +24,7 @@ export interface Package {
    architectures: string[];
    binaries: string[];
    tmpFile: tmp.SynchrounousResult;
+    experimentalPackageId?: string;


Change to whatever name Peter came up with :)

TheRealPiotrP

let's rename the experimentalPackageId. Otherwise lgtm.

…nisharp-vscode into experiment_download

akshita31 added 3 commits February 14, 2018 15:40

Merge remote-tracking branch 'upstream/master' into experiment_download

7aa752d

akshita31 requested review from DustinCampbell, TheRealPiotrP, rchande and gregg-miskelly February 23, 2018 23:02

colombod reviewed Feb 23, 2018

View reviewed changes

TheRealPiotrP reviewed Feb 23, 2018

View reviewed changes

rchande suggested changes Feb 24, 2018

View reviewed changes

TheRealPiotrP suggested changes Feb 24, 2018

View reviewed changes

akshita31 and others added 6 commits February 23, 2018 16:40

Copy pacakges by value

7e11418

Renamed experimentalId and some methods

180f438

Moved status item out of method and added version to message

88221c3

Fix typo (#2066)

9d6b82c

Formatting for better display structure

a917cd6

Merge branch 'master' into experiment_download

7cbccd8

rchande approved these changes Feb 26, 2018

View reviewed changes

akshita31 added 6 commits February 26, 2018 10:36

Move to const fields in server

29deec5

Make methods private

ea4c351

Merge branch 'experiment_download' of https://github.com/OmniSharp/om…

05a528c

…nisharp-vscode into experiment_download

Move chai-as-promised to devDependencies

4dde51c

Removed unnecessary function

1ce54c1

Modified test description

7cf9886

akshita31 mentioned this pull request Feb 26, 2018

Refactor CSharpExtDownloader to use the extracted helper functions #2072

Merged

Merge branch 'master' into experiment_download

5c78308

TheRealPiotrP approved these changes Feb 27, 2018

View reviewed changes

akshita31 merged commit 0342385 into master Feb 27, 2018

akshita31 deleted the experiment_download branch February 28, 2018 16:19

akshita31 mentioned this pull request May 9, 2018

Enable users to download alternative copies of omnisharp #1909

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable multiple download of omnisharp #2065

Enable multiple download of omnisharp #2065

akshita31 commented Feb 23, 2018 •

edited

Loading

colombod Feb 23, 2018

akshita31 Feb 24, 2018

colombod Feb 26, 2018

TheRealPiotrP Feb 23, 2018

rchande left a comment

rchande Feb 23, 2018

rchande Feb 23, 2018

akshita31 Feb 24, 2018

rchande Feb 26, 2018

rchande Feb 23, 2018

rchande Feb 23, 2018

rchande Feb 23, 2018

rchande Feb 23, 2018

akshita31 Feb 24, 2018

rchande Feb 26, 2018

rchande Feb 23, 2018

rchande Feb 23, 2018

akshita31 Feb 24, 2018

rchande Feb 26, 2018

rchande Feb 23, 2018

TheRealPiotrP left a comment


		let basePath = path.resolve(extensionPath, installPath, version);

		if (platformInfo.isWindows()) {

Enable multiple download of omnisharp #2065

Enable multiple download of omnisharp #2065

Conversation

akshita31 commented Feb 23, 2018 • 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

rchande 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

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

TheRealPiotrP left a comment

Choose a reason for hiding this comment

akshita31 commented Feb 23, 2018 •

edited

Loading