Fixed 'extends' option to work. (#3916)

[liloyek]

Hi, I fixed this issue by applying default values at yargs(), not yargs-parser.detailed().

• To be exact, applying 'extend' option is a feature of yargs().
• yargs-parser just parses literally options of cli.
• Therefore, extended configuration is overrided in yargs-parser.detailed() by default values even if there is a extends option.
• So default values should be concatenated in yargs() so that extended configuration could be applied.

Description of the Change

Before

Default values are applied by yargs-parser.detailed in options.js.

// lib/cli/options.js

...
const result = yargsParser.detailed(args, {
    configuration,
    configObjects,
    default: defaultValues, // here
    coerce: coerceOpts,
    narg: nargOpts,
    alias: aliases,
    string: types.string,
    array: types.array,
    number: types.number,
    boolean: types.boolean.concat(nodeArgs.map(pair => pair[0]))
});
...

// lib/cli/cli.js

...
yargs()
    ...
    .parserConfiguration(YARGS_PARSER_CONFIG)
    .config(args)
    .parse(args._);
};
...

After

// lib/cli/options.js

...
const result = yargsParser.detailed(args, {
  configuration,
  configObjects,
  coerce: coerceOpts,
  narg: nargOpts,
  alias: aliases,
  string: types.string,
  array: types.array,
  number: types.number,
  boolean: types.boolean.concat(nodeArgs.map(pair => pair[0]))
});
...

// lib/cli/cli.js

...
yargs()
    ...
    .parserConfiguration(YARGS_PARSER_CONFIG)
    .config(args)
    .default(mocharc) // here
    .parse(args._);
};
...

Etc

• Added a test code at /test/integration/options/extends.spec.js.
• Test code of 'extension' is deleted because now default values are not all applied in loadOptions() and there is a integration test for testing extension option including deleted test.

Applicable issues

fixed #3916

[jsf-clabot]

All committers have signed the CLA.

[coveralls]

Coverage increased (+0.07%) to 92.764% when pulling e4b90c9 on liloyek:liloyek into 3633a90 on mochajs:master.

[juergba]

loadOptions() is a public function. You can't eliminate one important part of the option parsing which are command line - configuration files - package.json - mocha.opts - default values.

yargs-parser just parses literally options of cli.

Above statement is incorrect.

[liloyek]

@juergba Thank you for review.

I chose to add default builder as above for yargs() because I thought it is the most simplest way to solve this problem.

So you mean I must not change the structure of parsing cli options and should keep the location of applying default, right?

Then is it ok to add a extra logic for processing '--extends' options in loadOptions() instead?

Thank you for your advice in advance.

[juergba]

I already commented this issue here. It's certainly more complex than a good-first-issue and not suited for a quick-and-dirty merge.

Then is it ok to add a extra logic for processing '--extends' options in loadOptions() instead?

Yes. I don't see any other solution at the moment.

Our configuration files are probably the ones which inherit by using --extend. Check here wether opts.config can be of any help. Which priority does it have? Before opts.configObjects or after? You really have to study and understand our parsing and its priorities. And yargs-parser and its features.

[liloyek]

Ok, I should've understood more about features of yargs and yargs-parser.
I'll try to change the code as the way above.
Thank you for comments.

[boneskull]

@liloyek Do you intend to continue with this PR?

[liloyek]

@boneskull I still want to continue, but I think it's better to close this PR and make new one after I finish it. Is it ok?