-
-
Notifications
You must be signed in to change notification settings - Fork 30
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
[New] Support for arrays in dotted notation (--a.0.b
)
#66
Conversation
For future reference, that's what issues are for :-) this isn't common syntax - is there a reason you can't do |
Thanks for your quick answer!
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.
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: 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). |
Given those two options, why is the second one preferable? |
It isn't. Both are OK, I don't prefer any of them. |
I decided to use |
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. |
I pulled the test case in with bedaa8b to ensure we have coverage of this input. |
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.
Yes. We could hide this feature behind a minimist config option that must be set explicitly. |
f535a96
to
6bb4bae
Compare
6bb4bae
to
73c65ea
Compare
Don't get me wrong, I'm perfectly fine with |
See feature/dotted-arrays-brackets for an implementation supporting bracket syntax |
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. |
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. |
--a.0.b
)
Closed due to comment. |
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:
Actual behavior
Desired behavior
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).