feat!: populate error if incompatible narg/count or array/count options are used

[juergba]

Array:

var args = parse('-f foo baz -f', {
    array: ['f'],
    count: ['f'],
    configuration: {
      'duplicate-arguments-array': true,
      'flatten-duplicate-arrays': false
    }
})       //  { _: [], f: [ [ [Function: increment], [Function: increment] ], [] ] }

The result is incorrect, it should be: { _: [ 'foo', 'baz' ], f: 2}

Narg:

var args = parse('-f foo baz -f', {
  narg: {'f': 1},
  count: ['f']
})     // { _: [ 'baz' ], f: 1 }

The result is incorrect, it should be: { _: [ 'foo', 'baz' ], f: 2 }

A configuration opts.count plus opts.array or opts.narg does not make sense. Nevertheless this combination can be set by the user.

• we delete opts.array / opts.narg settings before parsing
• we populate error with an error object (but don't throw one)

[bcoe]

I don't like that we would silently correct strange behavior like this.

I would rather that we potentially add this as a validation check in yargs itself, and raise an exception if incompatible flags are set.

[juergba]

A validation check in yargs itself does not help if yargs-parser is used independently.

I see two options:

• we fix incompatible flags before parsing (by deleting them). We wouldn't have to change the parsing logic this way.
• we add additional checks to the parsing logic (in another PR you are trying to avoid this)

IMO the validation of the user input - in this case the configuration settings - should be done before the actual parsing. Please let me know how to proceed.

There will be more incompatible combinations the more configuration options we have, eg. collect-unknown-options and halt-at-non-option.

[bcoe]

@juergba we use the error object elsewhere to provide information about why a parse failed:

https://github.com/yargs/yargs-parser/blob/master/index.js#L365

If a user provides configuration that's contradictory, e.g., indicating that something is an array and a counter, I think it might be worthwhile to get in the habit of populating an error object.

We could do this early on in the parse process, as a way of indicating to the user that the parse may have been invalid; I think this would be better than making an effort to correct incompatible config.

[juergba]

Ok, understood. I will have a look at the error object.
I set back this PR to a lower priority, the fix of this wouldn't be a breaking change anyway.

[bcoe]

@juergba the other option on my mind, if we did want to make explicit decisions like saying that count takes precedence over array, would be that we eventually write a former grammar for our CLI parser, along these lines ... it would be a lot of work but could be good for the community.

tldr; I think in some cases populating an error is a good call (if the combination of config is silly), but I would love to make an effort to formally define what is "correct" behavior over time. I think this could align with some of @boneskull's interests (he'd like to define a MVP command line parser for Node.js).

[bcoe]

@juergba where did you land on this one, still something you'd like to see over the finish line? what do you think of populating an error?

[juergba]

Populating an error is a good option. I can imagine many silly combinations of configuration settings, which can be handled that way adding case by case.
I haven't set a time line for this one. You can take over if you need it in your next release. Otherwise I will implement it later when I have more time.

[bcoe]

@juergba sounds good, as long as we feel we have an approach hammered out, no rush.

[juergba]

@bcoe I applied some changes, please have a look and tell me wether this is the way to go.

• error contains only one error object, in case of multiple errors the additional errors will be lost.
• I declared checkConfiguration() within the parse() function in order to have all variables in the scope. checkConfiguration() is also supposed to be extended in the future. Some functions as eg. combineAliases() has been declared outside of it.

[bcoe]

I like this approach, I think we'll gradually want to start fleshing it out into a more generic approach; one option would be that we create a configuration object in in index.js that allows us to describe inconsistencies between different config settings, e.g.,:

const configChecks = {
  counts: ['array', 'foo'],
  array: ['bar']
}

I'm comfortable with landing this as a first step though.

[juergba]

Ok, I added two tests.

[juergba]

@bcoe I'm not happy with this PR's title. We populate error, but don't throw an exception, and the incompatible options are: count/array and count/narg.

[bcoe]

fixed.