From 4f5ab2de1ba232893ed312816859c7eb9f7c6a98 Mon Sep 17 00:00:00 2001 From: Wee Bit Date: Sat, 5 Aug 2023 02:20:56 +0300 Subject: [PATCH 1/6] Add missing checks for passThroughOptions --- lib/command.js | 21 ++++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-) diff --git a/lib/command.js b/lib/command.js index 590a271dd..ac8189866 100644 --- a/lib/command.js +++ b/lib/command.js @@ -272,6 +272,8 @@ class Command extends EventEmitter { this.commands.push(cmd); cmd.parent = this; + this._checkForIllegalPassThroughOptions(cmd, this); + return this; } @@ -721,6 +723,9 @@ Expecting one of '${allowedValues.join("', '")}'`); */ enablePositionalOptions(positional = true) { this._enablePositionalOptions = !!positional; + this.commands.forEach((command) => { + this._checkForIllegalPassThroughOptions(command, this); + }); return this; } @@ -735,12 +740,22 @@ Expecting one of '${allowedValues.join("', '")}'`); */ passThroughOptions(passThrough = true) { this._passThroughOptions = !!passThrough; - if (!!this.parent && passThrough && !this.parent._enablePositionalOptions) { - throw new Error('passThroughOptions can not be used without turning on enablePositionalOptions for parent command(s)'); - } + this._checkForIllegalPassThroughOptions(this, this.parent); return this; } + /** + * @param {Command} command + * @param {Command | null} parent + * @api private + */ + + _checkForIllegalPassThroughOptions(command, parent) { + if (parent && command._passThroughOptions && !parent._enablePositionalOptions) { + throw new Error(`passThroughOptions cannot be used for '${command._name}' without turning on enablePositionalOptions for parent command${parent._name ? ` '${parent._name}'` : ''}`); + } + } + /** * Whether to store option values as properties on command object, * or store separately (specify false). In both cases the option values can be accessed using .opts(). From 0ac660905f0dfd4ec1ea6d31ddbf808ad5a4ac5d Mon Sep 17 00:00:00 2001 From: Wee Bit Date: Sat, 5 Aug 2023 02:35:29 +0300 Subject: [PATCH 2/6] Add tests for illegal passThroughOptions --- tests/command.positionalOptions.test.js | 35 ++++++++++++++++++++----- 1 file changed, 29 insertions(+), 6 deletions(-) diff --git a/tests/command.positionalOptions.test.js b/tests/command.positionalOptions.test.js index d130b7835..169866d20 100644 --- a/tests/command.positionalOptions.test.js +++ b/tests/command.positionalOptions.test.js @@ -362,13 +362,36 @@ describe('program with action handler and positionalOptions and subcommand', () // ------------------------------------------------------------------------------ -test('when program not positional and turn on passthrough in subcommand then error', () => { - const program = new commander.Command(); - const sub = program.command('sub'); +describe('illegal passThroughOptions', () => { + test('when program not positional and turn on passThroughOptions in subcommand then error', () => { + const program = new commander.Command(); + const sub = program.command('sub'); + + expect(() => { + sub.passThroughOptions(); + }).toThrow(); + }); + + test('when program not positional and add subcommand with passThroughOptions then error', () => { + const program = new commander.Command(); + const sub = new commander.Command('sub') + .passThroughOptions(); - expect(() => { - sub.passThroughOptions(); - }).toThrow(); + expect(() => { + program.addCommand(sub); + }).toThrow(); + }); + + test('when program has subcommand with passThroughOptions and reset to non-positional then error', () => { + const program = new commander.Command() + .enablePositionalOptions(); + program.command('sub') + .passThroughOptions(); + + expect(() => { + program.enablePositionalOptions(false); + }).toThrow(); + }); }); // ------------------------------------------------------------------------------ From 9d4c96a4fc40590dced784d24f26661d183fb512 Mon Sep 17 00:00:00 2001 From: Wee Bit Date: Sat, 5 Aug 2023 03:14:33 +0300 Subject: [PATCH 3/6] Remove illegal passThroughOptions check deemed unnecessary --- lib/command.js | 3 --- tests/command.positionalOptions.test.js | 11 ----------- 2 files changed, 14 deletions(-) diff --git a/lib/command.js b/lib/command.js index ac8189866..e01586a1d 100644 --- a/lib/command.js +++ b/lib/command.js @@ -723,9 +723,6 @@ Expecting one of '${allowedValues.join("', '")}'`); */ enablePositionalOptions(positional = true) { this._enablePositionalOptions = !!positional; - this.commands.forEach((command) => { - this._checkForIllegalPassThroughOptions(command, this); - }); return this; } diff --git a/tests/command.positionalOptions.test.js b/tests/command.positionalOptions.test.js index 169866d20..73dd7a877 100644 --- a/tests/command.positionalOptions.test.js +++ b/tests/command.positionalOptions.test.js @@ -381,17 +381,6 @@ describe('illegal passThroughOptions', () => { program.addCommand(sub); }).toThrow(); }); - - test('when program has subcommand with passThroughOptions and reset to non-positional then error', () => { - const program = new commander.Command() - .enablePositionalOptions(); - program.command('sub') - .passThroughOptions(); - - expect(() => { - program.enablePositionalOptions(false); - }).toThrow(); - }); }); // ------------------------------------------------------------------------------ From 43d9faa402345f0424ee642203535b2c1aa25bb2 Mon Sep 17 00:00:00 2001 From: Wee Bit Date: Sat, 5 Aug 2023 03:15:42 +0300 Subject: [PATCH 4/6] Use weaker wording: "broken" instead of "illegal" --- lib/command.js | 6 +++--- tests/command.positionalOptions.test.js | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/command.js b/lib/command.js index e01586a1d..d190512ed 100644 --- a/lib/command.js +++ b/lib/command.js @@ -272,7 +272,7 @@ class Command extends EventEmitter { this.commands.push(cmd); cmd.parent = this; - this._checkForIllegalPassThroughOptions(cmd, this); + this._checkForBrokenPassThrough(cmd, this); return this; } @@ -737,7 +737,7 @@ Expecting one of '${allowedValues.join("', '")}'`); */ passThroughOptions(passThrough = true) { this._passThroughOptions = !!passThrough; - this._checkForIllegalPassThroughOptions(this, this.parent); + this._checkForBrokenPassThrough(this, this.parent); return this; } @@ -747,7 +747,7 @@ Expecting one of '${allowedValues.join("', '")}'`); * @api private */ - _checkForIllegalPassThroughOptions(command, parent) { + _checkForBrokenPassThrough(command, parent) { if (parent && command._passThroughOptions && !parent._enablePositionalOptions) { throw new Error(`passThroughOptions cannot be used for '${command._name}' without turning on enablePositionalOptions for parent command${parent._name ? ` '${parent._name}'` : ''}`); } diff --git a/tests/command.positionalOptions.test.js b/tests/command.positionalOptions.test.js index 73dd7a877..82011e136 100644 --- a/tests/command.positionalOptions.test.js +++ b/tests/command.positionalOptions.test.js @@ -362,7 +362,7 @@ describe('program with action handler and positionalOptions and subcommand', () // ------------------------------------------------------------------------------ -describe('illegal passThroughOptions', () => { +describe('broken passThrough', () => { test('when program not positional and turn on passThroughOptions in subcommand then error', () => { const program = new commander.Command(); const sub = program.command('sub'); From 43cc821cb89da5b804713a9472b860187c090722 Mon Sep 17 00:00:00 2001 From: aweebit Date: Sat, 5 Aug 2023 03:42:38 +0300 Subject: [PATCH 5/6] Unclutter error message in broken passThrough checks Co-authored-by: John Gee --- lib/command.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/command.js b/lib/command.js index d190512ed..65559112a 100644 --- a/lib/command.js +++ b/lib/command.js @@ -749,7 +749,7 @@ Expecting one of '${allowedValues.join("', '")}'`); _checkForBrokenPassThrough(command, parent) { if (parent && command._passThroughOptions && !parent._enablePositionalOptions) { - throw new Error(`passThroughOptions cannot be used for '${command._name}' without turning on enablePositionalOptions for parent command${parent._name ? ` '${parent._name}'` : ''}`); + throw new Error(`passThroughOptions cannot be used for '${command._name}' without turning on enablePositionalOptions for parent command(s)`); } } From 7689506699b9db61a121baea40ba455e8d89c78d Mon Sep 17 00:00:00 2001 From: Wee Bit Date: Sat, 5 Aug 2023 04:01:21 +0300 Subject: [PATCH 6/6] Refactor _checkForBrokenPassThrough() to make it instance-aware --- lib/command.js | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/lib/command.js b/lib/command.js index 65559112a..e4b5d1699 100644 --- a/lib/command.js +++ b/lib/command.js @@ -272,7 +272,7 @@ class Command extends EventEmitter { this.commands.push(cmd); cmd.parent = this; - this._checkForBrokenPassThrough(cmd, this); + cmd._checkForBrokenPassThrough(); return this; } @@ -737,19 +737,17 @@ Expecting one of '${allowedValues.join("', '")}'`); */ passThroughOptions(passThrough = true) { this._passThroughOptions = !!passThrough; - this._checkForBrokenPassThrough(this, this.parent); + this._checkForBrokenPassThrough(); return this; } /** - * @param {Command} command - * @param {Command | null} parent * @api private */ - _checkForBrokenPassThrough(command, parent) { - if (parent && command._passThroughOptions && !parent._enablePositionalOptions) { - throw new Error(`passThroughOptions cannot be used for '${command._name}' without turning on enablePositionalOptions for parent command(s)`); + _checkForBrokenPassThrough() { + if (this.parent && this._passThroughOptions && !this.parent._enablePositionalOptions) { + throw new Error(`passThroughOptions cannot be used for '${this._name}' without turning on enablePositionalOptions for parent command(s)`); } }