Added config-dot-notation for config files.
#190
Conversation
Defaut is false
README.md
Outdated
| @@ -177,6 +177,33 @@ node example.js --foo.bar | |||
| { _: [], "foo.bar": true } | |||
| ``` | |||
|
|
|||
| _Note:_ This setting does not apply to config files (any more). See `config-dot-notation`. | |||
There was a problem hiding this comment.
Perhaps:
_Note: as of yargs-parser@14.0.0, dot-notationhas been split intodot-notationandconfig-dot-notation_
README.md
Outdated
| * default: `false` | ||
| * key: `config-dot-notation` | ||
|
|
||
| Should keys in the config file that contain `.` split up in objects? |
There was a problem hiding this comment.
nit:
Should keys in the config file that contain
.be split into objects.
| @@ -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 }, | |||
There was a problem hiding this comment.
I don't think this logic should need to change because we don't have keys of the form "foo.bar"?
There was a problem hiding this comment.
Yes, I see what you mean, but it fails without it. Ok, I'll have to look into this one more, tomorrow or Friday.
| @@ -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: { | |||
There was a problem hiding this comment.
It would be good to add two tests, demonstrating "foo.bar" with config-dot-notation enabled and disabled.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
I think I'm either misunderstanding your goals, or there's a bug in this implementation.
I thought what we wanted was for keys in config files with .s to not be expanded.
I believe in this current implementation, we just skip merging objects that are served via arguments with objects that are served via config files.
| @@ -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']) { | |||
There was a problem hiding this comment.
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"
}
}
|
just wanted to bump this PR back up, still something you'd like to see over the finish line @f0zi? |
|
I'd love to, but I don't think I have the time for this in the next few weeks or so. If you don't mind, don't close it, I might come back to it when I find some time. |
|
@f0zi closing for now, but please feel free to come back and dust this off when you have time. |
In reply to yargs/yargs#1305.