Throw for unsupported option flag setup (#2270)

Author: shadowspawn

lib/option.js

@@ -312,17 +312,32 @@ function camelcase(str) {
 function splitOptionFlags(flags) {
   let shortFlag;
   let longFlag;
-  // Use original very loose parsing to maintain backwards compatibility for now,
-  // which allowed for example unintended `-sw, --short-word` [sic].
-  const flagParts = flags.split(/[ |,]+/);
-  if (flagParts.length > 1 && !/^[[<]/.test(flagParts[1]))
-    shortFlag = flagParts.shift();
-  longFlag = flagParts.shift();
-  // Add support for lone short flag without significantly changing parsing!
-  if (!shortFlag && /^-[^-]$/.test(longFlag)) {
-    shortFlag = longFlag;
-    longFlag = undefined;
-  }
+  // short flag, single dash and single character
+  const shortFlagExp = /^-[^-]$/;
+  // long flag, double dash and at least one character
+  const longFlagExp = /^--[^-]/;
+
+  const flagParts = flags.split(/[ |,]+/).concat('guard');
+  if (shortFlagExp.test(flagParts[0])) shortFlag = flagParts.shift();
+  if (longFlagExp.test(flagParts[0])) longFlag = flagParts.shift();
+
+  // Check for some unsupported flags that people try.
+  if (/^-[^-][^-]/.test(flagParts[0]))
+    throw new Error(
+      `invalid Option flags, short option is dash and single character: '${flags}'`,
+    );
+  if (shortFlag && shortFlagExp.test(flagParts[0]))
+    throw new Error(
+      `invalid Option flags, more than one short flag: '${flags}'`,
+    );
+  if (longFlag && longFlagExp.test(flagParts[0]))
+    throw new Error(
+      `invalid Option flags, more than one long flag: '${flags}'`,
+    );
+  // Generic error if failed to find a flag or an unexpected flag left over.
+  if (!(shortFlag || longFlag) || flagParts[0].startsWith('-'))
+    throw new Error(`invalid Option flags: '${flags}'`);
+
   return { shortFlag, longFlag };
 }
 

tests/option.bad-flags.test.js

@@ -0,0 +1,34 @@
+const { Option } = require('../');
+
+// Check that unsupported flags throw.
+test.each([
+  { flags: '-a, -b' }, // too many short flags
+  { flags: '-a, -b <value>' },
+  { flags: '-a, -b, --long' },
+  { flags: '--one, --two' }, // too many long flags
+  { flags: '--one, --two [value]' },
+  { flags: '-ws' }, // short flag with more than one character
+  { flags: 'sdkjhskjh' }, // oops, no flags
+  { flags: '-a,-b' }, // try all the separators
+  { flags: '-a|-b' },
+  { flags: '-a -b' },
+])('when construct Option with flags %p then throw', ({ flags }) => {
+  expect(() => {
+    new Option(flags);
+  }).toThrow(/^invalid Option flags/);
+});
+
+// Check that supported flags do not throw.
+test.each([
+  { flags: '-s' }, // single short
+  { flags: '--long' }, // single long
+  { flags: '-b, --both' }, // short and long
+  { flags: '-b,--both <comma>' },
+  { flags: '-b|--both <bar>' },
+  { flags: '-b --both [space]' },
+  { flags: '-v, --variadic <files...>' },
+])('when construct Option with flags %p then do not throw', ({ flags }) => {
+  expect(() => {
+    new Option(flags);
+  }).not.toThrow();
+});

tests/options.registerClash.test.js

@@ -29,8 +29,8 @@ describe('.option()', () => {
   test('when reuse flags in subcommand then does not throw', () => {
     expect(() => {
       const program = new Command();
-      program.option('e, --example');
-      program.command('sub').option('e, --example');
+      program.option('-e, --example');
+      program.command('sub').option('-e, --example');
     }).not.toThrow();
   });
 });