From f7f5b28daeaff91abfb9e2f20b71e0e8bc38bef5 Mon Sep 17 00:00:00 2001
From: Hans <hans@hansl.ca>
Date: Wed, 19 Sep 2018 11:31:33 -0700
Subject: [PATCH] feat(@angular/cli): if parsing comes accross an obvious error
 throw it

We accumulate errors this way, and throw only once at the end, with messages for
all errors.
---
 .../angular/cli/commands/definitions.json     |  4 +-
 packages/angular/cli/models/command.ts        |  7 ---
 packages/angular/cli/models/parser.ts         | 37 +++++++++++++--
 packages/angular/cli/models/parser_spec.ts    | 46 ++++++++++++-------
 4 files changed, 65 insertions(+), 29 deletions(-)

diff --git a/packages/angular/cli/commands/definitions.json b/packages/angular/cli/commands/definitions.json
index 5314b93ca247..e292a75f9f97 100644
--- a/packages/angular/cli/commands/definitions.json
+++ b/packages/angular/cli/commands/definitions.json
@@ -30,8 +30,8 @@
       "type": "object",
       "properties": {
         "help": {
-          "type": ["boolean", "string"],
-          "description": "Shows a help message. Use '--help=json' as a value to output the help in JSON format.",
+          "enum": [true, false, "json", "JSON"],
+          "description": "Shows a help message. You can pass the format as a value.",
           "default": false
         }
       }
diff --git a/packages/angular/cli/models/command.ts b/packages/angular/cli/models/command.ts
index be077849e5ec..6234fb9dc596 100644
--- a/packages/angular/cli/models/command.ts
+++ b/packages/angular/cli/models/command.ts
@@ -153,13 +153,6 @@ export abstract class Command<T extends BaseCommandOptions = BaseCommandOptions>
       return this.printHelp(options);
     } else if (options.help === 'json' || options.help === 'JSON') {
       return this.printJsonHelp(options);
-    } else if (options.help !== false && options.help !== undefined) {
-      // The user entered an invalid type of help, maybe?
-      this.logger.fatal(
-        `--help was provided, but format ${JSON.stringify(options.help)} was not understood.`,
-      );
-
-      return 1;
     } else {
       return await this.run(options);
     }
diff --git a/packages/angular/cli/models/parser.ts b/packages/angular/cli/models/parser.ts
index a4a3b6b1abb0..baa5ec5f0bb4 100644
--- a/packages/angular/cli/models/parser.ts
+++ b/packages/angular/cli/models/parser.ts
@@ -6,10 +6,21 @@
  * found in the LICENSE file at https://angular.io/license
  *
  */
-import { strings } from '@angular-devkit/core';
+import { BaseException, strings } from '@angular-devkit/core';
 import { Arguments, Option, OptionType, Value } from './interface';
 
 
+export class ParseArgumentException extends BaseException {
+  constructor(
+    comments: string[],
+    public readonly parsed: Arguments,
+    public readonly ignored: string[],
+  ) {
+    super(`One or more errors occured while parsing arguments:\n  ${comments.join('\n  ')}`);
+  }
+}
+
+
 function _coerceType(str: string | undefined, type: OptionType, v?: Value): Value | undefined {
   switch (type) {
     case OptionType.Any:
@@ -103,6 +114,8 @@ function _assignOption(
   parsedOptions: Arguments,
   _positionals: string[],
   leftovers: string[],
+  ignored: string[],
+  errors: string[],
 ) {
   let key = arg.substr(2);
   let option: Option | null = null;
@@ -169,7 +182,15 @@ function _assignOption(
     if (v !== undefined) {
       parsedOptions[option.name] = v;
     } else {
-      leftovers.push(arg);
+      let error = `Argument ${key} could not be parsed using value ${JSON.stringify(value)}.`;
+      if (option.enum) {
+        error += ` Valid values are: ${option.enum.map(x => JSON.stringify(x)).join(', ')}.`;
+      } else {
+        error += `Valid type(s) is: ${(option.types || [option.type]).join(', ')}`;
+      }
+
+      errors.push(error);
+      ignored.push(arg);
     }
   }
 }
@@ -244,6 +265,9 @@ export function parseArguments(args: string[], options: Option[] | null): Argume
   const positionals: string[] = [];
   const parsedOptions: Arguments = {};
 
+  const ignored: string[] = [];
+  const errors: string[] = [];
+
   for (let arg = args.shift(); arg !== undefined; arg = args.shift()) {
     if (!arg) {
       break;
@@ -256,7 +280,7 @@ export function parseArguments(args: string[], options: Option[] | null): Argume
     }
 
     if (arg.startsWith('--')) {
-      _assignOption(arg, args, options, parsedOptions, positionals, leftovers);
+      _assignOption(arg, args, options, parsedOptions, positionals, leftovers, ignored, errors);
     } else if (arg.startsWith('-')) {
       // Argument is of form -abcdef.  Starts at 1 because we skip the `-`.
       for (let i = 1; i < arg.length; i++) {
@@ -264,7 +288,8 @@ export function parseArguments(args: string[], options: Option[] | null): Argume
         // Treat the last flag as `--a` (as if full flag but just one letter). We do this in
         // the loop because it saves us a check to see if the arg is just `-`.
         if (i == arg.length - 1) {
-          _assignOption('--' + flag, args, options, parsedOptions, positionals, leftovers);
+          const arg = '--' + flag;
+          _assignOption(arg, args, options, parsedOptions, positionals, leftovers, ignored, errors);
         } else {
           const maybeOption = _getOptionFromName(flag, options);
           if (maybeOption) {
@@ -318,5 +343,9 @@ export function parseArguments(args: string[], options: Option[] | null): Argume
     parsedOptions['--'] = [...positionals, ...leftovers];
   }
 
+  if (errors.length > 0) {
+    throw new ParseArgumentException(errors, parsedOptions, ignored);
+  }
+
   return parsedOptions;
 }
diff --git a/packages/angular/cli/models/parser_spec.ts b/packages/angular/cli/models/parser_spec.ts
index c5664da16599..88de630a2ddd 100644
--- a/packages/angular/cli/models/parser_spec.ts
+++ b/packages/angular/cli/models/parser_spec.ts
@@ -7,7 +7,7 @@
  *
  */
 import { Arguments, Option, OptionType } from './interface';
-import { parseArguments } from './parser';
+import { ParseArgumentException, parseArguments } from './parser';
 
 describe('parseArguments', () => {
   const options: Option[] = [
@@ -32,10 +32,11 @@ describe('parseArguments', () => {
       description: '' },
   ];
 
-  const tests: { [test: string]: Partial<Arguments> } = {
+  const tests: { [test: string]: Partial<Arguments> | ['!!!', Partial<Arguments>, string[]] } = {
     '--bool': { bool: true },
-    '--bool=1': { '--': ['--bool=1'] },
-    '--bool=yellow': { '--': ['--bool=yellow'] },
+    '--bool=1': ['!!!', {}, ['--bool=1']],
+    '-- --bool=1': { '--': ['--bool=1'] },
+    '--bool=yellow': ['!!!', {}, ['--bool=yellow']],
     '--bool=true': { bool: true },
     '--bool=false': { bool: false },
     '--no-bool': { bool: false },
@@ -45,7 +46,8 @@ describe('parseArguments', () => {
     '--b true': { bool: true },
     '--b false': { bool: false },
     '--bool --num': { bool: true, num: 0 },
-    '--bool --num=true': { bool: true, '--': ['--num=true'] },
+    '--bool --num=true': ['!!!', { bool: true }, ['--num=true']],
+    '-- --bool --num=true': { '--': ['--bool', '--num=true'] },
     '--bool=true --num': { bool: true, num: 0 },
     '--bool true --num': { bool: true, num: 0 },
     '--bool=false --num': { bool: false, num: 0 },
@@ -85,7 +87,7 @@ describe('parseArguments', () => {
     '--t2=true': { t2: true },
     '--t2': { t2: true },
     '--no-t2': { t2: false },
-    '--t2=yellow': { '--': ['--t2=yellow'] },
+    '--t2=yellow': ['!!!', {}, ['--t2=yellow']],
     '--no-t2=true': { '--': ['--no-t2=true'] },
     '--t2=123': { t2: 123 },
     '--t3=a': { t3: 'a' },
@@ -93,22 +95,22 @@ describe('parseArguments', () => {
     '--t3 true': { t3: true },
     '--e1 hello': { e1: 'hello' },
     '--e1=hello': { e1: 'hello' },
-    '--e1 yellow': { p1: 'yellow', '--': ['--e1'] },
-    '--e1=yellow': { '--': ['--e1=yellow'] },
-    '--e1': { '--': ['--e1'] },
-    '--e1 true': { p1: 'true', '--': ['--e1'] },
-    '--e1=true': { '--': ['--e1=true'] },
+    '--e1 yellow': ['!!!', { p1: 'yellow' }, ['--e1']],
+    '--e1=yellow': ['!!!', {}, ['--e1=yellow']],
+    '--e1': ['!!!', {}, ['--e1']],
+    '--e1 true': ['!!!', { p1: 'true' }, ['--e1']],
+    '--e1=true': ['!!!', {},  ['--e1=true']],
     '--e2 hello': { e2: 'hello' },
     '--e2=hello': { e2: 'hello' },
     '--e2 yellow': { p1: 'yellow', e2: '' },
-    '--e2=yellow': { '--': ['--e2=yellow'] },
+    '--e2=yellow': ['!!!', {}, ['--e2=yellow']],
     '--e2': { e2: '' },
     '--e2 true': { p1: 'true', e2: '' },
-    '--e2=true': { '--': ['--e2=true'] },
+    '--e2=true': ['!!!', {}, ['--e2=true']],
     '--e3 json': { e3: 'json' },
     '--e3=json': { e3: 'json' },
     '--e3 yellow': { p1: 'yellow', e3: true },
-    '--e3=yellow': { '--': ['--e3=yellow'] },
+    '--e3=yellow': ['!!!', {}, ['--e3=yellow']],
     '--e3': { e3: true },
     '--e3 true': { e3: true },
     '--e3=true': { e3: true },
@@ -116,9 +118,21 @@ describe('parseArguments', () => {
 
   Object.entries(tests).forEach(([str, expected]) => {
     it(`works for ${str}`, () => {
-      const actual = parseArguments(str.split(/\s+/), options);
+      try {
+        const actual = parseArguments(str.split(/\s+/), options);
 
-      expect(actual).toEqual(expected as Arguments);
+        expect(Array.isArray(expected)).toBe(false);
+        expect(actual).toEqual(expected as Arguments);
+      } catch (e) {
+        if (!(e instanceof ParseArgumentException)) {
+          throw e;
+        }
+
+        // The expected values are an array.
+        expect(Array.isArray(expected)).toBe(true);
+        expect(e.parsed).toEqual(expected[1] as Arguments);
+        expect(e.ignored).toEqual(expected[2] as string[]);
+      }
     });
   });
 });