Skip to content

Commit

Permalink
Default string settings to the empty string
Browse files Browse the repository at this point in the history
  • Loading branch information
50Wliu committed Apr 25, 2022
1 parent 43bacd3 commit 1acc7dc
Show file tree
Hide file tree
Showing 9 changed files with 37 additions and 55 deletions.
2 changes: 1 addition & 1 deletion src/features/dotnetTest.ts
Original file line number Diff line number Diff line change
Expand Up @@ -194,7 +194,7 @@ export default class TestManager extends AbstractProvider {

private _getRunSettings(filename: string): string | undefined {
const testSettingsPath = this.optionProvider.GetLatestOptions().testRunSettings;
if (!testSettingsPath) {
if (testSettingsPath.length === 0) {
return undefined;
}

Expand Down
2 changes: 1 addition & 1 deletion src/omnisharp/OmniSharpDotnetResolver.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ export class OmniSharpDotnetResolver implements IHostExecutableResolver {
const dotnet = this.platformInfo.isWindows() ? 'dotnet.exe' : 'dotnet';
const env = { ...process.env };

if (options.dotnetPath) {
if (options.dotnetPath.length > 0) {
env['PATH'] = options.dotnetPath + path.delimiter + env['PATH'];
}

Expand Down
5 changes: 2 additions & 3 deletions src/omnisharp/OmniSharpMonoResolver.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ export class OmniSharpMonoResolver implements IHostExecutableResolver {
private async configureEnvironmentAndGetInfo(options: Options): Promise<HostExecutableInformation> {
let env = { ...process.env };
let monoPath: string;
if (options.useGlobalMono !== "never" && options.monoPath !== undefined) {
if (options.useGlobalMono !== "never" && options.monoPath.length > 0) {
env['PATH'] = path.join(options.monoPath, 'bin') + path.delimiter + env['PATH'];
env['MONO_GAC_PREFIX'] = options.monoPath;
monoPath = options.monoPath;
Expand All @@ -40,7 +40,7 @@ export class OmniSharpMonoResolver implements IHostExecutableResolver {
if (options.useGlobalMono === "always") {
let isMissing = monoInfo.version === undefined;
if (isMissing) {
const suggestedAction = options.monoPath
const suggestedAction = options.monoPath.length > 0
? "Update the \"omnisharp.monoPath\" setting to point to the folder containing Mono's '/bin' folder."
: "Ensure that Mono's '/bin' folder is added to your environment's PATH variable.";
throw new Error(`Unable to find Mono. ${suggestedAction}`);
Expand All @@ -61,4 +61,3 @@ export class OmniSharpMonoResolver implements IHostExecutableResolver {
return undefined;
}
}

4 changes: 2 additions & 2 deletions src/omnisharp/OmnisharpManager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,8 @@ export class OmnisharpManager {
private platformInfo: PlatformInformation) {
}

public async GetOmniSharpLaunchInfo(defaultOmnisharpVersion: string, omnisharpPath: string | undefined, useFramework: boolean, serverUrl: string, latestVersionFileServerPath: string, installPath: string, extensionPath: string): Promise<LaunchInfo> {
if (omnisharpPath === undefined || omnisharpPath.length === 0) {
public async GetOmniSharpLaunchInfo(defaultOmnisharpVersion: string, omnisharpPath: string, useFramework: boolean, serverUrl: string, latestVersionFileServerPath: string, installPath: string, extensionPath: string): Promise<LaunchInfo> {
if (omnisharpPath.length === 0) {
// If omnisharpPath was not specified, return the default path.
const basePath = path.resolve(extensionPath, '.omnisharp', defaultOmnisharpVersion + (useFramework ? '' : `-net${modernNetVersion}`));
return this.GetLaunchInfo(this.platformInfo, useFramework, basePath);
Expand Down
43 changes: 22 additions & 21 deletions src/omnisharp/options.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import { vscode, WorkspaceConfiguration } from '../vscodeAdapter';

export class Options {
constructor(
public path: string | undefined,
public path: string,
public useModernNet: boolean,
public useGlobalMono: string,
public waitForDebugger: boolean,
Expand Down Expand Up @@ -48,13 +48,13 @@ export class Options {
public inlayHintsForImplicitVariableTypes: boolean,
public inlayHintsForLambdaParameterTypes: boolean,
public inlayHintsForImplicitObjectCreation: boolean,
public razorPluginPath: string | undefined,
public defaultLaunchSolution: string | undefined,
public monoPath: string | undefined,
public dotnetPath: string | undefined,
public razorPluginPath: string,
public defaultLaunchSolution: string,
public monoPath: string,
public dotnetPath: string,
public excludePaths: string[],
public maxProjectFileCountForDiagnosticAnalysis: number,
public testRunSettings: string | undefined) {
public testRunSettings: string) {
}

public static Read(vscode: vscode): Options {
Expand All @@ -72,8 +72,13 @@ export class Options {
const path = Options.readPathOption(csharpConfig, omnisharpConfig);
const useModernNet = omnisharpConfig.get<boolean>("useModernNet", false);
const useGlobalMono = Options.readUseGlobalMonoOption(omnisharpConfig, csharpConfig);
const monoPath = omnisharpConfig.get<string>('monoPath');
const dotnetPath = omnisharpConfig.get<string>('dotnetPath');

// VS Code coerces unset string settings to the empty string.
// Thus, to avoid dealing with the empty string AND undefined,
// explicitly pass in the empty string as the fallback if the setting
// isn't defined in package.json (which should never happen).
const monoPath = omnisharpConfig.get<string>('monoPath', '');
const dotnetPath = omnisharpConfig.get<string>('dotnetPath', '');

const waitForDebugger = omnisharpConfig.get<boolean>('waitForDebugger', false);

Expand All @@ -87,7 +92,7 @@ export class Options {

const projectLoadTimeout = omnisharpConfig.get<number>('projectLoadTimeout', 60);
const maxProjectResults = omnisharpConfig.get<number>('maxProjectResults', 250);
const defaultLaunchSolution = omnisharpConfig.get<string>('defaultLaunchSolution');
const defaultLaunchSolution = omnisharpConfig.get<string>('defaultLaunchSolution', '');
const useEditorFormattingSettings = omnisharpConfig.get<boolean>('useEditorFormattingSettings', true);

const enableRoslynAnalyzers = omnisharpConfig.get<boolean>('enableRoslynAnalyzers', false);
Expand Down Expand Up @@ -132,11 +137,11 @@ export class Options {

const razorDisabled = razorConfig?.get<boolean>('disabled', false) ?? false;
const razorDevMode = razorConfig?.get<boolean>('devmode', false) ?? false;
const razorPluginPath = razorConfig?.get<string>('plugin.path') ?? undefined;
const razorPluginPath = razorConfig?.get<string>('plugin.path', '') ?? '';

const maxProjectFileCountForDiagnosticAnalysis = csharpConfig.get<number>('maxProjectFileCountForDiagnosticAnalysis', 1000);

const testRunSettings = omnisharpConfig.get<string>('testRunSettings');
const testRunSettings = omnisharpConfig.get<string>('testRunSettings', '');

const excludePaths = this.getExcludedPaths(vscode);

Expand Down Expand Up @@ -204,29 +209,25 @@ export class Options {
return excludePaths;

function getExcludes(config: WorkspaceConfiguration, option: string): string[] {
const optionValue = config.get<{ [i: string]: boolean }>(option);
if (optionValue === undefined) {
return [];
}

const optionValue = config.get<{ [i: string]: boolean }>(option, {});
return Object.entries(optionValue)
.filter(([key, value]) => value)
.map(([key, value]) => key);
}
}

private static readPathOption(csharpConfig: WorkspaceConfiguration, omnisharpConfig: WorkspaceConfiguration): string | undefined {
private static readPathOption(csharpConfig: WorkspaceConfiguration, omnisharpConfig: WorkspaceConfiguration): string {
if (omnisharpConfig.has('path')) {
// If 'omnisharp.path' setting was found, use it.
return omnisharpConfig.get<string>('path');
return omnisharpConfig.get<string>('path', '');
}
else if (csharpConfig.has('omnisharp')) {
// BACKCOMPAT: If 'csharp.omnisharp' setting was found, use it.
return csharpConfig.get<string>('omnisharp');
return csharpConfig.get<string>('omnisharp', '');
}
else {
// Otherwise, undefined.
return undefined;
// Otherwise, the empty string.
return '';
}
}

Expand Down
2 changes: 1 addition & 1 deletion src/omnisharp/server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -342,7 +342,7 @@ export class OmniSharpServer {

if (!options.razorDisabled) {
// Razor support only exists for certain platforms, so only load the plugin if present
const razorPluginPath = options.razorPluginPath ?? path.join(
const razorPluginPath = options.razorPluginPath.length > 0 ? options.razorPluginPath : path.join(
this.extensionPath,
'.razor',
'OmniSharpPlugin',
Expand Down
18 changes: 0 additions & 18 deletions test/unitTests/OmnisharpManager.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -135,24 +135,6 @@ suite(OmnisharpManager.name, () => {
expect(launchInfo.LaunchPath).to.be.equal(tmpFile.name);
});

test('Returns the default path if the omnisharp path is not set', async () => {
const launchInfo = await manager.GetOmniSharpLaunchInfo(defaultVersion, undefined, useFramework, server.baseUrl, latestfilePath, installPath, extensionPath);
if (useFramework) {
expect(launchInfo.LaunchPath).to.be.equal(path.join(extensionPath, ".omnisharp", defaultVersion + suffix, elem.executable));
if (elem.platformInfo.isWindows()) {
expect(launchInfo.MonoLaunchPath).to.be.undefined;
}
else {
expect(launchInfo.MonoLaunchPath).to.be.equal(path.join(extensionPath, ".omnisharp", defaultVersion, "omnisharp", "OmniSharp.exe"));
}
}
else {
expect(launchInfo.LaunchPath).to.be.undefined;
expect(launchInfo.MonoLaunchPath).to.be.undefined;
expect(launchInfo.DotnetLaunchPath).to.be.equal(path.join(extensionPath, ".omnisharp", defaultVersion + suffix, elem.executable));
}
});

test('Returns the default path if the omnisharp path is empty', async () => {
const launchInfo = await manager.GetOmniSharpLaunchInfo(defaultVersion, "", useFramework, server.baseUrl, latestfilePath, installPath, extensionPath);
if (useFramework) {
Expand Down
8 changes: 4 additions & 4 deletions test/unitTests/optionStream.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
* Licensed under the MIT License. See License.txt in the project root for license information.
*--------------------------------------------------------------------------------------------*/

import { should, expect } from 'chai';
import { should } from 'chai';
import { ConfigurationChangeEvent, vscode } from "../../src/vscodeAdapter";
import { getVSCodeWithConfig, updateConfig } from "./testAssets/Fakes";
import Disposable from "../../src/Disposable";
Expand Down Expand Up @@ -36,7 +36,7 @@ suite('OptionStream', () => {
});

test('Returns the default options if there is no change', () => {
expect(options.path).to.be.undefined;
options.path.should.equal("");
options.useGlobalMono.should.equal("auto");
options.waitForDebugger.should.equal(false);
options.loggingLevel.should.equal("information");
Expand All @@ -56,11 +56,11 @@ suite('OptionStream', () => {
options.enableDecompilationSupport.should.equal(false);
options.enableImportCompletion.should.equal(false);
options.enableAsyncCompletion.should.equal(false);
expect(options.defaultLaunchSolution).to.be.undefined;
options.defaultLaunchSolution.should.equal("");
});

test('Gives the changed option when the omnisharp config changes', () => {
expect(options.path).to.be.undefined;
options.path.should.equal("");
let changingConfig = "omnisharp";
updateConfig(vscode, changingConfig, 'path', "somePath");
listenerFunction.forEach(listener => listener(GetConfigChangeEvent(changingConfig)));
Expand Down
8 changes: 4 additions & 4 deletions test/unitTests/options.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,9 @@ suite("Options tests", () => {
test('Verify defaults', () => {
const vscode = getVSCodeWithConfig();
const options = Options.Read(vscode);
expect(options.path).to.be.undefined;
options.path.should.equal("");
options.useGlobalMono.should.equal("auto");
expect(options.monoPath).to.be.undefined;
options.monoPath.should.equal("");
options.waitForDebugger.should.equal(false);
options.loggingLevel.should.equal("information");
options.autoStart.should.equal(true);
Expand All @@ -35,8 +35,8 @@ suite("Options tests", () => {
options.enableDecompilationSupport.should.equal(false);
options.enableImportCompletion.should.equal(false);
options.analyzeOpenDocumentsOnly.should.equal(false);
expect(options.testRunSettings).to.be.undefined;
expect(options.defaultLaunchSolution).to.be.undefined;
options.testRunSettings.should.equal("");
options.defaultLaunchSolution.should.equal("");
});

test('Verify return no excluded paths when files.exclude empty', () => {
Expand Down

0 comments on commit 1acc7dc

Please sign in to comment.