-
Notifications
You must be signed in to change notification settings - Fork 343
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: Do not treat an array of config values as a sub-command config section #1221
Changes from 1 commit
3848c77
1a54ce9
025db7c
6ded415
8f8e17c
5fa17c8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -401,7 +401,7 @@ Example: $0 --help run. | |
'run against multiple targets.', | ||
default: 'firefox-desktop', | ||
demand: false, | ||
type: 'string', | ||
type: 'array', | ||
}, | ||
'firefox': { | ||
alias: ['f', 'firefox-binary'], | ||
|
@@ -447,15 +447,15 @@ Example: $0 --help run. | |
'preference.', | ||
demand: false, | ||
requiresArg: true, | ||
type: 'string', | ||
type: 'array', | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you also update this Flow type: It should now just be |
||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This Flow type needs an update too |
||
}, | ||
'browser-console': { | ||
alias: ['bc'], | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -296,6 +296,30 @@ describe('config', () => { | |
assert.strictEqual(argv.ignoreFiles, configObject.ignoreFiles); | ||
}); | ||
|
||
it('does not mistake an array config values for a sub-command', | ||
() => { | ||
const params = makeArgv({ | ||
userCmd: ['fakecommand'], | ||
globalOpt: { | ||
pref: { | ||
demand: false, | ||
type: 'string', | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shouldn't this be |
||
}, | ||
}, | ||
}); | ||
|
||
const configObject = { | ||
pref: ['pref1=true', 'pref2=false'], | ||
}; | ||
|
||
// TODO: expect a raised exception array is not a string | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What does this todo comment mean? Was there another test you wanted to add? |
||
assert.throws( | ||
() => applyConf({...params, configObject}), | ||
UsageError, | ||
'type of "pref" incorrectly as "array" (expected type "string")' | ||
); | ||
}); | ||
|
||
it('uses CLI option over undefined configured option and default', () => { | ||
const cmdLineSrcDir = '/user/specified/source/dir/'; | ||
const params = makeArgv({ | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you also update the Flow type:
https://github.com/rpl/web-ext/blob/3848c777b290b4c5ed6ad1af0436afe5f952b005/src/cmd/run.js#L44
It should now just be
target?: Array<string>
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I applied these changes to the flow types in
025db7c