Restructure result conventions (again)

A new attempt to improve the results (#12 #22 #23 #38 #55) prompted by proposed changes to input configuration (#63), and with better understanding of some of original intent (#38 (comment)).

For context, the configuration syntax proposed in #63 is:

parseArgs({
  args: process.argv.slice(2),
  options: {
    foo: {
      short: 'f',
      type: 'string', // defaults to 'boolean'
      multiple: true,
      // default: 'bar' - Future iteration
    }
  },
})

Result Properties

I propose result has properties for:

optionFound: object with properties and true value for all parsed options found in args.
- e.g. optionFound: { debug: true, name: true, mult: true }
values: object with properties and values for the parsed options
- e.g. values: { debug: true, name: 'John', mult: ['a', 'b'] }
positionals: array of positionals from args
- e.g. positionals: ['alpha']

This is a rename of flags to optionFound. The intent of flags name was the options without values, but the proposed meaning is all options found in args as per original proposal.

vs status quo: storing all the options found in optionFound matches the current implementation, but not the current README examples which are only showing true for boolean options. See also following section on values.

The optionFound field is initially redundant and can be calculated from values. However, it offers a semantic affordance now, and is not redundant if we add support for defaults (or setting known boolean flags to false, say) . Both Command and Yargs did not start out with a concept of whether the option was found in the input args, and that led to requests later as not possible to tell from a value whether was an explicit default (say for a string) or an implicit default (say for a boolean) or for from the parsed arguments.

Values

I propose values property holds for an option:

string for option with a value in args (normal case for option with type:'string')
true for option used as boolean flag (normal case for option with type:'boolean')
array if multiples:true, with true and/or string elements

The change is storing true as the value for a found boolean option. I think this is the natural value, and is what other parsing libraries do such as Command and Yargs.

vs status quo: the current README examples are omitting boolean options entirely. The current implementation is storing undefined rather than true.

This design seems quite reasonable to me, with a nit that I don't love the variable name optionFound. To match the other values returned, seems like a picking something plural would makes sense, some suggestions:

optionsSet.
optionsProvided.
passedOptions

+1 for passedOptions

Agreed, this design seems reasonable to me as well. However, while were discussing the results object, I'd like to challenge the upcoming relevance of the name values. If #63 lands and withValues is removed, is values still the best name to convey it is the parsed args based on the options config? I quite like the intuitiveness of the meow api where the flags configuration is also the name of the parsed flags result.

What are folks thoughts around this convention?

const { options } = parseArgs({
  options: {
    foo: {
      type: 'string'
    },
  },
})

I like passedOptions too on its own and not too bad beside options:

const { options, positionals, passedOptions } = parseArgs({ strict: false });

A wild challenge would be to drop passedOptions for now. A quick sanity check, is anyone actually wanting to use passedOptions in code or examples? I'm not looking for a compelling use case, I'm just checking if there is any interest in the extra result, or everyone just ignoring it because it does not affect how you envisage library being used!

One complication with using options as property in both directions is it requires extra code to make this work:

const { options, positionals, passedOptions } = parseArgs({ args, options });

(I don't see any issues against Meow though, so perhaps does not come up much in practice.)

I've definitely run into that kind of issue before, but i think "options" is a pretty confusing name to pass as a property here.

Nit. I have gone off passedOptions as a result property name since we pass "options" into the configuration. In fact, the unit tests use passedOptions as the variable name!

parseargs/test/index.js

Line 31 in b412095

const passedOptions = { f: { type: 'string' } };

I wasn't enthusiastic about my original suggestion of optionFound either. My current plan is I'll have a go at refactor with foundOptions which at least has plural on end.

One complication with using options as property in both directions is it requires extra code to make this work

Can you point out the extra code? I'm struggling to see complications in the example.

I have gone off passedOptions as a result property name since we pass "options" into the configuration.

passedOptions still makes sense and reads well to me with that framing. Authors pass options into parseArgs and retrieved the (parsed) passedOptions on the other side.

In fact, the unit tests use passedOptions as the variable name!

I'm not sure this should influence API naming conventions. We could simply rename the test vars from passedOptions to optionConfigs

Can you point out the extra code? I'm struggling to see complications in the example.

I wasn't very clear, or accurate... What I was thinking was if there was already a variable called options in play then need to rename when destructuring, like from:

const { options, positionals, passedOptions } = parseArgs({ args, options });

to

const { options: resultOptions, positionals, passedOptions } = parseArgs({ args, options });

passedOptions still makes sense and reads well to me with that framing. Authors pass options into parseArgs and retrieved the (parsed) passedOptions on the other side.

Morally I feel the foundOptions are determined by the input args, and not the input options.

Although might be a good time to check expectations! In the future, if we added defaults to the input options, I was assuming a default would not cause the option to appear in foundOptions but would cause the option to appear in values.

A different name I thought of and you hint at is parsedOptions. How does that scan?

If we ever add defaults, it absolutely must be possible to determine if a defaulted value was explicitly passed or not; to do this in yargs I have to default things to, eg, Object('foo'), and then see if the value is the string foo or the cached Object foo, which is a horrific workaround.

"parsed" is better than "passed", because "passed" implies it's unchanged from what the user passed - but it is changed, because it's parsed.

For interest, Commander only added support for determining source of option value in v8.3.0 in late 2021 after about a decade without a mechanism. e.g. 'default' vs 'CLI'

https://github.com/tj/commander.js/blob/master/CHANGELOG.md#830-2021-10-22

@shadowspawn

@shadowspawn @ljharb what if rather than an object that contains all objects that were explicitly set, we have the opposite defaulted which contains all the options that were set to their default value?

I think this could serve a similar purpose we're looking to solve, but to me makes the return values less confusing. We could then perhaps run with:

const { values, positionals, defaulted } = parseArgs({ args, options });

I could also be pushed towards:

const { options, positionals, defaulted } = parseArgs({ args, options });

But I don't love the overloaded term options.

How do we feel about just having one object for parsed options containing the value and usage metadata? I think all the discussion points in issue #43 and PR #63 for input options apply to the outputs e.g. More natural to have all the information in one place and lays the foundation for future iterations:

// node parsedOptions.js --foo bar

const optionConfigs = {
  foo: { type: 'string' },
  baz: { type: 'string', default: 'hello' },
}
const { options, positionals } = parseArgs({ options: optionConfigs })
/*
options === {
  foo: {
    value: 'bar',
    defaulted: false,
    parsed: true,
  },
  baz: {
    value: 'hello',
    defaulted: true,
    parsed: false,
  },
}
*/

@ljharb

Here's a scrappy example of the ParsedOptions type with some examples of future iterations:

type ParsedOptions = {
    [option: string]: {
      /** Value corresponding to the `optionConfig.type`; Set to `optionConfig.default` if option was not passed */
      value: string | boolean
      /** True if the parsed option's value used the `optionConfig.default` */
      defaulted: boolean;

      // Future iterations...      

      /**
       * Could be helpful to understand how the parser inferred `strict:false` options: e.g.
       * node inferred.js --foo // parsedOptions === { foo: { type: 'boolean', value: true }}
       */
      type: 'string' | 'boolean'
      /** Number of times the option was defined */
      count: number
    }
}

As an example of how this pattern can be leveraged, a parsedOption.count field could address @ljharb's callout in #80 (comment) about optionConfig.type === 'boolean' and optionConfig.multiple === true having a somewhat obscure result (e.g. an array of true values [true, true, etc.]). Instead the result could simply be:

// node verbose.js -vvv
const { options } = parseArgs()
/*
options === {
  v: {
    type: 'boolean', // Helpful to see what the parser inferred
    value: true, // Returns a single option for type:'boolean'
    count: 3, // Metadata includes the frequency of the option
  }
}
*/

@shadowspawn

@shadowspawn @ljharb what if rather than an object that contains all objects that were explicitly set, we have the opposite defaulted which contains all the options that were set to their default value?

Actually, I would rather delete it. (Which I'll suggest in next comment!) The sense of "defaulted" is referencing a feature we don't even have yet.

Short version: I propose we drop optionFound. (The property previously known as args and flags, and also suggested as passedOptions, parsedOptions, and most recently inverted defaulted...)

Long version

I already speculated about dropping it earlier in this issue, and got no defenders (#70 (comment)). I think it is time to get serious about deleting it! I have been asking what it is for since Nov last year and got no compelling answers, and we are still trying to give it a sensible name, with no compelling use case. 😱

Why have the field at all?

Suggested: differentiate between the case where an option was passed no argument, vs., an option not being set at all (#13 (comment))
Counter: we can tell the difference without it. An option that appears in the input args will have an entry in values, an option not set at all will not appear in values.

Suggested: simplify testing for existence of option in args (#24 (comment)).
e.g. const fooExists = !! optionFound.foo
Counter: with values as described here: const fooExists = values.foo !== undefined (or != null if you prefer!)
Slightly longer to be fair, but you are going to be working with values anyway so not a new concept. Other packages use single property for the parsed options (#12 (comment)).

Suggested: tell where value came from in future (e.g. if add defaults)
Counter: do what makes sense in future

How do we feel about just having one object for parsed options containing the value and usage metadata?

My initial reaction is no, I think combining the values and usage metadata is overkill and complicating the primary target audience. i.e.

Thus, process.parseArgs() is not necessarily intended for library authors; it is intended for developers of simple CLI tools, ad-hoc scripts, deployed Node.js applications, and learning materials.

In the future, if there is demand for this as a base for building richer parsers and custom behaviour, I think there could be a metadata containing property. (Which could perhaps include the value as well so was self-contained.)

I think combining the values and usage metadata is overkill

This feels similar to the initial reactions when I proposed the restructured option configs API. I'm trying to imagine what parseArgs will look like one year from now and make sure we're considering what API will support new features and enhancements. From what I can see, we are going down the same path as the original option configs API where information for a single option is scattered across the primary config object.

Here is an example of what I'm trying to avoid after say one year of features and enhancements:
Please don't take my example features as literal suggestions, but rather to illustrate general enhancements to parseArgs.

// Where we're headed...
const {
  foundOptions: { foo: true },
  values: { foo: true },
  defaulted: { foo: false },
  count: { foo: 2 },
  types: { foo: 'boolean' },
  etc...
} = parseArgs()

// Proposal...
const {
  options: {
    foo: {
      type: 'boolean',
      value: true,
      defaulted: false,
      count: 2,
      etc...
    },
  },
} = parseArgs()

As far as the proposal being complicated for the target audience, I'm not sure I follow. I believe my suggestion improves the robustness of the API and is actually more intuitive for the target audience.

Certainly if we have no "defaults" at all, then it's easy to see when an option isn't provided.

However, it seems pretty important for almost every CLI workflow i know to have defaults, and for some, it's critical to be able to know whether they're defaulted or not.

Here is an example of what I'm trying to avoid after say one year of features and enhancements

Fair concern. I don't see organic growth of user-level properties as a likely outcome. Other libraries have had a decade of features and enhancements, and have a single layer of object values available. See #12 (comment)

If we add more information for people building richer parsers, I do prefer the idea of putting that richer metadata into one property and not scattering across multiple top level properties.

Other metadata mentioned in #52 (comment) #52 (comment) #84

(More metadata in: #84)

I am thinking a property like parseDetails which has data of use for people building on top of the library, but is usually ignored by direct users of the library. As with Aarons example, these properties are not suggestions just illustrations.

e.g.

const {
  values: { foo: true },
  positionals: ['index.js'],
  parseDetails: {
    options: {
      foo: { argIndex: 1, argIndices: [1], choosyArgumentIgnored: true, inconsistentUsage: true },
    },
    optionDelimiter: { argIndex: 2 },
    positionals: [
       { argIndex: 3 }
    ]
    }
  }
} = parseArgs({ strict: false })

Other libraries have had a decade of features and enhancements, and have a single layer of object values available.

100%, Totally valid argument. However, I still (personally) feel the combination of option value and metadata is the more intuitive/future-facing API. The closest example I can think of is RegExp match objects.

Notice: Each match object contains the full match, number and named capture group values, the start index, indices of capture groups, etc.

Now imagine if instead String.prototype.matchAll() returned the matches in one object and all the metadata in a separate one. Wouldn't be as intuitive imo.

I may be reaching a bit with the above demo, but am attempting to provide a real-world example of a parsed input that includes values and metadata in the same result to demonstrate the intuitiveness of the API.

@aaronccasanova

@aaronccasanova:

I think the overwhelmingly common case is going to be just getting the value without any metadata, possibly with destructuring, as in

let { values } = parseArgs(opts);
let { foo, bar = defaultBar } = values;

And that gets much more annoying if metadata is mixed with values. I think it's important to optimize for the common case.

Note that even in the matchAll example, there are separate groups and indices properties, and the groups property is an object which directly associates values with named groups; the metadata for named groups, i.e. the indices, are in a separate .indices.groups field. We had the destructuring example in mind when designing named capture groups for regexes, IIRC.

🔨 Moving towards MVP Milestone 1 (#87)

See #83 !

(Exploring interest, not pushing for a change!)

I do like options as the result property with flags gone:

const { options, positionals } = parseArgs({ strict: false });

I am not keen on using same property on input and output despite the symmetry (#70 (comment)), and feel there is additional confusion on the input usage with common usage of "options" for an option bag.

Perhaps the input property could be renamed? Inspired in part by strict:false usage:

const { options, positionals } = parseArgs({ 
   strict: false, 
   knownOptions: { host: { type: 'string' }}
});

Perhaps the input property could be renamed?

No objections from me. I was thinking the same thing after the initial concerns around using options for both the input and output. I'm a fan of knownOptions, optionConfigs, optionsConfig, etc. Kind of liking optionsConfig 🤓 (naming convention reminds me of the package.json publishConfig key).

const { options, positionals } = parseArgs({ 
   strict: false, 
   optionsConfig: { host: { type: 'string' }}
});

Landed #83. Closing this, the culmination for five months of issues. 😅

Navigation Menu

Repository navigation

Restructure result conventions (again) #70

Result Properties

Values

2 remaining items

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Restructure result conventions (again) #70

Description

Result Properties

Values

Activity

bcoe commented on Feb 27, 2022

aaronccasanova commented on Feb 27, 2022

shadowspawn commented on Feb 27, 2022

shadowspawn commented on Feb 28, 2022

ljharb commented on Feb 28, 2022

shadowspawn commented on Mar 12, 2022

aaronccasanova commented on Mar 13, 2022

shadowspawn commented on Mar 14, 2022

shadowspawn commented on Mar 14, 2022

ljharb commented on Mar 14, 2022

shadowspawn commented on Mar 14, 2022

bcoe commented on Mar 16, 2022

aaronccasanova commented on Mar 16, 2022

2 remaining items

aaronccasanova commented on Mar 16, 2022

shadowspawn commented on Mar 17, 2022

shadowspawn commented on Mar 17, 2022

shadowspawn commented on Mar 17, 2022

aaronccasanova commented on Mar 17, 2022

ljharb commented on Mar 17, 2022

shadowspawn commented on Mar 18, 2022

shadowspawn commented on Mar 20, 2022

aaronccasanova commented on Mar 28, 2022

bakkot commented on Mar 28, 2022

shadowspawn commented on Mar 30, 2022

shadowspawn commented on Apr 2, 2022

aaronccasanova commented on Apr 2, 2022

shadowspawn commented on Apr 13, 2022

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions