Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor CSharpExtDownloader to use the extracted helper functions #2072

Merged
merged 7 commits into from
Feb 27, 2018
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
136 changes: 43 additions & 93 deletions src/CSharpExtDownloader.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,34 +9,27 @@ import * as util from './common';
import { Logger } from './logger';
import { PackageManager, Status, PackageError } from './packages';
import { PlatformInformation } from './platform';
import { SetStatus, GetNetworkDependencies, ReportInstallationError, SendInstallationTelemetry } from './downloader.helper';

/*
* Class used to download the runtime dependencies of the C# Extension
*/
export class CSharpExtDownloader
{
export class CSharpExtDownloader {
public constructor(
private channel: vscode.OutputChannel,
private logger: Logger,
private reporter: TelemetryReporter /* optional */,
private packageJSON: any) {
}

public installRuntimeDependencies(): Promise<boolean> {
public async installRuntimeDependencies(): Promise<boolean> {
this.logger.append('Installing C# dependencies...');
this.logger.appendLine();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why?

this.channel.show();

let statusItem = vscode.window.createStatusBarItem(vscode.StatusBarAlignment.Right);
let status: Status = {
setMessage: text => {
statusItem.text = text;
statusItem.show();
},
setDetail: text => {
statusItem.tooltip = text;
statusItem.show();
}
};
let statusObject = SetStatus();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, the naming of this object is really tough with the current factoring!

How about we rename SetStatus to GetRightAlignedStatusBarItem? It's still not perfect, but we'll need to do deeper refactoring around this object to make it better, and that will be better done in a dedicated PR.

let status = statusObject.Status;
let statusItem = statusObject.StatusItem;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Instead of having SetStatus return the StatusItem, I think the code would be a lot more understandable if if you just returned Status, and Status had a Dispose method to dispose the StatusItem.


// Sends "AcquisitionStart" telemetry to indicate an acquisition started.
if (this.reporter) {
Expand All @@ -46,95 +39,52 @@ export class CSharpExtDownloader
let platformInfo: PlatformInformation;
let packageManager: PackageManager;
let installationStage = 'touchBeginFile';
let errorMessage = '';
let success = false;

let telemetryProps: any = {};

return util.touchInstallFile(util.InstallFileType.Begin)
.then(() => {
installationStage = 'getPlatformInfo';
return PlatformInformation.GetCurrent();
})
.then(info => {
platformInfo = info;
packageManager = new PackageManager(info, this.packageJSON);
this.logger.appendLine();

// Display platform information and RID followed by a blank line
this.logger.appendLine(`Platform: ${info.toString()}`);
this.logger.appendLine();

installationStage = 'downloadPackages';

const config = vscode.workspace.getConfiguration();
const proxy = config.get<string>('http.proxy');
const strictSSL = config.get('http.proxyStrictSSL', true);

return packageManager.DownloadPackages(this.logger, status, proxy, strictSSL);
})
.then(() => {
this.logger.appendLine();
try {
await util.touchInstallFile(util.InstallFileType.Begin);
installationStage = 'getPlatformInfo';
platformInfo = await PlatformInformation.GetCurrent();

installationStage = 'installPackages';
return packageManager.InstallPackages(this.logger, status);
})
.then(() => {
installationStage = 'touchLockFile';
return util.touchInstallFile(util.InstallFileType.Lock);
})
.then(() => {
installationStage = 'completeSuccess';
success = true;
})
.catch(error => {
if (error instanceof PackageError) {
// we can log the message in a PackageError to telemetry as we do not put PII in PackageError messages
telemetryProps['error.message'] = error.message;
packageManager = new PackageManager(platformInfo, this.packageJSON);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The original code had a this.logger.appendLine(); after this line.


if (error.innerError) {
errorMessage = error.innerError.toString();
} else {
errorMessage = error.message;
}
// Display platform information and RID followed by a blank line
this.logger.appendLine(`Platform: ${platformInfo.toString()}`);
this.logger.appendLine();

if (error.pkg) {
telemetryProps['error.packageUrl'] = error.pkg.url;
}
installationStage = 'downloadPackages';

} else {
// do not log raw errorMessage in telemetry as it is likely to contain PII.
errorMessage = error.toString();
}
let networkObject = GetNetworkDependencies();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how about:
let networkConfiguration = GetNetworkConfiguration();? Rename the variable and the method? this seems clearer.

const proxy = networkObject.Proxy;
const strictSSL = networkObject.StrictSSL;

this.logger.appendLine(`Failed at stage: ${installationStage}`);
this.logger.appendLine(errorMessage);
})
.then(() => {
telemetryProps['installStage'] = installationStage;
telemetryProps['platform.architecture'] = platformInfo.architecture;
telemetryProps['platform.platform'] = platformInfo.platform;
if (platformInfo.distribution) {
telemetryProps['platform.distribution'] = platformInfo.distribution.toTelemetryString();
}
await packageManager.DownloadPackages(this.logger, status, proxy, strictSSL);
this.logger.appendLine();

if (this.reporter) {
this.reporter.sendTelemetryEvent('Acquisition', telemetryProps);
}
installationStage = 'installPackages';
await packageManager.InstallPackages(this.logger, status);

this.logger.appendLine();
installationStage = '';
this.logger.appendLine('Finished');

statusItem.dispose();
})
.then(() => {
// We do this step at the end so that we clean up the begin file in the case that we hit above catch block
// Attach a an empty catch to this so that errors here do not propogate
return util.deleteInstallFile(util.InstallFileType.Begin).catch((error) => { });
}).then(() => {
return success;
});

installationStage = 'touchLockFile';
await util.touchInstallFile(util.InstallFileType.Lock);

installationStage = 'completeSuccess';
success = true;
}
catch (error) {
ReportInstallationError(this.logger, error, telemetryProps, installationStage);
}
finally {
SendInstallationTelemetry(this.logger, this.reporter, telemetryProps, installationStage, platformInfo);
statusItem.dispose();
// We do this step at the end so that we clean up the begin file in the case that we hit above catch block
// Attach a an empty catch to this so that errors here do not propogate
try {
util.deleteInstallFile(util.InstallFileType.Begin);
}
catch (error) { }
return success;
}
}
}
File renamed without changes.
2 changes: 1 addition & 1 deletion src/omnisharp/OmnisharpDownloader.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import { PlatformInformation } from '../platform';
import { Logger } from '../logger';
import TelemetryReporter from 'vscode-extension-telemetry';
import { GetPackagesFromVersion, GetVersionFilePackage } from './OmnisharpPackageCreator';
import { SetStatus, ReportInstallationError, SendInstallationTelemetry, GetNetworkDependencies } from '../OmnisharpDownload.Helper';
import { SetStatus, ReportInstallationError, SendInstallationTelemetry, GetNetworkDependencies } from '../downloader.helper';

export class OmnisharpDownloader {
private status: Status;
Expand Down