From ebc72e523463844ee5cee87f5fef3424d70db4b2 Mon Sep 17 00:00:00 2001 From: "Justin M. Keyes" Date: Thu, 24 Aug 2023 08:49:40 -0700 Subject: [PATCH] feat(settings): don't throw exception if default is provided Problem: If a setting value is invalid, `Settings.instance.get` throws an exception, even if a `defaultValue` is provided. We almost never want that behavior. Example: Settings.instance.get('files.exclude', Number, 42) Solution: Copy the logic from `AnonymousSettings.get()`. --- src/shared/settings.ts | 63 +++++++++++++------ src/test/shared/settingsConfiguration.test.ts | 4 ++ 2 files changed, 48 insertions(+), 19 deletions(-) diff --git a/src/shared/settings.ts b/src/shared/settings.ts index 98efdf02c90..866b4fc687b 100644 --- a/src/shared/settings.ts +++ b/src/shared/settings.ts @@ -31,19 +31,35 @@ export class Settings { ) {} /** - * Reads a setting, applying the {@link TypeConstructor} if provided. + * Gets a setting value, applying the {@link TypeConstructor} if provided. * - * Note that the absence of a key is indistinguisable from the absence of a value. - * That is, `undefined` looks the same across all calls. Any non-existent values are - * simply returned as-is without passing through the type cast. + * If the read fails or the setting value is invalid (does not conform to `type`): + * - `defaultValue` is returned if it was provided + * - else an exception is thrown + * + * @note An unknown setting is indistinguisable from a missing value: both return `undefined`. + * Non-existent values are returned as-is without passing through the type cast. + * + * @param key Setting name + * @param type Expected setting type + * @param defaultValue Value returned if setting is missing or invalid */ public get(key: string): unknown public get(key: string, type: TypeConstructor): T | undefined public get(key: string, type: TypeConstructor, defaultValue: T): T public get(key: string, type?: TypeConstructor, defaultValue?: T) { - const value = this.getConfig().get(key, defaultValue) + try { + const value = this.getConfig().get(key, defaultValue) - return !type || value === undefined ? value : cast(value, type) + return !type || value === undefined ? value : cast(value, type) + } catch (e) { + if (arguments.length <= 2) { + throw ToolkitError.chain(e, `Failed to read setting "${key}"`) + } + getLogger().error('settings: failed to read "%s": %s', key, (e as Error).message) + + return defaultValue + } } /** @@ -62,8 +78,8 @@ export class Settings { await this.getConfig().update(key, value, this.updateTarget) return true - } catch (error) { - getLogger().warn(`Settings: failed to update "${key}": %s`, error) + } catch (e) { + getLogger().warn('settings: failed to update "%s": %s', key, (e as Error).message) return false } @@ -224,6 +240,16 @@ function createSettingsClass(section: string, descript return this.config.keys() } + /** + * Gets a setting value. + * + * If the read fails or the setting value is invalid (does not conform to the type defined in package.json): + * - `defaultValue` is returned if it was provided + * - else an exception is thrown + * + * @param key Setting name + * @param defaultValue Value returned if setting is missing or invalid + */ public get(key: K & string, defaultValue?: Inner[K]) { try { return this.getOrThrow(key, defaultValue) @@ -231,8 +257,7 @@ function createSettingsClass(section: string, descript if (arguments.length === 1) { throw ToolkitError.chain(e, `Failed to read key "${section}.${key}"`) } - - this.logErr(`using default for key "${key}", read failed: %s`, e) + this.logErr('failed to read "%s": %s', key, (e as Error).message) return defaultValue as Inner[K] } @@ -243,8 +268,8 @@ function createSettingsClass(section: string, descript await this.config.update(key, value) return true - } catch (error) { - this.log(`failed to update field "${key}": %s`, error) + } catch (e) { + this.log('failed to update "%s": %s', key, (e as Error).message) return false } @@ -255,8 +280,8 @@ function createSettingsClass(section: string, descript await this.config.update(key, undefined) return true - } catch (error) { - this.log(`failed to delete field "${key}": %s`, error) + } catch (e) { + this.log('failed to delete "%s": %s', key, (e as Error).message) return false } @@ -265,8 +290,8 @@ function createSettingsClass(section: string, descript public async reset() { try { return await this.config.reset() - } catch (error) { - this.log(`failed to reset settings: %s`, error) + } catch (e) { + this.log('failed to reset settings: %s', e) } } @@ -317,7 +342,7 @@ function createSettingsClass(section: string, descript } for (const key of props.filter(isDifferent)) { - this.log(`key "${key}" changed`) + this.log('key "%s" changed', key) emitter.fire({ key }) } }) @@ -509,8 +534,8 @@ export class PromptSettings extends Settings.define( public async isPromptEnabled(promptName: PromptName): Promise { try { return !this.getOrThrow(promptName, false) - } catch (error) { - this.log(`prompt check for "${promptName}" failed: %s`, error) + } catch (e) { + this.log('prompt check for "%s" failed: %s', promptName, (e as Error).message) await this.reset() return true diff --git a/src/test/shared/settingsConfiguration.test.ts b/src/test/shared/settingsConfiguration.test.ts index 8284cc87a05..be6ad9f8ccc 100644 --- a/src/test/shared/settingsConfiguration.test.ts +++ b/src/test/shared/settingsConfiguration.test.ts @@ -72,6 +72,10 @@ describe('Settings', function () { assert.throws(() => sut.get(settingKey, String)) assert.throws(() => sut.get(settingKey, Object)) assert.throws(() => sut.get(settingKey, Boolean)) + // Wrong type, but defaultValue was given: + assert.deepStrictEqual(sut.get(settingKey, String, ''), '') + assert.deepStrictEqual(sut.get(settingKey, Object, {}), {}) + assert.deepStrictEqual(sut.get(settingKey, Boolean, true), true) }) })