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

fix: Config file array files should be recognized as options of array type #1199

Conversation

rpl
Copy link
Member

@rpl rpl commented Jan 9, 2018

This PR contains the changes (and additional test case) to fix #1198

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling fb89c01 on rpl:fix/issue-1198-config-file-array-values-unrecognized into cdb5b00 on mozilla:master.

Copy link
Contributor

@kumar303 kumar303 left a comment

Choose a reason for hiding this comment

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

Hah. I can't believe we missed this. We should support number too.

@@ -66,7 +66,11 @@ export function applyConfigToArgv({
const expectedType = options[decamelizedOptName].type ===
'count' ? 'number' : options[decamelizedOptName].type;

const optionType = typeof configObject[option];
const optionType = (
Array.isArray(configObject[option]) ?
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh man, duh. 🧀

@@ -66,7 +66,11 @@ export function applyConfigToArgv({
const expectedType = options[decamelizedOptName].type ===
'count' ? 'number' : options[decamelizedOptName].type;

const optionType = typeof configObject[option];
const optionType = (
Array.isArray(configObject[option]) ?
Copy link
Contributor

Choose a reason for hiding this comment

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

While you're in here, could you also add support for number types like this one?

Copy link
Contributor

Choose a reason for hiding this comment

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

Nevermind, sorry, I thought that it would need special handling but it does not.

Copy link
Contributor

@kumar303 kumar303 left a comment

Choose a reason for hiding this comment

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

Ignore me about number, I was mistaken. Looks great!

@kumar303 kumar303 merged commit 854c82c into mozilla:master Jan 9, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Array values are not recognized as is in config file
3 participants