From 2c3eab35be56fb7ccbee26bf9d43df70e2ea7fbb Mon Sep 17 00:00:00 2001 From: John Gee Date: Sat, 6 Jun 2020 11:52:50 +1200 Subject: [PATCH 1/6] Make a start on warning for option name clashes with Command properties --- index.js | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/index.js b/index.js index 7134a3865..be82e27d1 100644 --- a/index.js +++ b/index.js @@ -110,6 +110,7 @@ class Command extends EventEmitter { this._name = name || ''; this._optionValues = {}; this._storeOptionsAsProperties = true; // backwards compatible by default + this._storeOptionsAsPropertiesCalled = false; this._passCommandToAction = true; // backwards compatible by default this._actionResults = []; this._actionHandler = null; @@ -440,6 +441,12 @@ class Command extends EventEmitter { const name = option.attributeName(); option.mandatory = !!config.mandatory; + if (!this._storeOptionsAsPropertiesCalled && this[oname] !== undefined && !option.negate) { + throw new Error(`option name '${oname}' clashes with existing property on Command + - either call storeOptionsAsProperties(false) and use .opts() for option values, + - or call storeOptionsAsProperties(true) to suppress this error and continue storing option values as properties on Command`); + } + // default as 3rd arg if (typeof fn !== 'function') { if (fn instanceof RegExp) { @@ -596,6 +603,7 @@ class Command extends EventEmitter { */ storeOptionsAsProperties(value) { + this._storeOptionsAsPropertiesCalled = true; this._storeOptionsAsProperties = (value === undefined) || value; if (this.options.length) { throw new Error('call .storeOptionsAsProperties() before adding options'); From 825f6a0cc97309e3353958b5e9fc9ecbcf8c98c5 Mon Sep 17 00:00:00 2001 From: John Gee Date: Sun, 7 Jun 2020 09:45:50 +1200 Subject: [PATCH 2/6] Use correct property name for lookup --- index.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/index.js b/index.js index be82e27d1..c30ea9746 100644 --- a/index.js +++ b/index.js @@ -441,8 +441,8 @@ class Command extends EventEmitter { const name = option.attributeName(); option.mandatory = !!config.mandatory; - if (!this._storeOptionsAsPropertiesCalled && this[oname] !== undefined && !option.negate) { - throw new Error(`option name '${oname}' clashes with existing property on Command + if (!this._storeOptionsAsPropertiesCalled && this[name] !== undefined && !option.negate) { + throw new Error(`option name '${name}' clashes with existing property on Command - either call storeOptionsAsProperties(false) and use .opts() for option values, - or call storeOptionsAsProperties(true) to suppress this error and continue storing option values as properties on Command`); } From 8bf6e858cf692f93ff374fae61d38cf4ef9ef945 Mon Sep 17 00:00:00 2001 From: John Gee Date: Sun, 7 Jun 2020 19:35:30 +1200 Subject: [PATCH 3/6] Shift clash detection into routine, still WIP --- index.js | 38 +++++++++++++++++++++++++++++++++----- 1 file changed, 33 insertions(+), 5 deletions(-) diff --git a/index.js b/index.js index c30ea9746..f75de007b 100644 --- a/index.js +++ b/index.js @@ -423,6 +423,38 @@ class Command extends EventEmitter { return this; }; + /** + * Internal routine to check whether there is a clash storing option value with a Command property. + * + * @param {Option} option + * @api private + */ + + _checkForOptionNameClash(option) { + if (!this._storeOptionsAsProperties || this._storeOptionsAsPropertiesCalled) { + // Storing options safely, or user has been explicit and up to them. + return; + } + // User may override help, and hard to tell if worth warning. + if (option.name() === 'help') { + return; + } + + const commandProperty = this._getOptionValue(option.attributeName()); + if (commandProperty === undefined) { + return; + } + + let foundClash = false; + foundClash = true; + + if (foundClash) { + throw new Error(`option '${option.name()}' clashes with existing property on Command +- either call storeOptionsAsProperties(false) and use .opts() for option values, +- or call storeOptionsAsProperties(true) to suppress this error and continue storing option values as properties on Command`); + } + }; + /** * Internal implementation shared by .option() and .requiredOption() * @@ -441,11 +473,7 @@ class Command extends EventEmitter { const name = option.attributeName(); option.mandatory = !!config.mandatory; - if (!this._storeOptionsAsPropertiesCalled && this[name] !== undefined && !option.negate) { - throw new Error(`option name '${name}' clashes with existing property on Command - - either call storeOptionsAsProperties(false) and use .opts() for option values, - - or call storeOptionsAsProperties(true) to suppress this error and continue storing option values as properties on Command`); - } + this._checkForOptionNameClash(option); // default as 3rd arg if (typeof fn !== 'function') { From 49e28faf464c7c3ef5441f12e863c2376b62778c Mon Sep 17 00:00:00 2001 From: John Gee Date: Sun, 7 Jun 2020 20:39:45 +1200 Subject: [PATCH 4/6] Prevent false positive clashes when negated option --- index.js | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/index.js b/index.js index b59efa02f..7920e4318 100644 --- a/index.js +++ b/index.js @@ -450,16 +450,25 @@ class Command extends EventEmitter { const commandProperty = this._getOptionValue(option.attributeName()); if (commandProperty === undefined) { + // no clash return; } - let foundClash = false; - foundClash = true; + let foundClash = true; + if (option.negate) { + // It is ok if define foo before --no-foo. + const positiveLongFlag = option.long.replace(/^--no-/, '--'); + foundClash = !this._findOption(positiveLongFlag); + } else if (option.long) { + const negativeLongFlag = option.long.replace(/^--/, '--no-'); + foundClash = !this._findOption(negativeLongFlag); + } if (foundClash) { - throw new Error(`option '${option.name()}' clashes with existing property on Command -- either call storeOptionsAsProperties(false) and use .opts() for option values, -- or call storeOptionsAsProperties(true) to suppress this error and continue storing option values as properties on Command`); + throw new Error(`option '${option.name()}' clashes with existing property '${option.attributeName()}' on Command +- either call storeOptionsAsProperties(false) and use .opts() to access option values, +- or call storeOptionsAsProperties(true) to suppress this check, +- or change option name`); } }; From 2a8150115e114d944a8ea22425ce295941ffd892 Mon Sep 17 00:00:00 2001 From: John Gee Date: Sun, 7 Jun 2020 20:40:20 +1200 Subject: [PATCH 5/6] Add tests for option name clashes --- tests/options.detectClash.test.js | 52 +++++++++++++++++++++++++++++++ 1 file changed, 52 insertions(+) create mode 100644 tests/options.detectClash.test.js diff --git a/tests/options.detectClash.test.js b/tests/options.detectClash.test.js new file mode 100644 index 000000000..e9e9efd30 --- /dev/null +++ b/tests/options.detectClash.test.js @@ -0,0 +1,52 @@ +const commander = require('../'); + +// Check detection of likely name clashes + +test('when option clashes with property then throw', () => { + const program = new commander.Command(); + expect(() => { + program.option('-n, --name '); + }).toThrow(); +}); + +test('when option clashes with property and storeOptionsAsProperties(true) then ok', () => { + const program = new commander.Command(); + program.storeOptionsAsProperties(true); + expect(() => { + program.option('-n, --name '); + }).not.toThrow(); +}); + +test('when option would clash with property but storeOptionsAsProperties(false) then ok', () => { + const program = new commander.Command(); + program.storeOptionsAsProperties(false); + expect(() => { + program.option('-n, --name '); + }).not.toThrow(); +}); + +test('when negated option clashes with property then throw', () => { + const program = new commander.Command(); + expect(() => { + program.option('-E, --no-emit'); + }).toThrow(); +}); + +test('when positive and negative option then ok', () => { + const program = new commander.Command(); + expect(() => { + program + .option('-c, --colour', 'red') + .option('-C, --no-colour'); + }).not.toThrow(); +}); + +test('when negative and positive option then ok', () => { + // Not a likely pattern, but possible and not an error. + const program = new commander.Command(); + expect(() => { + program + .option('-C, --no-colour') + .option('-c, --colour'); + }).not.toThrow(); +}); From 544602a1f0a9d9ff8bced80a7e279475aa5d455d Mon Sep 17 00:00:00 2001 From: John Gee Date: Sun, 7 Jun 2020 21:12:05 +1200 Subject: [PATCH 6/6] Refine advice --- index.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/index.js b/index.js index 7920e4318..e73254f7f 100644 --- a/index.js +++ b/index.js @@ -466,7 +466,7 @@ class Command extends EventEmitter { if (foundClash) { throw new Error(`option '${option.name()}' clashes with existing property '${option.attributeName()}' on Command -- either call storeOptionsAsProperties(false) and use .opts() to access option values, +- call storeOptionsAsProperties(false) to store option values safely, - or call storeOptionsAsProperties(true) to suppress this check, - or change option name`); }