Separate config keys from CLI flags #1297
Conversation
lib/cli.js
Outdated
| timeout: conf.timeout, | ||
| concurrency: conf.concurrency, | ||
| updateSnapshots: conf.updateSnapshots, | ||
| color: true || conf.color |
There was a problem hiding this comment.
I think this should be 'color' in conf ? color.conf : true. The idea is that it defaults to true if not configured at all, but otherwise the config value should prevail.
It's strange that this doesn't fail any tests though.
There was a problem hiding this comment.
Oh, I don't know how I missed out on this. Was stupid! 😛 So what you're suggesting here is something like color: conf.color ? conf.color : true, right?
There was a problem hiding this comment.
No, that'll still force the value to be true if conf.color is false.
If color is in conf, assume it's a usable value, else use true.
There was a problem hiding this comment.
I get it. Then I guess we can use (conf.color !== undefined) ? conf.color : true. Any thoughts?
There was a problem hiding this comment.
Why not 'color' in conf ? color.conf : true?
There was a problem hiding this comment.
Cool. No problem. I wasn't so sure about that. You just enhanced my knowledge. Thank you. :)
There was a problem hiding this comment.
I pushed the changes. You can check them now!
lib/cli.js
Outdated
| concurrency: conf.concurrency, | ||
| updateSnapshots: conf.updateSnapshots, | ||
| color: true || conf.color | ||
| }, |
There was a problem hiding this comment.
Would you mind sorting the keys alphabetically?
lib/cli.js
Outdated
| @@ -107,31 +121,34 @@ exports.run = () => { | |||
| throw new Error(colors.error(figures.cross) + ' The --require and -r flags are deprecated. Requirements should be configured in package.json - see documentation.'); | |||
| } | |||
|
|
|||
| // copy resultant cli.flags into conf for use with Api and elsewhere | |||
| conf = Object.assign(conf, cli.flags); | |||
There was a problem hiding this comment.
Object.assign(conf, …) modifies and returns conf. There's no need to reassign it (since you're reassigning it to itself).
sindresorhus
changed the title
|
Nice work @yatharthk 👌 |
|
@sindresorhus Happy to contribute 😄. Thanks to @novemberborn for all the help! |
Fixes #1046
This separates out package config keys from CLI flags by picking up defaults from
confand passing them tomeowas defaults. Furthermore, the resultantcli.flagsis copied backconfso that it can be used elsewhere (instead ofcli.flags).@novemberborn The changes have been made as per our discussion about the same. Let me know if something doesn't seems like fitting in right!