Respond to pr feedback from Sarah to improve code quality

This commit is contained in:
Noah Gilson 2023-10-03 13:58:23 -07:00
Родитель ab7c2c8360
Коммит 594659acd1
10 изменённых файлов: 115 добавлений и 108 удалений

Просмотреть файл

@ -26,7 +26,9 @@ export function activate(context: vscode.ExtensionContext) {
"extensionDependencies": [
"ms-dotnettools.vscode-dotnet-runtime",
"ms-dotnettools.vscode-dotnet-sdk"
]
]
This would enable the sample to require the vscode-dotnet-runtime extension
*/
const requestingExtensionId = 'ms-dotnettools.sample-extension';

Просмотреть файл

@ -102,7 +102,7 @@ export function activate(context: vscode.ExtensionContext, extensionContext?: IE
}
const resolvedTimeoutSeconds = timeoutValue === undefined ? defaultTimeoutValue : timeoutValue;
const proxyUrl = extensionConfiguration.get<string>(configKeys.proxyUrl);
const proxyLink = extensionConfiguration.get<string>(configKeys.proxyUrl);
const acquisitionWorker = new DotnetCoreAcquisitionWorker({
storagePath: context.globalStoragePath,
@ -112,10 +112,10 @@ export function activate(context: vscode.ExtensionContext, extensionContext?: IE
installationValidator: new InstallationValidator(eventStream),
timeoutValue: resolvedTimeoutSeconds,
installDirectoryProvider: new RuntimeInstallationDirectoryProvider(context.globalStoragePath),
proxyUrl: proxyUrl
proxyUrl: proxyLink
});
const existingPathResolver = new ExistingPathResolver();
const versionResolver = new VersionResolver(context.globalState, eventStream, resolvedTimeoutSeconds, proxyUrl);
const versionResolver = new VersionResolver(context.globalState, eventStream, resolvedTimeoutSeconds, proxyLink);
const dotnetAcquireRegistration = vscode.commands.registerCommand(`${commandPrefix}.${commandKeys.acquire}`, async (commandContext: IDotnetAcquireContext) => {
let fullyResolvedVersion = '';

Просмотреть файл

@ -26,10 +26,11 @@ import { IInstallScriptAcquisitionWorker } from './IInstallScriptAcquisitionWork
import { InstallScriptAcquisitionWorker } from './InstallScriptAcquisitionWorker';
import { TelemetryUtilities } from '../EventStream/TelemetryUtilities';
import { DotnetCoreAcquisitionWorker } from './DotnetCoreAcquisitionWorker';
import { FileUtilities } from '../Utils/FileUtilities';
export class AcquisitionInvoker extends IAcquisitionInvoker {
protected readonly scriptWorker: IInstallScriptAcquisitionWorker;
protected fileUtilities : FileUtilities;
private noPowershellError = `powershell.exe is not discoverable on your system. Is PowerShell added to your PATH and correctly installed? Please visit: https://learn.microsoft.com/powershell/scripting/install/installing-powershell-on-windows.
You will need to restart VS Code after these changes. If PowerShell is still not discoverable, try setting a custom existingDotnetPath following our instructions here: https://github.com/dotnet/vscode-dotnet-runtime/blob/main/Documentation/troubleshooting-runtime.md.`
@ -37,6 +38,7 @@ You will need to restart VS Code after these changes. If PowerShell is still not
super(eventStream);
this.scriptWorker = new InstallScriptAcquisitionWorker(extensionState, eventStream, timeoutTime);
this.fileUtilities = new FileUtilities();
}
public async installDotnet(installContext: IDotnetInstallationContext): Promise<void> {
@ -93,7 +95,7 @@ You will need to restart VS Code after these changes. If PowerShell is still not
}
private async getInstallCommand(version: string, dotnetInstallDir: string, installRuntime: boolean, architecture: string): Promise<string> {
const arch = this.nodeArchToDotnetArch(architecture);
const arch = this.fileUtilities.nodeArchToDotnetArch(architecture, this.eventStream);
let args = [
'-InstallDir', this.escapeFilePath(dotnetInstallDir),
'-Version', version,
@ -111,48 +113,6 @@ You will need to restart VS Code after these changes. If PowerShell is still not
return `${ this.escapeFilePath(scriptPath) } ${ args.join(' ') }`;
}
/**
*
* @param nodeArchitecture the architecture in node style string of what to install
* @returns the architecture in the style that .net / the .net install scripts expect
*
* Node - amd64 is documented as an option for install scripts but its no longer used.
* s390x is also no longer used.
* ppc64le is supported but this version of node has no distinction of the endianness of the process.
* It has no mapping to mips or other node architectures.
*
* @remarks Falls back to string 'auto' if a mapping does not exist which is not a valid architecture.
*/
private nodeArchToDotnetArch(nodeArchitecture : string)
{
switch(nodeArchitecture)
{
case 'x64': {
return nodeArchitecture;
}
case 'ia32': {
return 'x86';
}
case 'x86': {
// In case the function is called twice
return 'x86';
}
case 'arm': {
return nodeArchitecture;
}
case 'arm64': {
return nodeArchitecture;
}
case 's390x': {
return 's390x';
}
default: {
this.eventStream.post(new DotnetCommandFallbackArchitectureEvent(`The architecture ${os.arch()} of the platform is unexpected, falling back to auto-arch.`));
return 'auto';
}
}
}
private escapeFilePath(path: string): string {
if (os.platform() === 'win32') {
// Need to escape apostrophes with two apostrophes

Просмотреть файл

@ -130,7 +130,7 @@ export class DotnetCoreAcquisitionWorker implements IDotnetCoreAcquisitionWorker
*
* @param version the version to get of the runtime or sdk.
* @param installRuntime true for runtime acquisition, false for SDK.
* @param global false for local install, true for global SDK installs.
* @param globalInstallerResolver Create this and add it to install globally.
* @returns the dotnet acquisition result.
*/
private async acquire(version: string, installRuntime: boolean, globalInstallerResolver : GlobalInstallerResolver | null = null): Promise<IDotnetAcquireResult>
@ -174,7 +174,7 @@ export class DotnetCoreAcquisitionWorker implements IDotnetCoreAcquisitionWorker
}
}
public static getInstallKeyCustomArchitecture(version : string, architecture: string | null | undefined, isGlobal : boolean = false) : string
public static getInstallKeyCustomArchitecture(version : string, architecture: string | null | undefined, isGlobal = false) : string
{
if(!architecture)
{
@ -196,7 +196,7 @@ export class DotnetCoreAcquisitionWorker implements IDotnetCoreAcquisitionWorker
*
* @param version The version of the object to acquire.
* @param installRuntime true if the request is to install the runtime, false for the SDK.
* @param global false if we're doing a local install, true if we're doing a global install. Only supported for the SDK atm.
* @param installKey The install record / key of the version managed by us.
* @returns the dotnet path of the acquired dotnet.
*
* @remarks it is called "core" because it is the meat of the actual acquisition work; this has nothing to do with .NET core vs framework.

Просмотреть файл

@ -17,6 +17,7 @@ import { DotnetFeatureBandDoesNotExistError,
} from '../EventStream/EventStreamEvents';
import { Debugging } from '../Utils/Debugging';
import { IVersionResolver } from './IVersionResolver';
import { FileUtilities } from '../Utils/FileUtilities';
/* tslint:disable:no-any */
/* tslint:disable:only-arrow-functions */
@ -41,6 +42,8 @@ export class GlobalInstallerResolver {
private proxyUrl : string | undefined;
protected fileUtilities : FileUtilities;
private versionResolver : VersionResolver;
private releasesJsonErrorString = `The API hosting the dotnet releases.json is invalid or has changed and the extension needs to be updated. Invalid API URL: `;
@ -79,6 +82,7 @@ export class GlobalInstallerResolver {
this.versionResolver = new VersionResolver(extensionState, eventStream, timeoutTime, proxyUrl);
this.timeoutSecs = timeoutTime;
this.proxyUrl = proxyUrl;
this.fileUtilities = new FileUtilities();
}
@ -124,7 +128,7 @@ export class GlobalInstallerResolver {
const numberOfPeriods = version.split('.').length - 1;
const indexUrl = this.getIndexUrl(numberOfPeriods === 0 ? `${version}.0` : version);
const indexJsonData = await this.fetchJsonObjectFromUrl(indexUrl);
const fullySpecifiedVersionRequested = indexJsonData![<any>this.releasesLatestSdkKey];
const fullySpecifiedVersionRequested = indexJsonData![(this.releasesLatestSdkKey as any)];
return [await this.findCorrectInstallerUrl(fullySpecifiedVersionRequested, indexUrl), fullySpecifiedVersionRequested];
}
else if(this.versionResolver.isNonSpecificFeatureBandedVersion(version))
@ -166,59 +170,21 @@ export class GlobalInstallerResolver {
throw versionErr.error;
}
const operatingSys : string = os.platform();
const operatingArch : string = os.arch();
let convertedOs = '';
let convertedArch = '';
switch(operatingSys)
const convertedOs = this.fileUtilities.nodeOSToDotnetOS(os.platform(), this.eventStream);
if(convertedOs === 'auto')
{
case 'win32': {
convertedOs = 'win';
break;
}
case 'darwin': {
convertedOs = 'osx';
break;
}
case 'linux': {
convertedOs = operatingSys;
break;
}
default:
{
const osErr = new DotnetUnexpectedInstallerOSError(new Error(`The OS ${operatingSys} is currently unsupported or unknown.`));
this.eventStream.post(osErr);
throw osErr.error;
}
const osErr = new DotnetUnexpectedInstallerOSError(new Error(`The OS ${os.platform()} is currently unsupported or unknown.`));
this.eventStream.post(osErr);
throw osErr.error;
}
switch(operatingArch)
const convertedArch = this.fileUtilities.nodeArchToDotnetArch(os.arch(), this.eventStream);
if(convertedArch === 'auto')
{
case 'x64': {
convertedArch = operatingArch;
break;
}
case 'ia32': {
convertedArch = 'x86';
break;
}
case 'arm': {
convertedArch = operatingArch;
break;
}
case 'arm64': {
convertedArch = operatingArch;
break;
}
default:
{
const archErr = new DotnetUnexpectedInstallerArchitectureError(new Error(`The architecture ${operatingArch} is currently unsupported or unknown.
Your architecture: ${os.arch()}. Your OS: ${os.platform()}.`));
this.eventStream.post(archErr);
throw archErr.error;
}
const archErr = new DotnetUnexpectedInstallerArchitectureError(new Error(`The architecture ${os.arch()} is currently unsupported or unknown.
Your architecture: ${os.arch()}. Your OS: ${os.platform()}.`));
this.eventStream.post(archErr);
throw archErr.error;
}
const desiredRidPackage = `${convertedOs}-${convertedArch}`;
@ -363,7 +329,6 @@ export class GlobalInstallerResolver {
}
}
// TODO: make a test for this error msg
const availableBands = Array.from(new Set(sdks.map((x : any) => this.versionResolver.getFeatureBandFromVersion(x[this.releasesSdkVersionKey]))));
const err = new DotnetFeatureBandDoesNotExistError(new Error(`The feature band '${band}' doesn't exist for the SDK major version '${version}'. Available feature bands for this SDK version are ${availableBands}.`));
this.eventStream.post(err);

Просмотреть файл

@ -397,6 +397,10 @@ export class DotnetCommandFallbackArchitectureEvent extends DotnetCustomMessageE
public readonly eventName = 'DotnetCommandFallbackArchitectureEvent';
}
export class DotnetCommandFallbackOSEvent extends DotnetCustomMessageEvent {
public readonly eventName = 'DotnetCommandFallbackOSEvent';
}
export class DotnetInstallKeyCreatedEvent extends DotnetCustomMessageEvent {
public readonly eventName = 'DotnetInstallKeyCreatedEvent';
}

Просмотреть файл

@ -9,7 +9,14 @@
import * as proc from 'child_process';
import * as lockfile from 'proper-lockfile';
import { IEventStream } from '../EventStream/EventStream';
import { DotnetFileWriteRequestEvent, DotnetLockAcquiredEvent, DotnetLockAttemptingAcquireEvent, DotnetLockErrorEvent, DotnetLockReleasedEvent } from '../EventStream/EventStreamEvents';
import { DotnetCommandFallbackArchitectureEvent,
DotnetCommandFallbackOSEvent,
DotnetFileWriteRequestEvent,
DotnetLockAcquiredEvent,
DotnetLockAttemptingAcquireEvent,
DotnetLockErrorEvent,
DotnetLockReleasedEvent
} from '../EventStream/EventStreamEvents';
export class FileUtilities {
@ -74,7 +81,7 @@ export class FileUtilities {
}
/**
* @param directoryToWipe the directory to delete all of the files in if privellege to do so exists.
* @param directoryToWipe the directory to delete all of the files in if privilege to do so exists.
*/
public wipeDirectory(directoryToWipe : string)
{
@ -89,7 +96,75 @@ export class FileUtilities {
/**
*
* @returns true if the process is running with admin privelleges on windows.
* @param nodeArchitecture the architecture in node style string of what to install
* @returns the architecture in the style that .net / the .net install scripts expect
*
* Node - amd64 is documented as an option for install scripts but its no longer used.
* s390x is also no longer used.
* ppc64le is supported but this version of node has no distinction of the endianness of the process.
* It has no mapping to mips or other node architectures.
*
* @remarks Falls back to string 'auto' if a mapping does not exist which is not a valid architecture.
*/
public nodeArchToDotnetArch(nodeArchitecture : string, eventStream : IEventStream)
{
switch(nodeArchitecture)
{
case 'x64': {
return nodeArchitecture;
}
case 'ia32': {
return 'x86';
}
case 'x86': {
// In case the function is called twice
return 'x86';
}
case 'arm': {
return nodeArchitecture;
}
case 'arm64': {
return nodeArchitecture;
}
case 's390x': {
return 's390x';
}
default: {
eventStream.post(new DotnetCommandFallbackArchitectureEvent(`The architecture ${os.arch()} of the platform is unexpected, falling back to auto-arch.`));
return 'auto';
}
}
}
/**
*
* @param nodeOS the OS in node style string of what to install
* @returns the OS in the style that .net / the .net install scripts expect
*
*/
public nodeOSToDotnetOS(nodeOS : string, eventStream : IEventStream)
{
switch(nodeOS)
{
case 'win32': {
return 'win';
}
case 'darwin': {
return 'osx';
}
case 'linux': {
return nodeOS;
}
default: {
eventStream.post(new DotnetCommandFallbackOSEvent(`The OS ${os.platform()} of the platform is unexpected, falling back to auto-os.`));
return 'auto'
}
}
}
/**
*
* @returns true if the process is running with admin privileges
*/
public isElevated() : boolean
{

Просмотреть файл

@ -45,7 +45,7 @@ suite('Global Installer Resolver Tests', () =>
assert.include(installerUrl, 'pkg');
}
// The architecture in the installer file will match unless its x32, in which case it'll be called x86.
assert.include(installerUrl, (os.arch() === 'x32' ? 'x86' : os.arch()));
assert.include(installerUrl, (os.arch() === 'ia32' ? 'x86' : os.arch()));
});
test('It parses the major format', async () => {

Просмотреть файл

@ -280,7 +280,7 @@ suite('DotnetCoreAcquisitionExtension End to End', function()
if(os.arch() === 'x64')
{
// We check this only on x64 because that matches the build machines and we dont want to duplicate architecture mapping logic
// We check this only on x64 because that matches the build machines and we don't want to duplicate architecture mapping logic
if(os.platform() === 'win32')
{
const expectedWinInstallerUrl = 'https://download.visualstudio.microsoft.com/download/pr/dotnet-sdk-6.0.311-win-x64.exe';
@ -313,7 +313,7 @@ suite('DotnetCoreAcquisitionExtension End to End', function()
let error : any;
let pathAfterInstall;
// We cannot test much as we don't want to leave global installs on devboxes. But we do want to make sure the e-2-e goes through the right path. Vendors can test the rest.
// We cannot test much as we don't want to leave global installs on dev boxes. But we do want to make sure the e-2-e goes through the right path. Vendors can test the rest.
// So we have this environment variable that tells us to stop before running any real install.
process.env.VSCODE_DOTNET_GLOBAL_INSTALL_FAKE_PATH = 'true';
try

Просмотреть файл

@ -46,6 +46,7 @@ const extensionConfig = {
new CopyPlugin({ patterns: [
{ from: path.resolve(__dirname, '../vscode-dotnet-runtime-library/install scripts'), to: path.resolve(__dirname, 'dist', 'install scripts') },
{ from: path.resolve(__dirname, '../vscode-dotnet-runtime-library/distro-data'), to: path.resolve(__dirname, 'dist', 'distro-data') },
{ from: path.resolve(__dirname, '../images'), to: path.resolve(__dirname, 'images') },
{ from: path.resolve(__dirname, '../LICENSE.txt'), to: path.resolve(__dirname, 'LICENSE.txt') }
]}),
]