Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

make it possible to require long options with values use = #52

Closed
ljharb opened this issue Jan 27, 2022 · 11 comments · Fixed by #136
Closed

make it possible to require long options with values use = #52

ljharb opened this issue Jan 27, 2022 · 11 comments · Fixed by #136
Labels
bring-your-own-code Experiments with how authors might build on parseArgs discussion

Comments

@ljharb
Copy link
Member

ljharb commented Jan 27, 2022

Per discussion around #25 (comment), I want to use this API, but enforce that options with values use an = sign.

It's fine if the validation code is in userland (ie, my code) on top of parseArgs.

How can I do that?

@shadowspawn
Copy link
Collaborator

There is information available to the author in the setup of the parsing configuration, in addition to the parseArgs results.

Possible approach, with bonus support for --:

const allArgs = process.argv;
const withValue = ['foo'];

const dashdash = allArgs.indexOf('--');
const possibleOptions = (dashdash >= 0) ? allArgs.slice(0, dashdash) : allArgs;

withValue.forEach((option) => {
    if (possibleOptions.includes(`--${option}`)) {
        console.error(`--${option} must have a value specified using "=", like "--${option}=value"`);
        process.exit(2);
    }
})

parseArgs(allArgs, { withValue });

@ljharb
Copy link
Member Author

ljharb commented Jan 28, 2022

imo I think that having to look at process.argv is a failure of parseArgs - in particular, parsing it myself (which your snippet does).

Perhaps there's metadata that could be provided that could give me enough info to directly do the validation?

@shadowspawn
Copy link
Collaborator

shadowspawn commented Jan 28, 2022

I have some empathy with your reaction. But there are only going to be nice answers to some custom changes and additions to the parseArgs base behaviour.

I think the "Intended Audience" from the previous proposal was quite realistic in this regard. #45 (comment)

There are other parsing decisions which are not represented in the results, and an author can not always change the rules by looking at the results. For example, an author wanting to make it an error if an option is specified more than once. parseArgs will silently return the second usage.

A different case that came up recently in context of parseArgs was npm allows -ws as a shortcut for --workspace. The wisdom of this is debatable, but it is a nice example for something more elegantly done before calling parseArgs. Making the substitution before parsing avoids -ws triggering short option groups, and gets the full support in parsing for withValue and multiples for --workspace.

@shadowspawn shadowspawn removed their assignment Jan 29, 2022
@shadowspawn
Copy link
Collaborator

shadowspawn commented Jan 30, 2022

Another possible approach, parseArgs does the parsing and then bring-your-own validation. I think this is more in the style you are wanting.

const myStringOptions = ['foo'];
const myBooleanOptions = ['flag'];

// Do not declare any withValue since do not want to allow "--foo value"
const { options, values } = parseArgs({ strict: false });

// Bring your own validation.
// (Assuming boolean option stores true, which has not landed yet)
Object.keys(options).forEach((option) => {
    if (myBooleanOptions.includes(option)) {
        if (typeof values[option] !== 'boolean') {
            console.error(`Unexpected value for option: ${option}`);
        }
    }
    else if (myStringOptions.includes(option)) {
        if (typeof values[option] === 'string') {
            console.error(`Missing value for option: ${option}`);
        }
    } else {
        console.error(`Unknown option: ${option}`);
    }
});

[Edit: some refactoring for newer terminology and conventions.]

@bcoe bcoe added the discussion label Feb 6, 2022
@bcoe
Copy link
Collaborator

bcoe commented Feb 6, 2022

As with #45, I'm a bit worried about feature creep ... yargs, and probably commander too?, ended up with dozens of configuration options to support people's various personal preferences for command line parsing. I'd like to side step this in Node core, by sticking to the message:

It is already possible to build great arg parsing modules on top of what Node.js provides; the prickly API is abstracted away by these modules. 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.

It is exceedingly difficult to provide an API which would both be friendly to these Node.js users while being extensible enough for libraries to build upon. We chose to prioritize these use cases because these are currently not well-served by Node.js' API.

I agree that if we could come up with a reasonable hook for "bringing your own validation", perhaps we could both keep the API surface really small, while supporting various preferences.

@bakkot
Copy link
Collaborator

bakkot commented Feb 6, 2022

I agree that if we could come up with a reasonable hook for "bringing your own validation"

To throw out an option I've used before in other parsing libraries: if the parsed object surfaces (or can be made to surface) location information, that makes it easy for a consumer to do this sort of check. For example, imagine that there's a indexes property (name obviously tbd) which gives a map from each option to the indexes in argv which contributed to that option:

const { flags, values, positionals, indexes } = parseArgs(argv, { withValue, ... });
for (let option of withValue) {
  for (let index of indexes[option] ?? []) {
    if (!argv[index].includes('=')) {
      console.error(`--${option} must have a value specified using "=", like "--${option}=value"`);
      process.exit(2);
    }
  }
}

It also makes it possible to build many other sorts of validation, like "was this flag duplicated" (check to see if indexes[flag].length > 1), or "option A must be specified before option B" (just compare their indexes).

It's almost universal for parsers to expose location information (even regexes do, these days!), so there's good precedent for this kind of thing.


(It might be better for indexes to be a tiny bit richer than I've described it here, since as described you can't tell without re-parsing if a given index was --arg=value, --arg, or value. So perhaps { index, kind } pairs would be better than just the raw number.)

@shadowspawn
Copy link
Collaborator

Interesting idea. I like your examples of situations that this can detect: duplicate options, ordered options. What parsing libraries offer this, or is this something you added yourself?

However, I'm only exploring. I don't think exposing more information is important at this time!

@bakkot
Copy link
Collaborator

bakkot commented Feb 6, 2022

What parsing libraries offer this

No JS-based CLI argument parser I'm aware of does this, to be clear, but parsers in general almost always do. See pretty much every parser on astexplorer.net, for some examples.

@bcoe
Copy link
Collaborator

bcoe commented Feb 6, 2022

I like @bakkot's suggestion a lot. Another advantage of this design, is that errors can point to the exact token in process.argv that broke the parse, if we do opt for strict by default.

There's a standard parse format I've used before, uinst, https://www.npmjs.com/package/unist-util-visit. Which has the benefit of tooling behind it.

I agree with @shadowspawn, perhaps this is a post MVP addition, but can be a plan for how we expose highly customizable validation.

@shadowspawn
Copy link
Collaborator

For interest and can safely be ignored, I came across a mode in Minimist that could have led to confusion about long options and values. Minimist lets you switch zero-config from withValue to default flag (boolean), but only affects long flags. (Minimist is pretty cool, but is not a shining example of consistent UX behaviour...)

var argv = require('minimist')(process.argv.slice(2));
console.log(argv);
% node boolean-true.js -a A --vv=VV --ww WW
{ _: [], a: 'A', vv: 'VV', ww: 'WW' }
var argv = require('minimist')(process.argv.slice(2),  { boolean: true });
console.log(argv);
% node boolean-true.js -a A --vv=VV --ww WW
{ _: [ 'WW' ], a: 'A', vv: 'VV', ww: true }

@shadowspawn
Copy link
Collaborator

Support for more meta-data has been added in #129
Using the new meta-data to require that long option values are supplied with = is an example in #136

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bring-your-own-code Experiments with how authors might build on parseArgs discussion
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants