From 42f61cae193f7ac6f4b301dd7e64af91b76d32e5 Mon Sep 17 00:00:00 2001
From: John Gee <john@ruru.gen.nz>
Date: Mon, 15 Jun 2020 14:13:22 +1200
Subject: [PATCH] Throw for likely option name problems (#1275)

* Make a start on warning for option name clashes with Command properties

* Use correct property name for lookup

* Shift clash detection into routine, still WIP

* Prevent false positive clashes when negated option

* Add tests for option name  clashes

* Refine advice
---
 index.js                          | 45 ++++++++++++++++++++++++++
 tests/options.detectClash.test.js | 52 +++++++++++++++++++++++++++++++
 2 files changed, 97 insertions(+)
 create mode 100644 tests/options.detectClash.test.js

diff --git a/index.js b/index.js
index 5846d42e7..e73254f7f 100644
--- a/index.js
+++ b/index.js
@@ -118,6 +118,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;
@@ -430,6 +431,47 @@ 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) {
+      // no clash
+      return;
+    }
+
+    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 '${option.attributeName()}' on Command
+- call storeOptionsAsProperties(false) to store option values safely,
+- or call storeOptionsAsProperties(true) to suppress this check,
+- or change option name`);
+    }
+  };
+
   /**
    * Internal implementation shared by .option() and .requiredOption()
    *
@@ -448,6 +490,8 @@ class Command extends EventEmitter {
     const name = option.attributeName();
     option.mandatory = !!config.mandatory;
 
+    this._checkForOptionNameClash(option);
+
     // default as 3rd arg
     if (typeof fn !== 'function') {
       if (fn instanceof RegExp) {
@@ -612,6 +656,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');
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 <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 <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 <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();
+});