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

Added config-dot-notation for config files. #190

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 27 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -177,6 +177,33 @@ node example.js --foo.bar
{ _: [], "foo.bar": true }
```

_Note: as of yargs-parser@14.0.0, `dot-notation` has been split into `dot-notation` and `config-dot-notation`_

### config-dot-notation

* default: `false`
* key: `config-dot-notation`

Should keys in the config file that contain `.` split into objects?

example config.json:
```json
{
"foo.bar": true
}
```
_if enabled:_
```sh
node example.js --config config.json
{ _: [], foo: { bar: true } }
```

_if disabled:_
```sh
node example.js --config config.json
{ _: [], "foo.bar": true }
```

### parse numbers

* default: `true`
Expand Down
3 changes: 2 additions & 1 deletion index.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ function parse (args, opts) {
'short-option-groups': true,
'camel-case-expansion': true,
'dot-notation': true,
'config-dot-notation': false,
'parse-numbers': true,
'boolean-negation': true,
'negation-prefix': 'no-',
Expand Down Expand Up @@ -545,7 +546,7 @@ function parse (args, opts) {
// if the value is an inner object and we have dot-notation
// enabled, treat inner objects in config the same as
// heavily nested dot notations (foo.bar.apple).
if (typeof value === 'object' && value !== null && !Array.isArray(value) && configuration['dot-notation']) {
if (typeof value === 'object' && value !== null && !Array.isArray(value) && configuration['config-dot-notation']) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This logic seems wrong to me, which I believe is why you've had to explicitly turn on config-dot-notation to get tests passing.

What I believe we want, when config-dot-notation is false is to still run setConfigObject (which takes care of merging the config file with our arguments.

It's a specific edge-case you want to turn off, which is how someone handles a config file that looks like this:

{
  "foo.com": {
     "hello": "world"
  }
}

// if the value is an object but not an array, check nested object
setConfigObject(value, fullKey)
} else {
Expand Down
2 changes: 2 additions & 0 deletions test/yargs-parser.js
Original file line number Diff line number Diff line change
Expand Up @@ -567,6 +567,7 @@ describe('yargs-parser', function () {
it('should load nested options from config file', function () {
var jsonPath = path.resolve(__dirname, './fixtures/nested_config.json')
var argv = parser(['--settings', jsonPath, '--nested.foo', 'bar'], {
configuration: { 'config-dot-notation': true },
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this logic should need to change because we don't have keys of the form "foo.bar"?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I see what you mean, but it fails without it. Ok, I'll have to look into this one more, tomorrow or Friday.

config: ['settings']
})

Expand Down Expand Up @@ -718,6 +719,7 @@ describe('yargs-parser', function () {

it('should load nested options from config object', function () {
var argv = parser(['--nested.foo', 'bar'], {
configuration: { 'config-dot-notation': true },
configObjects: [{
a: 'a',
nested: {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be good to add two tests, demonstrating "foo.bar" with config-dot-notation enabled and disabled.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I was thinking of adding a test but I saw "does not append nested-object keys from config to top-level key" seems to be testing exactly that.

Except, now that I look at it again, the only thing I would change is line 2100 change 'dot-notation': false to 'config-dot-notation': false to make it explicit.

Or would you rather like me to add another test case for this?

Expand Down