minimistjs / minimist

parse argument options
MIT License
515 stars 30 forks source link

[New] Support for arrays in dotted notation (`--a.0.b`) #66

Closed git-developer closed 1 month ago

git-developer commented 1 month ago

I'd like to suggest a feature that seems to be missing: Support for arrays in dotted notation.

Let me explain the feature request using this simple example:

$ echo "console.log(require('minimist')(['--foo.0.bar=x', '--foo.2.bar=42']))" | node

Actual behavior

{ _: [], foo: { '0': { bar: 'x' }, '2': { bar: 42 } } }

Desired behavior

{ _: [], foo: [ { bar: 'x' }, <1 empty item>, { bar: 42 } ] }

Motivation

I'd like to use program args as alternative or optional addon to a configuration file. In my concrete use case, I read a YAML configuration and merge it with the values from command line args. Minimist supports this greatly for anything except arrays (lists).

Comments

Please tell if you think this feature could be useful. I apologize for this minimal PR with a single test and no documentation. I'd like to discuss the feature before putting more work into it.

All tests are still green, but since this feature is not fully backwards compatible, we could enforce to enable it explicitely by a config option. If you agree, please advise on how to implement that (javascript is not one of my native languages).

ljharb commented 1 month ago

I'd like to discuss the feature before putting more work into it.

For future reference, that's what issues are for :-)

this isn't common syntax - is there a reason you can't do [0] instead of .0 in your yaml config?

git-developer commented 1 month ago

Thanks for your quick answer!

For future reference, that's what issues are for :-)

I decided to create a PR because it's easier to link a code proposal. I can open and link an issue if you prefer that.

this isn't common syntax - is there a reason you can't do [0] instead of .0 in your yaml config?

The YAML config is not a problem. Let me try to explain the use case with a simple example. Configuration:

read:
- file: /tmp/a
  every: 5s
- file: /tmp/b
  every: 1h

The user should be able to override this configuration using command line args: $ node demo.js --read[0].every=10s or $ node demo.js --read.0.every=10s

My strategy is to parse both the YAML config (using YAML package) and the program args (using minimistjs) into a js object and merge them. This works fine for anything except arrays (lists).

ljharb commented 1 month ago

Given those two options, why is the second one preferable?

git-developer commented 1 month ago

It isn't. Both are OK, I don't prefer any of them.

git-developer commented 1 month ago

I decided to use foo.0.bar because it works using my tiny patch. foo[0].bar is probably more complex to parse.

ljharb commented 1 month ago

One of the reasons yaml is often undesirable is precisely because of the ambiguity between objects and arrays - it seems preferable to me to require users to be explicit.

It could still be a feature request solely on an aesthetic basis - but it'd be a breaking change, and i'm not sure the complexity (which isn't measured in diff size) is warranted solely on that basis.

ljharb commented 1 month ago

I pulled the test case in with bedaa8b9ab5a901fa342aad4494cbbf676b11a21 to ensure we have coverage of this input.

git-developer commented 1 month ago

yaml is often undesirable is precisely because of the ambiguity between objects and arrays

I don't get your point here. The YAML above is not ambigous, it's an array containing 2 objects. The dotted notation is ambiguous because we can't tell if the number is a list index or an object key.

it'd be a breaking change

Yes. We could hide this feature behind a minimist config option that must be set explicitly.

git-developer commented 1 month ago

Don't get me wrong, I'm perfectly fine with foo[0].bar and I agree that this notation is explicit about objects and arrays. So I'd definitely appreciate if this syntax could be supported by minimist. I just can't contribute that because it exceeds my javascript know-how.

git-developer commented 1 month ago

See feature/dotted-arrays-brackets for an implementation supporting bracket syntax foo[0].bar. I can update this PR, or open another PR for it (and create an issue to keep things sorted).

shadowspawn commented 1 month ago

Thanks for the tidy description, including motivation.

I see what you want to do @git-developer , but think it is complicated to implement well, and pretty opaque on the command-line. I do not think this offers enough value.

To expand on the "complicated to implement well", the dotted notation has caused CVE and multiple fixes already (#11 #24).

I noticed we don't have dotted notation even documented in the README currently. That would be a step forward! I'll add an issue.

git-developer commented 1 month ago

Thanks for your feedback. If you think that none of the two approaches offer enough value, it would be great if a user could disable dotted syntax completely in mimimist. This would allow him/her to resolve the dotted notation as needed using a different library.

git-developer commented 1 month ago

Closed due to comment.