Add implied to typedef and precedence

[mshima]

Pull Request

Problem

I was trying to understand source. I had difficulties, so did some changes to try to make easier to understand.
Following #1788 (comment).

• add implied to definition
• implement _isNewSourcePreferred instead of hardcoded includes
• set 'unknown' source when using setOptionValue(), instead of not changing the source.

Solution

ChangeLog

[shadowspawn]

1. setOptionValue() should be clearing the source, as you suggest. Moving the value-location logic to setOptionValueWithSource() makes sense. I think undefined would be simpler than "unknown".

2. source priorities

I had wondered about making priorities more explicit in earlier PR, but it got too complicated for what at the time was just adding one more usage! Did some direct tests instead. See if we make it work this time...

I am finding it hard to reason about what _isNewSourcePreferred(a, b) means, and the code in there is way complicated! I wonder if a routine like _getSourcePriority(c) would be clearer so then can use >= or = or whatever makes sense for the use case. An unknown or undefined source would return 1 higher than highest known.

[shadowspawn]

I was having a look at the code being touched, and will have to refresh myself on the goals.

Some of the processing relies on the order of the various methods and does not respect a "priority" as such. I went to some care so a parsing function would only be called once if both environment variable and CLI defined, so more than just which is higher priority.

The current code is effectively only over-writing known sources in a couple of places. Treating it as priorities might be overpromising.

[mshima]

1. setOptionValue() should be clearing the source, as you suggest. Moving the value-location logic to setOptionValueWithSource() makes sense. I think undefined would be simpler than "unknown".

IMO undefined is means no value is set, 'unknown' is value is set with unknown source.
This avoids getOptionValue() === undefined && getValueSource() to know that the source is actually unknown.

I am finding it hard to reason about what _isNewSourcePreferred(a, b) means, and the code in there is way complicated! I wonder if a routine like _getSourcePriority(c) would be clearer so then can use >= or = or whatever makes sense for the use case. An unknown or undefined source would return 1 higher than highest known.

This can be simplified by removing the second parameter and changing the comparison a bit.

Some of the processing relies on the order of the various methods and does not respect a "priority" as such. I went to some care so a parsing function would only be called once if both environment variable and CLI defined, so more than just which is higher priority.

Yes, that isn't a pattern right now.
Ideally, _isNewSourcePreferred should be checked at setValueWithSource.

setValueWithSource(value, source) {
  if (!this._isNewSourcePreferred(source)) return false;
  [set the value]
  return true;
}

[shadowspawn]

Have you got a compelling use case for .strictPriority()? I think people making decisions with complex priorities might be better to handle it themselves.

I have experimenting with it myself in past, so know it is tempting! But not sneaking in new features when the initial goal is just adding 'implied' to the known sources... 😄

On a somewhat related note, I had wanted to make author supplied value possible, and have opened a PR with some typing to achieve that over here: commander-js/extra-typings#3

[mshima]

Actually the goal of this PR is to priorities make sense.
setValueWithSource() is useless exposed in current state. It should be internal only.

• There is no hook to be used.
• Implementing the priority check is overwhelming complicated, it's easier to implement config load at a later step without using commander.

Since 'config' source is 'built-in' I would love to use something like this with having to worry about priorities:

program
  .on('parseConfig', function() {
    const attributeNames = this.options.map(o => o.attributeName());
    Object
      .entries(JSON.parse(readSync(this.opts().configFile).toString()))
      .filter(([attributeName, value]) => attributeNames.includes(attributeName))
      .forEach(([attributeName, value]) => this.setValueWithSource(attributeName, name, 'config'));
  })

A custom option source is not useful and will complicate even more the situation. It should be implemented after the above point are resolved.

I really don't know if there is anything in this PR you think it's useful other than the 'implied' at source definition.
Fill free to edit the PR or close.
I would love to use untouched commander but a custom command is satisfactory for my needs.
Thanks.

[shadowspawn]

Ok, thanks, that makes the intent of this PR make more sense. I was struggling with this PR based on its problem definition.

I didn't pick up from the other issues that you are trying to implement a config source and were hoping "priorities" would make it easier.

.setValueWithSource() got exposed in #1613 and was initially implemented internally when adding .env(). Because I was working on configuration at the time, I added config into the env handling so that if the option values had already been added (in some unspecified way) the environment variables would still overwrite them. A bit optimistic!

My experience in the development of .env()was that robust solutions are much easier when the work is done at the appropriate point in the parsing for the desired priority. Adding in the option values later in the parsing would mostly work but there would be edge cases where the result would be wrong or unexpected or what I wanted.

For configuration files specified using options this is extra complicated! Not just how to manage the priorities, but how to trigger the option reading at all. For configuration from a fixed source (like say GITHUB environment variables) I think this should be easier.

You mentioned 'parseConfig' in a couple of issues. Maybe open an issue about what you are trying to do currently and what the problem is. Two implementation decisions are:

• for a single valued option, which source wins? (aka priority!)
• for a multiple value option (like variadic) do the sources both contribute, or only take from one?

[shadowspawn]

I would love to use untouched commander but a custom command is satisfactory for my needs.

Glad you aren't blocked! Don't open a parseConfig issue unless it would be useful to you.

(I'm going to do a couple of changes from issues raised in this PR.)

[shadowspawn]

Added #1794 and #1795