fix(@angular/cli): numerical flags should not give 0 if empty

And numerical positional flags will be ignored. If the value is an empty string, a number conversion would give 0. It is unexpected from the user standpoint ("--num=" has the user expect a string value).
angular · Sep 27, 2018 · 5faf0cb · 5faf0cb
1 parent 4daa299
commit 5faf0cb
Show file tree

Hide file tree

Showing 2 changed files with 17 additions and 5 deletions.
diff --git a/packages/angular/cli/models/parser.ts b/packages/angular/cli/models/parser.ts
@@ -54,6 +54,8 @@ function _coerceType(str: string | undefined, type: OptionType, v?: Value): Valu
     case OptionType.Number:
       if (str === undefined) {
         return 0;
+      } else if (str === '') {
+        return undefined;
       } else if (Number.isFinite(+str)) {
         return +str;
       } else {
@@ -308,6 +310,8 @@ export function parseArguments(args: string[], options: Option[] | null): Argume
   }
 
   // Deal with positionals.
+  // TODO(hansl): this is by far the most complex piece of code in this file. Try to refactor it
+  //   simpler.
   if (positionals.length > 0) {
     let pos = 0;
     for (let i = 0; i < positionals.length;) {
@@ -317,10 +321,11 @@ export function parseArguments(args: string[], options: Option[] | null): Argume
 
       // We do this with a found flag because more than 1 option could have the same positional.
       for (const option of options) {
-        // If any option has this positional and no value, we need to remove it.
+        // If any option has this positional and no value, AND fit the type, we need to remove it.
         if (option.positional === pos) {
-          if (parsedOptions[option.name] === undefined) {
-            parsedOptions[option.name] = positionals[i];
+          const coercedValue = _coerce(positionals[i], option, parsedOptions[option.name]);
+          if (parsedOptions[option.name] === undefined && coercedValue !== undefined) {
+            parsedOptions[option.name] = coercedValue;
             found = true;
           } else {
             incrementI = false;

diff --git a/packages/angular/cli/models/parser_spec.ts b/packages/angular/cli/models/parser_spec.ts
@@ -19,6 +19,7 @@ describe('parseArguments', () => {
     { name: 'arr', aliases: [ 'a' ], type: OptionType.Array, description: '' },
     { name: 'p1', positional: 0, aliases: [], type: OptionType.String, description: '' },
     { name: 'p2', positional: 1, aliases: [], type: OptionType.String, description: '' },
+    { name: 'p3', positional: 2, aliases: [], type: OptionType.Number, description: '' },
     { name: 't1', aliases: [], type: OptionType.Boolean,
       types: [OptionType.Boolean, OptionType.String], description: '' },
     { name: 't2', aliases: [], type: OptionType.Boolean,
@@ -63,8 +64,13 @@ describe('parseArguments', () => {
     'val1 --num=1 val2': { num: 1, p1: 'val1', p2: 'val2' },
     '--p1=val1 --num=1 val2': { num: 1, p1: 'val1', p2: 'val2' },
     '--p1=val1 --num=1 --p2=val2 val3': { num: 1, p1: 'val1', p2: 'val2', '--': ['val3'] },
-    '--bool val1 --etc --num val2 --v': { bool: true, num: 0, p1: 'val1', p2: 'val2',
-                                          '--': ['--etc', '--v'] },
+    '--bool val1 --etc --num val2 --v': [
+      '!!!',
+      { bool: true, p1: 'val1', p2: 'val2', '--': ['--etc', '--v'] },
+      ['--num' ],
+    ],
+    '--bool val1 --etc --num=1 val2 --v': { bool: true, num: 1, p1: 'val1', p2: 'val2',
+                                            '--': ['--etc', '--v'] },
     '--arr=a --arr=b --arr c d': { arr: ['a', 'b', 'c'], p1: 'd' },
     '--arr=1 --arr --arr c d': { arr: ['1', '', 'c'], p1: 'd' },
     '--arr=1 --arr --arr c d e': { arr: ['1', '', 'c'], p1: 'd', p2: 'e' },
@@ -121,6 +127,7 @@ describe('parseArguments', () => {
     '--e3': { e3: true },
     '--e3 true': { e3: true },
     '--e3=true': { e3: true },
+    'a b c 1': { p1: 'a', p2: 'b', '--': ['c', '1'] },
   };
 
   Object.entries(tests).forEach(([str, expected]) => {