fix: Do not treat an array of config values as a sub-command config section #1221
Conversation
There was a problem hiding this comment.
I like the idea of switching these options to array -- simple and effective 👍
Is there a way to tell yargs not to allow multiple declarations of the other arguments (like --artifacts-dir)? All I see is nargs(1) but I don't think that's what we want.
I had some change requests around Flow types and some test cleanup.
tests/unit/test.config.js
Outdated
| pref: ['pref1=true', 'pref2=false'], | ||
| }; | ||
|
|
||
| // TODO: expect a raised exception array is not a string |
There was a problem hiding this comment.
What does this todo comment mean? Was there another test you wanted to add?
tests/unit/test.config.js
Outdated
| globalOpt: { | ||
| pref: { | ||
| demand: false, | ||
| type: 'string', |
There was a problem hiding this comment.
Shouldn't this be type: 'array' so that it tests the bug you were fixing more directly? I'm a bit confused because the exception message in your assertion doesn't seem to match the spec of this test.
| @@ -401,7 +401,7 @@ Example: $0 --help run. | |||
| 'run against multiple targets.', | |||
| default: 'firefox-desktop', | |||
| demand: false, | |||
| type: 'string', | |||
| type: 'array', | |||
There was a problem hiding this comment.
Can you also update the Flow type:
It should now just be target?: Array<string>.
There was a problem hiding this comment.
I applied these changes to the flow types in
025db7c
| @@ -447,15 +447,15 @@ Example: $0 --help run. | |||
| 'preference.', | |||
| demand: false, | |||
| requiresArg: true, | |||
| type: 'string', | |||
| type: 'array', | |||
There was a problem hiding this comment.
Could you also update this Flow type:
It should now just be cliPrefs: Array<string>. I think you may need to update some tests after that.
| coerce: coerceCLICustomPreference, | ||
| }, | ||
| 'start-url': { | ||
| alias: ['u', 'url'], | ||
| describe: 'Launch firefox at specified page', | ||
| demand: false, | ||
| requiresArg: true, | ||
| type: 'string', | ||
| type: 'array', |
There was a problem hiding this comment.
This Flow type needs an update too
src/program.js
Outdated
| logStream.makeVerbose(); | ||
|
|
||
| if (!versionLogged) { | ||
| log.info('Version:', getVersion(absolutePackageDir)); |
There was a problem hiding this comment.
Oof. This duplication is unfortunate. I can't think of another way to do it though. I suppose you could pull it into a shared function just for a bit more maintainability.
There was a problem hiding this comment.
yeah, I agree, I've refactored the part that enables the verbose mode and prints the version into a class method and then reused it into the two places where we have to be sure that the verbose mode is being enabled (when it is specified on the command line and when it is only specified in a config file).
Let me know how it looks to you.
tests/unit/test.program.js
Outdated
| const loadJSConfigFile = makeConfigLoader({ | ||
| configObjects: { | ||
| [customConfig]: { | ||
| sourceDir: finalSourceDir, |
There was a problem hiding this comment.
This doesn't seem like it's necessary for the test. You could remove it along with the variable.
|
Let's ship it |
This PR contains a set of proposed changes to fix #1213, #1214 and #1215
#1213 is fixed by prevent
applyConfigToArgvto recurse on an array of config values (by consider it as a sub-command config section by mistake)#1214 is fixed by checking if
adjustedArgv.verboseistrueafter we loaded and applied the config file and then callinglogStream.makeVerboseaccordingly.#1215 proposed fix is (as describe in this comment) to mark explicitly as array the options that may makes sense as an array of values in the config file