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 zero config --foo=a ? #24

Closed
shadowspawn opened this issue Dec 22, 2021 · 37 comments · Fixed by #43
Closed

Behaviour for zero config --foo=a ? #24

shadowspawn opened this issue Dec 22, 2021 · 37 comments · Fixed by #43

Comments

@shadowspawn
Copy link
Collaborator

shadowspawn commented Dec 22, 2021

I suggest the intent of the user is clear, and the value should be stored?

Added TL;DR proposal: #24 (comment)


The initial README did not clarify what happens with --foo=a with no configuration, as the result was overwritten by repeated option --foo b.

I assumed the value would be stored in update to README in #23, which has landed.

The current implementation ignores the value unless withValue is set for foo.

@shadowspawn shadowspawn changed the title Result for zero config --foo=a Behaviour for zero config --foo=a ? Dec 22, 2021
@bcoe
Copy link
Collaborator

bcoe commented Dec 28, 2021

@shadowspawn are you thinking that --foo-a should implicitly behave as if foo was configured withValue? This seems like a good assumption to me.

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

darcyclarke commented Jan 16, 2022

I don't think this is a good idea. We should be keeping as few assumptions out of this implementation as possible & force the user to define when/if values are to be captured & returned or not (which puts them into a better overall experience).

What I think is happening here, & is concerning, is that we've changed the meaning of args (aka. flags) which is causing us to loose information & muddy context along the way.

