From b51625ad684aef691013c2bbd9f19b34a7406388 Mon Sep 17 00:00:00 2001
From: Tobias Lidskog <tobias@lidskog.se>
Date: Mon, 16 May 2016 02:55:37 +0200
Subject: [PATCH] Fix cli args merging.

Cleanup solution for:
- args in config files should trigger plugin loading
- alias shortcuts (e.g. -b) should trigger plugin loading
- consistent handling of canonical vs. alias form of cli options
- documentation for verbose flag
---
 bin/sitespeed.js   |  2 +-
 lib/support/cli.js | 80 ++++++++++++++++++++++++++++------------------
 2 files changed, 50 insertions(+), 32 deletions(-)

diff --git a/bin/sitespeed.js b/bin/sitespeed.js
index dd8c5587b..d245fd195 100755
--- a/bin/sitespeed.js
+++ b/bin/sitespeed.js
@@ -35,7 +35,7 @@ if (log.isEnabledFor(log.CRITICAL)) { // TODO change the threshold to VERBOSE be
 
 log.info('Versions OS: %s sitespeed.io: %s browsertime: %s coach: %s', os.platform() + ' ' + os.release(), packageInfo.version, packageInfo.dependencies.browsertime, packageInfo.dependencies.webcoach);
 
-loader.parsePluginNames(parsed.raw)
+loader.parsePluginNames(parsed.explicitOptions)
   .then((pluginNames) => {
     if (allInArray(['browsertime', 'coach'], pluginNames)) {
       parsed.options.browsertime = merge({}, parsed.options.browsertime, {coach: true});
diff --git a/lib/support/cli.js b/lib/support/cli.js
index d4418262f..98eeeea67 100644
--- a/lib/support/cli.js
+++ b/lib/support/cli.js
@@ -1,32 +1,40 @@
 'use strict';
 
 let yargs = require('yargs'),
+  path = require('path'),
+  packageInfo = require('../../package'),
   merge = require('lodash.merge'),
-  packageInfo = require('../../package');
+  reduce = require('lodash.reduce'),
+  set = require('lodash.set');
 
 module.exports.parseCommandLine = function parseCommandLine() {
-  let argv = yargs
+  let parsed = yargs
     .usage('$0 [options] <url>/<file>')
     .require(1, 'urlOrFile')
     .version(() => `${packageInfo.name} ${packageInfo.version}`)
-    .count('verbose')
     .option('debug', {
       default: false,
       describe: 'Debug mode logs all internal messages to the console.',
       type: 'boolean'
     })
+    .option('verbose', {
+      alias: 'v',
+      describe: 'Verbose mode prints progress messages to the console. Enter up to three times (-vvv)' +
+      ' to increase the level of detail.',
+      type: 'count'
+    })
     /*
      Browsertime cli options
      */
-    .option('b', {
-      alias: 'browsertime.browser',
+    .option('browsertime.browser', {
+      alias: 'b',
       default: 'firefox',
       describe: 'Choose which Browser to use when you test [firefox|chrome]',
       choices: ['chrome', 'firefox'],
       group: 'Browser'
     })
-    .option('n', {
-      alias: 'browsertime.iterations',
+    .option('browsertime.iterations', {
+      alias: 'n',
       default: 3,
       describe: 'How many times you want to test each page',
       group: 'Browser'
@@ -74,23 +82,23 @@ module.exports.parseCommandLine = function parseCommandLine() {
     /*
      Crawler options
      */
-    .option('d', {
-      alias: 'crawler.depth',
+    .option('crawler.depth', {
+      alias: 'd',
       describe: 'How deep to crawl (1=only one page, 2=include links from first page, etc.)',
       group: 'Crawler'
     })
-    .option('m', {
-      alias: 'crawler.maxPages',
+    .option('crawler.maxPages', {
+      alias: 'm',
       describe: 'The max number of pages to test. Default is no limit.',
       group: 'Crawler'
     })
-    .option('s', {
-      alias: 'crawler.skip',
+    .option('crawler.skip', {
+      alias: 's',
       describe: 'Do not crawl pages that contains this value in the URL path. NOT YET IMPLEMENTED',
       group: 'Crawler'
     })
-    .option('c', {
-      alias: 'crawler.containInPath',
+    .option('crawler.containInPath', {
+      alias: 'c',
       describe: 'Only crawl URLs that contains this value in the URL path. NOT YET IMPLEMENTED',
       group: 'Crawler'
     })
@@ -237,32 +245,42 @@ module.exports.parseCommandLine = function parseCommandLine() {
       describe: 'The folder name where the result will be stored. By default the name is generated using current_date/domain/filename'
     })
     .help('h')
-    .alias('h', 'help')
+    .alias('help', 'h')
     .config('config')
-    .alias('v', 'verbose')
-    .alias('V', 'version')
+    .alias('version', 'V')
     //     .describe('browser', 'Specify browser')
     .wrap(yargs.terminalWidth())
     //  .check(validateInput)
-    .epilog('Read the docs at https://www.sitespeed.io')
-    .argv;
+    .epilog('Read the docs at https://www.sitespeed.io');
 
-    // TODO: Remove this hack when yargs supports merging nested objects when
-    // both a json file is passed and the key is passed via an option
-    let rawArgv = yargs.reset().argv;
-    if(argv.webpagetest && argv.config) {
-      rawArgv = merge({}, {"webpagetest": argv.webpagetest}, rawArgv);
-    }
-    // End hack
+  const aliases = parsed.getOptions().alias,
+    argv = parsed.argv;
 
-    // FIXME: Find a solution that would work for all aliasing
-    if(argv.d) {
-      rawArgv = merge({}, {"crawler": {"depth": argv.d}}, rawArgv);
+  // aliases are long options -> short option
+  const aliasLookup = reduce(aliases, (lookup, value, key) => {
+    lookup.set(value[0], key);
+    return lookup;
+  }, new Map());
+
+  let explicitOptions = yargs.reset().argv;
+
+  explicitOptions = reduce(explicitOptions, (result, value, key) => {
+    if (aliasLookup.has(key)) {
+      const fullKey = aliasLookup.get(key);
+      result = set(result, fullKey, value);
     }
+    result = set(result, key, value);
+    return result;
+  }, {});
+
+  if (argv.config) {
+    const config = require(path.resolve(process.cwd(), argv.config));
+    explicitOptions = merge(explicitOptions, config);
+  }
 
   return {
     url: argv._[0],
     options: argv,
-    raw: rawArgv
+    explicitOptions: explicitOptions
   };
 };