Merge pull request #47 from hpaulj/conflictHandler

Conflict handler with 'resolve', issue #46

lib/action_container.js

@@ -65,6 +65,9 @@ var ActionContainer = module.exports = function ActionContainer(options) {
   this.register('action', 'version', ActionVersion);
   this.register('action', 'parsers', ActionSubparsers);
 
+  // raise an exception if the conflict handler is invalid
+  this._getHandler();
+
   // action storage
   this._actions = [];
   this._optionStringActions = {};
@@ -278,7 +281,7 @@ ActionContainer.prototype._addAction = function (action) {
 ActionContainer.prototype._removeAction = function (action) {
   var actionIndex = this._actions.indexOf(action);
   if (actionIndex >= 0) {
-    this._actions.splice(actionIndex);
+    this._actions.splice(actionIndex, 1);
   }
 };
 
@@ -390,7 +393,7 @@ ActionContainer.prototype._getOptional = function (args, options) {
   // infer dest, '--foo-bar' -> 'foo_bar' and '-x' -> 'x'
   var dest = options.dest || null;
   delete options.dest;
-  
+
   if (!dest) {
     var optionStringDest = optionStringsLong.length ? optionStringsLong[0] :optionStrings[0];
     dest = _.str.strip(optionStringDest, this.prefixChars);
@@ -421,30 +424,64 @@ ActionContainer.prototype._popActionClass = function (options, defaultValue) {
   return actionClass;
 };
 
+ActionContainer.prototype._getHandler = function () {
+  var handlerString = this.conflictHandler;
+  var handlerFuncName = "_handleConflict" + _.str.capitalize(handlerString);
+  var func = this[handlerFuncName];
+  if (typeof func === 'undefined') {
+    var msg = "invalid conflict resolution value: " + handlerString;
+    throw new Error(msg);
+  } else {
+    return func;
+  }
+};
+
 ActionContainer.prototype._checkConflict = function (action) {
-  var conflictHandler = this._container.conflictHandler;
   var optionStringActions = this._optionStringActions;
   var conflictOptionals = [];
 
   // find all options that conflict with this option
+  // collect pairs, the string, and an existing action that it conflicts with
   action.optionStrings.forEach(function (optionString) {
-    if (!!optionStringActions[optionString]) {
-      conflictOptionals.push(optionString);
+    var conflOptional = optionStringActions[optionString];
+    if (typeof conflOptional !== 'undefined') {
+      conflictOptionals.push([optionString, conflOptional]);
     }
   });
 
   if (conflictOptionals.length > 0) {
+    var conflictHandler = this._getHandler();
+    conflictHandler.call(this, action, conflictOptionals);
+  }
+};
 
-    if (conflictHandler === 'resolve') {
-      this._removeAction(optionStringActions['--glop']);
-      return;
-    }
+ActionContainer.prototype._handleConflictError = function (action, conflOptionals) {
+  var conflicts = _.map(conflOptionals, function (pair) {return pair[0]; });
+  conflicts = conflicts.join(', ');
+  throw argumentErrorHelper(
+    action,
+    _.str.sprintf('Conflicting option string(s): %(conflict)s', {
+      conflict: conflicts
+    })
+  );
+};
 
-    throw argumentErrorHelper(
-      action,
-      _.str.sprintf('Conflicting option string(s): %(conflict)s', {
-        conflict: conflictOptionals.join(', ')
-      })
-    );
-  }
+ActionContainer.prototype._handleConflictResolve = function (action, conflOptionals) {
+  // remove all conflicting options
+  var self = this;
+  conflOptionals.forEach(function (pair) {
+    var optionString = pair[0];
+    var conflictingAction = pair[1];
+    // remove the conflicting option string
+    var i = conflictingAction.optionStrings.indexOf(optionString);
+    if (i >= 0) {
+      conflictingAction.optionStrings.splice(i, 1);
+    }
+    delete self._optionStringActions[optionString];
+    // if the option now has no option string, remove it from the
+    // container holding it
+    if (conflictingAction.optionStrings.length === 0) {
+      conflictingAction.container._removeAction(conflictingAction);
+    }
+  });
 };

lib/argument/group.js

@@ -34,6 +34,7 @@ var ArgumentGroup = module.exports = function ArgumentGroup(container, options)
   options = options || {};
 
   // add any missing keyword arguments by checking the container
+  options.conflictHandler = (options.conflictHandler || container.conflictHandler);
   options.prefixChars = (options.prefixChars || container.prefixChars);
   options.argumentDefault = (options.argumentDefault || container.argumentDefault);
 
@@ -66,6 +67,9 @@ ArgumentGroup.prototype._addAction = function (action) {
 ArgumentGroup.prototype._removeAction = function (action) {
   // Parent remove action
   ActionContainer.prototype._removeAction.call(this, action);
-  this._groupActions.splice(action);
+  var actionIndex = this._groupActions.indexOf(action);
+  if (actionIndex >= 0) {
+    this._groupActions.splice(actionIndex, 1);
+  }
 };
 

lib/argument_parser.js

@@ -54,23 +54,23 @@ var Namespace = require('./namespace');
 var ArgumentParser = module.exports = function ArgumentParser(options) {
   var self = this;
   options = options || {};
+
+  options.description = (options.description || null);
+  options.argumentDefault = (options.argumentDefault || null);
   options.prefixChars = (options.prefixChars || '-');
+  options.conflictHandler = (options.conflictHandler || 'error');
+  ActionContainer.call(this, options);
+
   options.addHelp = (options.addHelp === undefined || !!options.addHelp);
   options.parents = (options.parents || []);
-
-  options.argumentDefault = options.argumentDefault || null;
   // default program name
   options.prog = (options.prog || Path.basename(process.argv[1]));
-
-  ActionContainer.call(this, options);
-
   this.prog = options.prog;
   this.usage = options.usage;
   this.epilog = options.epilog;
   this.version = options.version;
 
   this.debug = (options.debug === true);
-  this.conflictHandler = options.conflictHandler;
 
   this.formatterClass = (options.formatterClass || HelpFormatter);
   this.fromfilePrefixChars = options.fromfilePrefixChars || null;
@@ -283,7 +283,7 @@ ArgumentParser.prototype.parseKnownArgs = function (args, namespace) {
   // parse the arguments and exit if there are any errors
   try {
     var res = this._parseKnownArgs(args, namespace);
-    
+
     namespace = res[0];
     args = res[1];
     if (_.has(namespace, $$._UNRECOGNIZED_ARGS_ATTR)) {
@@ -330,7 +330,7 @@ ArgumentParser.prototype._parseKnownArgs = function (argStrings, namespace) {
       conflicts.push.apply(conflicts, groupActions.slice(i + 1));
     });
   });
-  
+
   // find all option indices, and determine the arg_string_pattern
   // which has an 'O' if there is an option at an index,
   // an 'A' if there is an argument, or a '-' if there is a '--'
@@ -455,7 +455,7 @@ ArgumentParser.prototype._parseKnownArgs = function (argStrings, namespace) {
       // optional's string arguments with the following strings
       // if successful, exit the loop
       else {
-  
+
         start = startIndex + 1;
         var selectedPatterns = argStringsPattern.substr(start);
 
@@ -464,7 +464,7 @@ ArgumentParser.prototype._parseKnownArgs = function (argStrings, namespace) {
 
 
         args = argStrings.slice(start, stop);
-    
+
         actionTuples.push([action, args, optionString]);
         break;
       }
@@ -504,7 +504,7 @@ ArgumentParser.prototype._parseKnownArgs = function (argStrings, namespace) {
         startIndex += argCount;
         takeAction(action, args);
       });
-      
+
     // slice off the Positionals that we just parsed and return the
     // index at which the Positionals' string args stopped
     positionals = positionals.slice(argCounts.length);
@@ -552,7 +552,7 @@ ArgumentParser.prototype._parseKnownArgs = function (argStrings, namespace) {
         startIndex = positionalsEndIndex;
       }
     }
-    
+
     // if we consumed all the positionals we could and we're not
     // at the index of an option string, there were extra arguments
     if (!optionStringIndices[startIndex]) {
@@ -592,7 +592,7 @@ ArgumentParser.prototype._parseKnownArgs = function (argStrings, namespace) {
       actionUsed = _.any(group._groupActions, function (action) {
         return _.contains(seenNonDefaultActions, action);
       });
-      
+
       // if no actions were used, report the error
       if (!actionUsed) {
         var names = [];
@@ -687,7 +687,7 @@ ArgumentParser.prototype._matchArgumentsPartial = function (actions, regexpArgSt
   var result = [];
   var actionSlice, pattern, matches;
   var i, j;
-   
+
   var getLength = function (string) {
     return string.length;
   };
@@ -907,7 +907,7 @@ ArgumentParser.prototype._getValues = function (action, argStrings) {
 
   // optional argument produces a default when not present
   if (argStrings.length === 0 && action.nargs === $$.OPTIONAL) {
-    
+
     value = (action.isOptional()) ? action.constant: action.defaultValue;
 
     if (typeof(value) === 'string') {
@@ -1009,7 +1009,7 @@ ArgumentParser.prototype._checkValue = function (action, value) {
     if (_.isObject(choices) && !_.isArray(choices) && choices[value]) {
       return;
     }
-  
+
     if (_.isString(choices)) {
       choices = choices.split('').join(', ');
     }

test/base.js

@@ -115,17 +115,6 @@ describe('base', function () {
     );
   });
 
-  it("should overwrite arguments when given the 'resolve' conflictHandler", function () {
-    parser = new ArgumentParser({conflictHandler: 'resolve'});
-
-    parser.addArgument(['--foo'], {help: 'old foo'});
-    parser.addArgument(['--foo'], {help: 'new foo'});
-
-    var help = parser._optionStringActions['--foo'].help;
-
-    assert.equal(help, 'new foo');
-  });
-
   it("should parse negative arguments", function () {
     parser = new ArgumentParser({debug: true});
     parser.addArgument(['-f', '--foo']);

test/conflict.js

@@ -0,0 +1,97 @@
+/*global describe, it*/
+
+'use strict';
+
+var assert = require('assert');
+
+var ArgumentParser = require('../lib/argparse').ArgumentParser;
+
+describe('Argument conflict handling', function () {
+  var parser;
+  var help;
+
+  it("test_bad_type", function () {
+    assert.throws(function () {
+      parser = new ArgumentParser({conflictHandler: 'foo'});
+    },
+    /invalid conflict resolution value: foo/i
+    );
+  });
+  it("test_conflict_error", function () {
+    parser = new ArgumentParser();
+    parser.addArgument(['-x']);
+    assert.throws(function () {
+      parser.addArgument(['-x']);
+    },
+    /Conflicting option string/i
+    );
+    parser.addArgument(['--spam']);
+    assert.throws(function () {
+      parser.addArgument(['--spam']);
+    },
+    /Conflicting option string/i
+    );
+  });
+  it("test_resolve_error", function () {
+    parser = new ArgumentParser({prog: 'PROG', conflictHandler: 'resolve'});
+    parser.addArgument(['-x'], {help: 'OLD X'});
+    parser.addArgument(['-x'], {help: 'NEW X'});
+    help = parser.formatHelp();
+    /* expect
+    usage: PROG [-h] [-x X]
+
+    optional arguments:
+      -h, --help  show this help message and exit
+      -x X        NEW X
+    */
+    assert(help.match(/usage: PROG \[-h\] \[-x X\]/im));
+    assert(help.match(/Show this help message and exit/im));
+    assert(help.match(/-x X\s*NEW X/im));
+    parser.addArgument(['--spam'], {metavar: 'OLD_SPAM'});
+    parser.addArgument(['--spam'], {metavar: 'NEW_SPAM'});
+    help = parser.formatHelp();
+    /* expect
+    usage: PROG [-h] [-x X] [--spam NEW_SPAM]
+
+    optional arguments:
+      -h, --help       show this help message and exit
+      -x X             NEW X
+      --spam NEW_SPAM
+    */
+    assert(help.match(/--spam NEW_SPAM/im));
+  });
+  it("TypeError with multiple conflicts", function () {
+    parser = new ArgumentParser({debug: true});
+    parser.addArgument(['-f', '--foo']);
+    parser.addArgument(['-b', '--bar']);
+    assert.throws(
+      function () { parser.addArgument(['--foo', '--bar', '--foobar']); },
+      /argument "--foo\/--bar\/--foobar": Conflicting option string\(s\): --foo, --bar/
+    );
+  });
+  it("resolving multiple conflicts", function () {
+    parser = new ArgumentParser({debug: true, conflictHandler: 'resolve'});
+    parser.addArgument(['-f', '--foo']);
+    parser.addArgument(['--bar']);
+    help = parser.formatHelp();
+    /*
+    usage: _mocha [-h] [-f FOO] [--bar BAR]
+    Optional arguments:
+      -h, --help         Show this help message and exit.
+      -f FOO, --foo FOO
+      --bar BAR
+    */
+    assert(help.match(/-f FOO, --foo FOO/im));
+    parser.addArgument(['--foo', '--bar', '--foobar']);
+    help = parser.formatHelp();
+    assert(help.match(/-f FOO$/im));
+    assert(help.match(/--foo FOO, --bar FOO, --foobar FOO/im));
+    /*
+    usage: _mocha [-h] [-f FOO] [--foo FOO]
+    Optional arguments:
+      -h, --help            Show this help message and exit.
+      -f FOO
+      --foo FOO, --bar FOO, --foobar FOO
+    */
+  });
+});