Looking back (ref. https://github.com/pkgjs/parseargs/blob/3f057c1b158a1bdbe878c64b57460c58e56e465f/README.md), args (aka. flags) was meant to track the existence of an argument no matter what its value, or the provided function options, were. withValue was meant to define whether to return any value back in values (ie. a parsed string from "key=value", a captured positional string or undefined).

Looking at the most recent version of the spec (2022/01/16):

// unconfigured
const argv = ['-f', '--foo=a', '--bar', 'b'];
const options = {};
const { flags, values, positionals } = parseArgs(argv, options);
// flags = { f: true, bar: true }
// values = { foo: 'a' }
// positionals = ['b']

I now need to do two different lookups to find the existence of an argument, & we've created an arbitrary delineation of two "types" of arguments (ie. ones with & without values). Worse yet, afaict, there's no way to determine existence, vs. value, when using withValue.

// unconfigured
const argv = ['-f', '--foo=a', '--bar', 'b'];
const options = {};
const { flags, values, positionals } = parseArgs(argv, options);
const fooExists = (flags.foo || Object.keys(values).foo)

// `withValue`
const argv = ['-f', '--foo=a', '--bar', 'b'];
const options = { 
  withValue: [ 'foo' ] 
};
const { flags, values, positionals } = parseArgs(argv, options);
const fooExists = (flags.foo || Object.keys(values).foo) // always true

Previously, I could write the following no matter what the options...

const { args, values, positionals } = parseArgs(argv, options);
const fooExists = !!args.foo

Either way, my knee-jerk reaction here is that if we assume parsing of --key=value then...

  • we need to determine what the value is we return for --key= with & without withValue: [ 'key' ] defined (potentially making further assumptions)
  • we need to re-evaluate withValue as a whole (as it seems like it would only be relevant for positional value association/parsing now)

@shadowspawn
Copy link
Collaborator Author

What I think is happening here, & is concerning, is that we've changed the meaning of args (aka. flags) which is causing us to loose information & muddy context along the way.

I was a driver for that, sorry. The intended usage and potential benefits were not apparent to me (#13).

I am not sure there is more information with the original implementation, but I'll work through your example carefully before commenting further. Thanks for the detail, and I look forward to discussing the issues raised!

  1. What behaviour were you expecting for an unconfigured --key=value?

  2. Is your primary use case a user whipping up a CLI, or using this as a base for a more robust and fuller implementation?

@darcyclarke
Copy link
Member

darcyclarke commented Jan 16, 2022

  1. What behaviour were you expecting for an unconfigured --key=value?

My original thought was thatkey would get added to flags & value would be ignored (I'm not opposed to changing this though, see more below)

const { flags, values, positionals } = parseArgs(['--foo=a']);
// flags = { foo: true }
// values = { }
// positionals = []
  1. Is your primary use case a user whipping up a CLI, or using this as a base for a more robust and fuller implementation?

Both. The ease of use & understanding parseArgs().flags.foo returning the existence of --foo captures both audiences.

Take these two examples:

// node program.js --foo=
parseArgs().flags.foo // undefined
parseArgs().values.foo // undefined

// node program.js --foo
parseArgs().flags.foo // true
parseArgs().values.foo // undefined

This feels like non-deterministic behavior as in both cases I've set a flag but only changed how I've declared it's value. If I were able to rely on flags always returning true when a flag existed I can write better programs or abstractions.

Example: quick & dirty...

// node program.js --debug # or...
// node program.js --debug="Some debugger prefix"
if (parseArgs().flags.debug) {
  const prefix = parseArgs().values.debug || 'Debugger'
  console.log(`${prefix}:`, process)
}

Example: more fully-baked validation & aliasing...

const { flags, values } = parseArgs({ withValue: ['org', 'force'] })
const settings = {
  org: {
    alias: 'b',
    usage: 'run this program with `--org <name>`',
    required: true,
    validate: Object.toString
  },
  force: {
    alias: 'f',
    usage: 'run this program with `--force` to force some action',
    default: false,
    validate: (value) => {
      if (~['true','false'].indexOf(value.toLowerCase())) {
        return Boolean(value)
      }
      throw Error('Type validation error!')
    }
  }
}
Object.entries(settings).map(([flag, option]) => {
  const isDefined = flags[arg] || flags[option.alias]
  const value = values[flag] || values[option.alias]
  if (!isDefined && option.required)
    throw Error('Required arguments are missing', option.usage)
  if (defined && option.validate)
    values[flag] = option.validate(value)
})

On "zero config"...

I'm honestly not that opposed to parsing --key=value by default but it does change how I see withValue & it's usefullness.

Changes & explicit rules we might want to consider if we go this route...

  • drop withValue
  • introduce acceptsPositionalValues(or some better name, we can bikeshed here)
  • --key= parses to undefined
  • --key=value parses to a string value
  • --key=value1 --key value2 parse things differently with & without acceptsPositionalValues &/or multiples options associated with the key

Examples of this in practice...

Notably, these all include/respect the idea that we'd return the existence of flags again in flags (as referenced above):

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

// key=value
parseArgs(['--foo=a']) 
/*
{
  flags: { foo: true }, 
  values { foo: 'a' },
  positionals: []
}
*/

// multiple flags, no options
parseArgs(['--foo=a', '--foo']) 
/*
{
  flags: { foo: true }, 
  values { foo: undefined },
  positionals: []
}
*/

// multiple flags, with `multiples` option
parseArgs(['--foo=a', '--foo'], { multiples: [ 'foo' ] }) 
/*
{
  flags: { foo: true }, 
  values { foo: [ 'a', undefined ] },
  positionals: []
}
*/

// positional, no options 
parseArgs(['--foo=', 'bar']) 
/*
{
  flags: { foo: true }, 
  values { foo: 'a' },
  positionals: ['bar']
}
*/

// positional, with `acceptsPositionalValues` option
parseArgs(['--foo=', 'bar'], { acceptsPositionalValues: ['foo'] }) 
/*
{
  flags: { foo: true }, 
  values { foo: 'bar' },
  positionals: []
}
*/

@shadowspawn
Copy link
Collaborator Author

Examples of this in practice...

A quick note that I think --foo= should set the value to the empty string, not to undefined.

(Early morning here. Hopefully I will have time tonight for some in depth replies.)

@darcyclarke
Copy link
Member

darcyclarke commented Jan 16, 2022

"(Early morning here. Hopefully I will have time tonight for some in depth replies.)"

No worries. Take your time. I know I wrote a lot (apologies; I'm playing catch-up for not having time over the last ~3-6months)

"A quick note that I think --foo= should set the value to the empty string, not to undefined.

We might just have to agree to diasgree 🤷🏼‍♂️ (I noted as much in our other thread). I don't feel super strongly about it though unless that decision effects if/how we throw in strict mode (ie. would we consider --foo= an error if withValue: [ 'foo' ] & strict: true were both defined... even though we would parse that "" otherwise 🤔 something to consider imo).

For me, my expectation is that --foo & --foo= would be the same (ie. undefined). --foo="" is explicitly an empty string.

I know the context is somewhat different but in JS... v not relevant

let foo=;    // is an error
let foo;     // is undefined
let foo='';  // is an empty string

@shadowspawn
Copy link
Collaborator Author

shadowspawn commented Jan 16, 2022

For normal command-line use, there is parsing going on from shell script before node or client code every see the args. I think this makes the distinction between --foo= and --foo="" moot, they look the same by the time they reach parseArgs. Simple example:

$ echo --foo=""
--foo=
$ echo --foo=  
--foo=

@darcyclarke
Copy link
Member

darcyclarke commented Jan 16, 2022

With that in mind, I'd think that --foo should probably be treated the same way... so --foo="", --foo= & --foo all set values.foo === ""; thoughts?

@ljharb
Copy link
Member

ljharb commented Jan 17, 2022

--key= should parse to the empty string, just like in a URL query string. --key should parse to either undefined or true.

@shadowspawn
Copy link
Collaborator Author

shadowspawn commented Jan 17, 2022

I'll pick up this comment first because I think it highlights some key points of confusion or contention. I won't write an essay about every two sentences! And epic exposition in later comment #24 (comment) , thanks @darcyclarke , I'm going to read through that multiple times before commenting on it.

I now need to do two different lookups to find the existence of an argument, & we've created an arbitrary delineation of two "types" of arguments (ie. ones with & without values).

There are different user affordances provided by either approach. To be clear, I am not against your original intentions now you have explained it. From experience with Commander and reading about Yargs and Minimist, I expected a single result object rather than a pair and came up with a different pattern to try and justify the pair: an explicit delineation of two types of arguments, the ones with and without values. I actually thought this might have been part of the original intent, some sort of type safety, one holds booleans and one holds strings in the normal case. I was way wrong about that intent. :-)

Let's assume for a moment we stick with the current suggestion, misguided as it may be. I don't think there is loss of information or difficulty in use.

As the author, I know what I am expecting. If I want to check a boolean flag has been used, I simply check flags[key]. If I want to check an option for which I am expecting a value, if I am ignoring empty strings I can simply check !!values[key], or to include the empty string then values[key] !== undefined.

If I am testing for the existence of an argument with or without a value, I do need to check two values. But this does not seem like a happy-path case.

Worse yet, afaict, there's no way to determine existence, vs. value, when using withValue.

I don't think I understand. If foo is included in withValue, it will have a value in values if it was specified on the command-line. It will not have a value in values if it was not specified on the command-line. The tricky case is --foo without a positional to supply the value (#25) and the suggestion was to store undefined. (I think storing undefined should be be avoided in happy-path cases because it is a pain to test for and a pain to talk about, the property is defined with the value undefined. More reasonable in unhappy path cases like this one.)

@shadowspawn
Copy link
Collaborator Author

shadowspawn commented Jan 17, 2022

(Treating --foo=a like implicit withValue.)

I don't think this is a good idea. We should be keeping as few assumptions out of this implementation as possible & force the user to define when/if values are to be captured & returned or not (which puts them into a better overall experience).

My original thought was thatkey would get added to flags & value would be ignored (I'm not opposed to changing this though, see more below)

My concern with that approach when I came across it in the implementation was it has thrown away information. This seems inconsistent with "bring your own validation".

@shadowspawn
Copy link
Collaborator Author

shadowspawn commented Jan 17, 2022

I should say explicitly that I am not heavily invested in the current mutually exclusive flags/values! (I came in expecting a single options result with strings for options with values and true for boolean flags. The XOR flags/values was influenced by that more common pattern.)

I am fine with the seen/any-value concept in theory. However, I am questioning whether it preserves any additional information. If there is no extra information, the pattern has to stand on its other merits.

Part of the point of my refactor in PR #26 was to make it easy to adjust the behaviour, and easy to reuse the behaviour when adding short options.

@shadowspawn
Copy link
Collaborator Author

shadowspawn commented Jan 17, 2022

This may be a side-track, but caught my eye.

Example from #24 (comment)

Example: quick & dirty...

// node program.js --debug # or...
// node program.js --debug="Some debugger prefix"
if (parseArgs().flags.debug) {
  const prefix = parseArgs().values.debug || 'Debugger'
  console.log(`${prefix}:`, process)
}

Alarm! Alarm! When I reread this, are you wanting to routinely support options which may or may not have a value supplied on the command line? If that is the intent, I have a longer story about why I think options with optional values should be avoided as a concept in a simple parser. (In Commander they are explicitly supported, so I have some experience. They seem best of both worlds, a boolean flag or an option with a value, but lead to end-user confusion about parsing outcomes. I admit they are easy to implement with seen/any-value semantics though!)

You may have just used this as an example of conveniently coping with an error case, in which case disregard this alarm! You did say:

force the user to define when/if values are to be captured & returned or not (which puts them into a better overall experience).

@shadowspawn
Copy link
Collaborator Author

shadowspawn commented Jan 17, 2022

I'm honestly not that opposed to parsing --key=value by default but it does change how I see withValue & it's usefullness.

If we do parse the value, I still see withValue as the preferred approach for a well defined user interface, to make these two the same:

do --foo=bar
do --foo bar

and to (potentially) make this a detectable error:

do --foo

It is appealing that taking a value from --foo=bar allows zero-config values if the author and the end-user agree on the convention, as the end-user intent is clear. But cunning-appealing often comes with a cost, and this is a special case we don't need to support.

My problem is I don't think silently parsing it as a boolean flag and discarding the value is appropriate. (Not least because of possible exposure to programs using Minimist and Yargs, and erroneous use of --explode=false.)

Hmm, a suggestion from most recent Tooling meeting was to pass-through unrecognised uses. Maybe this qualifies, so this could be parsed as a positional? A bit wild and would be ambiguous with uses of --. A third unsatisfying approach!

Maybe this is a good case for strict to detect and reject?!

@shadowspawn
Copy link
Collaborator Author

Example: more fully-baked validation & aliasing...

This is a nice example of serious intent! #24 (comment)

I think the aliases need to be passed to parseArgs in shorts, not handled afterwards.

The two lines that change are the first two lines (from example, with aliases removed):

  const isDefined = flags[flag]
  const value = values[flag] 
  if (!isDefined && option.required)
    throw Error('Required arguments are missing', option.usage)
  if (defined && option.validate)
    values[flag] = option.validate(value)

Arguably it would be better for lone --foo to be detected as missing required value, and included that as extra example.

With XOR flags/values semantics:

// to match original example behaviour faithfully
const isDefined = values[flag] !== undefined || flags[flag]
const value = values[flag] 

// `--foo` detected as missing required value, and empty string allowed
const isDefined = values[flag] !== undefined
const value = values[flag] 

To implement these with a combined options object with true for boolean flags, like Commander or Yargs:

// to match original example behaviour faithfully
const isDefined = combined[flag] !== undefined
const value = combined[flag]

// `--foo` detected as missing required value, and empty string allowed
const isDefined = typeof  combined[flag] === 'string'
const value = combined[flag]

@shadowspawn
Copy link
Collaborator Author

shadowspawn commented Jan 17, 2022

I found that all worthwhile, but should I just change my refactor PR to the seen/value semantics and we run with that again as the original vision?

(And separately improve the documentation so gentle readers understand the intent.)

(And probably change the name of flags, again.)

(And I am not a fan of storing undefined for the value for a boolean flag, but since it is same in most code as storing nothing it won't get in the way much in practice. Will just annoy me until I see some concrete benefit. 😄 )

@shadowspawn
Copy link
Collaborator Author

shadowspawn commented Jan 18, 2022

Ok, back on topic for a fresh look informed by discussions.

I asked what the behaviour should be for zero config --foo=a. I consider this an edge case because not consistent with what author specified. Possible approaches with pros and cons.

  1. Treat as if foo was configured in withConfig (and store value)
  • pro: what the end-user intends
  • pro: no data loss for bring-your-own-validation
  • con: not what author configured
  1. Treat as if --foo was specified as a plain boolean flag (and discard value)
  • con: not what the end-user intends
  • con: data loss so no bring-your-own-validation
  • pro: consistent with what author configured
  1. Punt and return arg as positional.
  • con: not what the end-user intends
  • neutral: some data loss for bring-your-own-validation, as ambiguous with -- escaped arguments
  • con: not really consistent with what author configured
  1. Independent of above, should probably be an error if we support strict

Early feedback: Ben and Jordan and I ok with (1).
Darcy open to possibility of (1), but still catching up and considering implications (which could affect early feedback).

@ljharb
Copy link
Member

ljharb commented Jan 18, 2022

I think if not 1, we’d want 4.

@Eomm
Copy link
Collaborator

Eomm commented Jan 19, 2022

I think the 1st option is the best default to implement

@shadowspawn
Copy link
Collaborator Author

Option 1 is consistent with my new story for high level approach. Looks like a flag, stored like a flag. Looks like a value, stored like a value.

@ljharb
Copy link
Member

ljharb commented Jan 21, 2022

In other words, the way they're stored matches the intention of the user, not the configurer, which will ensure the configurer can most accurately respond to the user's intentions.

@shadowspawn
Copy link
Collaborator Author

(Yes indeed. Nicely put @ljharb, thanks.)

@shadowspawn
Copy link
Collaborator Author

shadowspawn commented Jan 27, 2022

Here is a run through of my proposed behaviours for an option used with zero-config. The option is implicitly a boolean flag and not 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();
console.log({ options, values });

Remember, only the first usage is consistent with a flag which does not take a value. The other two are misuse which can be detected by the client if desired.

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

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

@ljharb
Copy link
Member

ljharb commented Jan 27, 2022

I’m confused; with zero-config, why would foo ever give true if there’s an equals sign present?

@shadowspawn
Copy link
Collaborator Author

Because I am using the convention from Darcy's initial spec that one of the results holds whether an option was used on the command line at all, either as a flag or with a value. So in this example the options result records the existence of the option in the input args. (Unfortunately, having multiple conventions in flight is making it hard to talk about anything clearly!) See #38 for longer discussion. I'll add a line clarifying what options is.

@ljharb
Copy link
Member

ljharb commented Jan 27, 2022

That makes sense, thanks - so we have "options", "values", and "flags"?

@shadowspawn
Copy link
Collaborator Author

Those are the terms I am using, yes. (Or are you asking if parseArgs returns all three?)

@ljharb
Copy link
Member

ljharb commented Jan 27, 2022

I'm asking if those are the three potential properties of that object.

@shadowspawn
Copy link
Collaborator Author

The returned object will have just two of those properties, depending on which we implement:

  • "options" and "values" (aka "existence"), or
  • "flags" and "values" (xor, option appears in one or the other)

(I'm being very careful as I am not sure I understand your question and not sure if yes or no is the answer! Ask again if my answer is still not useful, we might be at cross-purposes.)

@ljharb
Copy link
Member

ljharb commented Jan 27, 2022

If I configure one flag, one value, and three arguments are passed, I would expect "options" to contain all three, "flags" to contain one or two, and "values" to contain one or two. No?

@shadowspawn
Copy link
Collaborator Author

shadowspawn commented Jan 27, 2022

Short version: no, or at least it depends on the arguments.

(If you give me an example call, I'll lay out the two result flavours I have in mind.)

@ljharb
Copy link
Member

ljharb commented Jan 27, 2022

node index.js --foo --bar --baz, where foo is defined as a flag and bar as withValue?

@shadowspawn
Copy link
Collaborator Author

First clarification, there is currently no ability to define flags. (Which is a blocker for strict!)

Second, I'll reply with a worked example, but have run out of time to make sure I get it right. Will answer later.

@ljharb
Copy link
Member

ljharb commented Jan 27, 2022

If there were such an ability, then would my expectations match? ie, { options: { foo: true, bar: true, baz: true }, flags: { foo: true }, values: { foo: undefined, baz: undefined } }?

@shadowspawn
Copy link
Collaborator Author

node index.js --foo --bar --baz, where foo is defined as a flag and bar as withValue?

Long answer.

In this comment:

  • returned options records whether an option appears in the args
  • returnedflags records whether an option has been used as a flag in the args
// hypothetical `flags` and `strict` configuration
console.log(parseArgs(['--foo', '--bar', '--baz'], { flags: 'foo', withValue: ['bar'], strict: false }));

foo: configured as a flag and used as a flag
bar: configured as withValue but used as a flag
baz: zero-config flag and used as a flag

"options" and "values" (aka "existence")

{
  options: { foo: true, bar: true, baz: true },
  values: {},
  positionals: []
}

"flags" and "values" (xor, option appears in one or the other)

{
  flags: { foo: true, bar: true, baz: true },
  values: {},
  positionals: []
}

Side note: I am suggesting we do not store undefined in values for flags, and just leave it undefined. Author still writes code like values[key] !== undefined without needing to wonder about hasOwnProperty.

@ljharb
Copy link
Member

ljharb commented Jan 28, 2022

That also means Object.keys(values) lacks useful information.

"needing to wonder about hasOwnProperty" is also "is able to differentiate between absent and undefined", which is a pretty important ability.

@shadowspawn
Copy link
Collaborator Author

That also means Object.keys(values) lacks useful information.

Yes indeed, values is not the sole source of truth. But the questions I considered can easily be answered with the information available.

(I started out working through examples testing for explicit undefined and was not happy with the experience. Then I had the realisation that "looks like a flag, store like a flag" reduces and simplifies the possible results. Still championing that concept!)

Leaving aside multiples for now, an option is either used as a flag or with a value. The author know which they intended. The results show what the end-user did.

Have a look at the uses cases I run through in #38 to see if I missed something important.

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

Successfully merging a pull request may close this issue.

5 participants