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

Behaviour for withValue --foo followed by --bar ? #25

Closed
shadowspawn opened this issue Dec 22, 2021 · 24 comments
Closed

Behaviour for withValue --foo followed by --bar ? #25

shadowspawn opened this issue Dec 22, 2021 · 24 comments

Comments

@shadowspawn
Copy link
Collaborator

shadowspawn commented Dec 22, 2021

I am working through some of the special cases during a refactor of the results code.

parseArgs(['--foo', '--bar'], { withValue: ['foo'] )

This is a case where the parser could behave two ways. The --foo option is expecting a value, there is an argument available, but the argument looks like an option.

The current implementation is that foo gets the value undefined, and --bar is seen as a flag.

  1. I think seeing --bar as a flag rather than the value is the least surprising behaviour, so just confirming that this is as intended and/or expected by others?
  2. Would it be better if foo was an empty string rather than undefined? This might be more consistent from a typing point of view. I don't have a wrong preference.

Current behaviour, assuming flags are stored in flags and values are stored in values:

flags // { bar: true }  <-- ok?
values // { foo: undefined } <-- or empty string?

If the user wants the --bar to be the value then they can achieve this with --foo=--bar.

For comparison:

  • In Commander both behaviours are available. An option with a required option-argument does eat the next argument, but an option wth an optional option-argument does not eat arguments that look like options.
  • In Minimist, with foo configured to be a "string" it gets an empty string as value, and --bar is parsed as a flag.

Added TL;DR proposal: #25 (comment)

@ljharb
Copy link
Member

ljharb commented Dec 22, 2021

On the command line, --foo must be either true or undefined, and --foo= would be the empty string.

In other words, without the equals sign, I think it must be undefined; with, it must be the empty string.

I also agree that valid dashed things shouldn't be eligible to be values, eg am unquoted --bar can't be the value for --foo.

@shadowspawn shadowspawn self-assigned this Dec 28, 2021
@darcyclarke
Copy link
Member

  1. I think seeing --bar as a flag rather than the value is the least surprising behaviour, so just confirming that this is as intended and/or expected by others?

As @ljharb noted, --bar should never be parsed/returned as a value & seems like the right behavior to me if we're already doing that.

  1. Would it be better if foo was an empty string rather than undefined? This might be more consistent from a typing point of view. I don't have a wrong preference.

Setting the value as an empty string infers it's a "truthy" value, which sort of goes against the idea of "bringing your own validation" (which, imo, should be a goal of ours if we want to minimize critics of when/if/how we determine those values). --foo= & --foo="" should not be the same thing & my expectations are that they equate to undefined & "" values respectively.

For me, the following examples illustrate what my ultimate expectations would be if we were to properly disassociate the existence of, vs. value of, flags... (ref. to my comment over here: #24 (comment) on the differentiation for "existence of" vs. "value of")

parseArgs(['--foo', '--bar']) 
/*
{
  flags: { foo: true, bar: true }, 
  values { },
  positionals: []
}
*/

parseArgs(['--foo', '--bar'], { withValue: ['foo'] }) 
/*
{
  flags: { foo: true, bar: true }, 
  values { foo: undefined },
  positionals: []
}
*/

parseArgs(['--foo=', '--bar=""'], { withValue: ['foo'] }) 
/*
{
  flags: { foo: true, bar: true }, 
  values { foo: undefined },
  positionals: []
}
*/

parseArgs(['--foo=', '--bar=""'], { withValue: ['foo', 'bar'] }) 
/*
{
  flags: { foo: true, bar: true }, 
  values { foo: undefined, bar: '' },
  positionals: []
}
*/

parseArgs(['--foo=', 'baz', '--bar=""'], { withValue: ['foo', 'bar'] }) 
/*
{
  flags: { foo: true, bar: true }, 
  values { foo: 'baz', bar: '' },
  positionals: []
}
*/

parseArgs(['--foo=', 'baz'], { withValue: ['foo', 'bar'] }) 
/*
{
  flags: { foo: true, bar: false }, 
  values { foo: 'baz', bar: undefined },
  positionals: []
}
*/

parseArgs(['--foo=', 'baz', '--bar=false'], { withValue: ['foo', 'bar'] }) 
/*
{
  flags: { foo: true, bar: true }, 
  values { foo: 'baz', bar: 'false' },
  positionals: []
}
*/

@shadowspawn
Copy link
Collaborator Author

shadowspawn commented Jan 18, 2022

parseArgs(['--foo=', '--bar=""'], { withValue: ['foo'] }) 
/*
{
  flags: { foo: true, bar: true }, 
  values { foo: undefined },
  positionals: []
}
*/

Disagree, I recommend empty string is implied by --foo=

@shadowspawn
Copy link
Collaborator Author

shadowspawn commented Jan 18, 2022

parseArgs(['--foo=', '--bar=""'], { withValue: ['foo', 'bar'] }) 
/*
{
  flags: { foo: true, bar: true }, 
  values { foo: undefined, bar: '' },
  positionals: []
}
*/

Disagree, empty string as before but fun speech marks case:

values { foo: '', bar: '""' }

I think the bar value is the string following the equals, which because it is passed in code is actually two double speech marks. I know this may not have been intended, but I think it is nice it came up. I suggest we do not do any fancy speech mark processing, KISS (Keep It Simple Stupid).

Now if --bar="" had been supplied on the command-line the shell would have likely turned it into --bar= anyway, so a quirk of an example in code.

@shadowspawn
Copy link
Collaborator Author

parseArgs(['--foo=', 'baz', '--bar=""'], { withValue: ['foo', 'bar'] }) 
/*
{
  flags: { foo: true, bar: true }, 
  values { foo: 'baz', bar: '' },
  positionals: []
}
*/

Disagree, if you use the equals then the value is what is in the same argument after the equals.

parseArgs(['--foo=', 'baz', '--bar=""'], { withValue: ['foo', 'bar'] }) 
{
  flags: { foo: true, bar: true }, 
  values { foo: '', bar: '""' },
  positionals: ['baz']
}

@shadowspawn
Copy link
Collaborator Author

shadowspawn commented Jan 18, 2022

Recapping the original question:

parseArgs(['--foo', '--bar'], { withValue: ['foo'] )

The current implementation is that foo gets the value undefined, and --bar is seen as a flag.

Current feedback: Jordan and Darcy and I all happy with status quo.

@shadowspawn

This comment has been minimized.

@shadowspawn
Copy link
Collaborator Author

shadowspawn commented Jan 21, 2022

I changed my mind! (Actually the end result may be the same, but my reasoning is now different.)

Short version: Looks like a flag, stored like a flag.

Assuming we stop storing undefined in values for flags:

parseArgs(['--foo', '--bar'], { withValue: ['foo'] )
// { flags: { foo: true, bar: true }, values: {}, positionals: [] }

Long version

I initially felt it was wrong to store the same results as a flag, because we knew we were expecting a value. But the knowledge we were expecting a value is external to the results, we still know that whatever we store.

If --foo is used like a flag and without a value, the results should be same as for a flag (as if foo not in withValue).

Treating the result like a flag was used has a nice simple symmetry:

  • How do I detect if a boolean flag was used wrong? Check if result like a withValue option was used.
  • How do I detect if a withValue option was used wrong? Check if result like a boolean flag was used.
  • the parse results do not contain a state that can only occur for this one case, instead the results contain a familiar state reflecting the usage

Looks like a flag, stored like a flag. Looks like a value, stored like a value.

@shadowspawn
Copy link
Collaborator Author

shadowspawn commented Jan 27, 2022

Here is a run through of my proposed behaviours for an option listed in withValue. The option is explicitly expecting a value.

The naming and output I have used here is based on the "existence" pattern per: #38 (comment)

  • returned options: track the existence of an option in parsed args no matter whether used as a flag or with a value
  • returned values: option values (plain boolean flags do not have a value)
const { parseArgs } = require('@pkgjs/parseargs');
const { options, values } = parseArgs(process.argv, { withValue: ['foo'] });]
console.log({ options, values });

Remember, only the first usages are consistent with a flag which takes a value. The later cases are misuse which can be detected by the client if desired.

$ node zero.js --foo bar
{ options: { foo: true }, values: { foo: 'bar' } }
$ node zero.js --foo=bar
{ options: { foo: true }, values: { foo: 'bar' } }
$ node zero.js --foo=
{ options: { foo: true }, values: { foo: '' } }

$ node zero.js --foo
{ options: { foo: true }, values: {} }
$ node zero.js --foo --bar
{ options: { foo: true, bar: true }, values: {} }

[Edit: added missing withValue to parseArgs call.]

@ljharb

This comment has been minimized.

@shadowspawn

This comment has been minimized.

@ljharb
Copy link
Member

ljharb commented Jan 27, 2022

OK, but still, --foo=bar is how you pass a value for foo. --foo bar is two separate arguments, despite that there's some approaches (like npm's) that incorrectly allow you to omit the equals sign.

@aaronccasanova
Copy link
Collaborator

Why is omitting the equal sign incorrect?

@shadowspawn
Copy link
Collaborator Author

I think of --foo bar as a direct equivalent to -f bar. It is very widely supported, including all of the Big Four on downloads: Commander, Yargs, minimist, and argparse.

It is widely accepted in the parsers consulted in the initial tooling proposal. See the column with the = in survey in original proposal:

nodejs/tooling#19 (comment)

@ljharb
Copy link
Member

ljharb commented Jan 27, 2022

That's the convention (from GNU and/or POSIX, i think). -f b or --f=b, not --f b.

I realize it is widely accepted, but that doesn't mean it's the proper default. At the very least, both "modes" should be configurable - requiring the equals sign, or allowing it to be omitted.

@shadowspawn
Copy link
Collaborator Author

Why configurable? An end-user always using the form --foo=bar is fully supported. We don't need to require it to make that so.

The man pages for git for checkout show using an = with --pathspec-from-file in the synopsis and in the longer description. But git does not require the = at runtime.

% git checkout --pathspec-from-file=foo
fatal: could not open 'foo' for reading: No such file or directory
% git checkout --pathspec-from-file foo
fatal: could not open 'foo' for reading: No such file or directory

Do you have an example of a common utility that supports long options and does not support --foo bar?

@shadowspawn
Copy link
Collaborator Author

shadowspawn commented Jan 27, 2022

The four conventions that I see commonly supported for supplying a value for an option which requires a value are:

$ ex --foo bar
$ ex --foo=bar
$ ex -f bar
$ ex -fbar

@ljharb
Copy link
Member

ljharb commented Jan 27, 2022

I explicitly want to require use of the = sign with double-dashed arguments for every single command line tool I touch.

To be fair, the more i search around, the more I'm horrified to find that everything seems to be loosey-goosey with this, and allow the = to be omitted; but I can't recall the last time I've seen help output or examples omit the =.

@targos
Copy link

targos commented Jan 27, 2022

The help output of gh (the GitHub CLI) omits the = 🙈

@ljharb
Copy link
Member

ljharb commented Jan 27, 2022

there's always a counterexample :-)

sadly i think i'm convinced that the default needs to allow omitting the =, but i think it's very important that there be a way to enforce it.

@aaronccasanova
Copy link
Collaborator

aaronccasanova commented Jan 27, 2022

Ideally we could just have a strong opinion that = is always optional and not configurable. I find it to be a small inconvenience when tools require an equal sign and I have to go digging around to confirm (Sure I can just trial and error one or the other, but I prefer seeing it in writing).

$ ex --foo bar
$ ex --foo=bar
$ ex -f bar
$ ex -fbar

This all looks great to me except the last option which I would expect to be expanded to -f -b -a -r. Though we don't have to dig into that here as it's already being discussed elsewhere.

@shadowspawn
Copy link
Collaborator Author

shadowspawn commented Jan 27, 2022

but i think it's very important that there be a way to enforce it.

I disagree, I do not think there needs to be way to enforce it in this library. I suggest we discuss that in a separate issue if desired. This issue is intimidatingly long, but great to uncover some more assumptions!

@ljharb
Copy link
Member

ljharb commented Jan 27, 2022

@shadowspawn it's fine if the validation part is done on my own, but how can it be possible for me to enforce it with this library? Filed #52 for this.

@shadowspawn
Copy link
Collaborator Author

I wasn't clear on what issues I was uncovering when I opened this issue, so got interesting wide discussion. 😅

The parseArgs implementation is currently "choosy" as initially proposed here. I have opened #77 for a separate more informed discussion of "choosy" vs "greedy".

The result of parsing a type:string option missing an option-argument are:

  • strict: false same result as for a boolean option, up to author to deal with
  • strict: true (proposed) throw error for missing option-argument

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants