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.

Sign up for GitHub

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

Jump to bottom

cli: accept csv for array values #8933

Merged
merged 13 commits into from
May 22, 2019
Merged

cli: accept csv for array values #8933

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
6b8c607
cli: accept csv for array values
connorjclark May 11, 2019
9fe961a
dont hardcode list of array keys
connorjclark May 11, 2019
35e2f01
Revert "dont hardcode list of array keys"
connorjclark May 13, 2019
879b288
add tests for csv whitelist
connorjclark May 13, 2019
3e3cc4c
lint, move out of .check
connorjclark May 13, 2019
ee44360
add example
connorjclark May 13, 2019
0d1a611
move example down, revise param type for flatten
connorjclark May 16, 2019
0067f00
add comment
connorjclark May 16, 2019
6f58b90
fix run test
connorjclark May 16, 2019
e4da3a9
revert getFlags param change
connorjclark May 17, 2019
d49db90
Update lighthouse-cli/cli-flags.js
connorjclark May 20, 2019
2402ce8
Merge branch 'master' into cli-csv
connorjclark May 20, 2019
2b46961
edit comments
connorjclark May 20, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 33 additions & 5 deletions lighthouse-cli/cli-flags.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,13 +12,23 @@ const pkg = require('../package.json');
const printer = require('./printer');

/**
* @param {string=} manualArgv
* @param {(string | string[])[]} arr
* @return {string[]}
brendankenny marked this conversation as resolved.
Show resolved Hide resolved
*/
function flatten(arr) {
brendankenny marked this conversation as resolved.
Show resolved Hide resolved
/** @type {string[]} */
const result = [];
return result.concat(...arr);
}

/**
* @param {string[]} manualArgv
* @return {LH.CliFlags}
*/
function getFlags(manualArgv) {
// @ts-ignore yargs() is incorrectly typed as not returning itself
const y = manualArgv ? yargs(manualArgv) : yargs;
return y.help('help')
function getFlags(...manualArgv) {
const y = manualArgv.length ? yargs(manualArgv) : yargs;
Copy link
Member

Choose a reason for hiding this comment

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

this ends up somewhat confusing from the caller's perspective since it's no longer explicitly optional (even though we usually call it with no args) and when we do pass an argument it's still a single string (it's annoying that the yargs types require this because yargs definitely supports parsing just a string).

What about keeping it string= and doing const y = manualArgv ? yargs([manualArgv]) : yargs; instead?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm confused on how it's confusing to the caller?

if reverting the change, would you want the code using multiple args (admittedly it's just test code) to look like this?

getFlags([
	'http://www.example.com',
	'--only-categories=performance,seo',
	'--skipAudits=unused-javascript,redirects',
	'--skipAudits=bootup-time',
].join(' '));

or this?

getFlags('http://www.example.com --only-categories=performance,seo --skipAudits=unused-javascript,redirects --skipAudits=bootup-time');

Copy link
Member

Choose a reason for hiding this comment

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

if reverting the change, would you want the code using multiple args (admittedly it's just test code) to look like this?

yes from me. Maybe it's just that I haven't been completely indoctrinated to rest parameters yet, but to me an optional manualArgv?: string parameter is a lot clearer than ...manaulArgv: string[] at informing that the argument is optional. It's not a huge deal since it's not a commonly used interface, but it's only test code that uses this, so it makes sense to me to make the real use case clear in the signature.

Copy link
Member

Choose a reason for hiding this comment

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

ha, actually, that test failure may be because our version of yargs doesn't take an array at all?

Copy link
Collaborator Author

@connorjclark connorjclark May 16, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

It passes locally.

(EDIT: misread the build log. one sec)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ok the issue is that if you pass in an array, you cannot have each item have multiple args. test just needs to change

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

also I guess they expect some preprocessing, b/c = is required too. 6f58b90

Copy link
Member

Choose a reason for hiding this comment

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

also I guess they expect some preprocessing, b/c = is required too.

You can only omit the = when passing in as a single string? This seems like the real downside...we should be able to write tests using both styles since the real deal accepts both

/** @type {LH.CliFlags} */
Copy link
Member

Choose a reason for hiding this comment

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

this doesn't buy us much except having to coerce with /** @type {string[]} */ below

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

would like to keep /** @type {string[]} */ otherwise more errors come up. I added a comment saying why we can assume it to be a string[].

LH.CliFlags one could be removed if you want.

Copy link
Member

Choose a reason for hiding this comment

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

would like to keep /** @type {string[]} */ otherwise more errors come up.

but it's wrong because all of this is wrong :)

The ts-ignore was suppressing a different error than it said (I assume/hope it changed at some point), but the comment was still correct ('yargs() is incorrectly typed as not returning itself'): all the calls on yargs after help() are of type any and return any. In addition:

  • the type of LH.CliFlags[keyof LH.CliFlags] is definitely not a string array, really it should be LH.CliFlags['onlyAudits'|'onlyCategories'|'output'|'plugins'|'skipAudits']
  • if we pretend the type is string[] because yargs is returning that for the subset of keys we care about, assigning back as string[] is still not correct even for that subset because e.g. output is LH.OutputMode[], not string[] (and that's ignoring trying to reassign back to LH.CliFlags[keyof LH.CliFlags]). This will be an error in tsc 3.5+.

I think the most honest thing is to just leave as any internal to getFlags with a note about why and leave it up to tests to verify that getFlags really is returning LH.CliFlags.

const argv = y.help('help')
brendankenny marked this conversation as resolved.
Show resolved Hide resolved
.version(() => pkg.version)
.showHelpOnFail(false, 'Specify --help for available options')

Expand Down Expand Up @@ -171,6 +181,24 @@ function getFlags(manualArgv) {
'For more information on Lighthouse, see https://developers.google.com/web/tools/lighthouse/.')
.wrap(yargs.terminalWidth())
.argv;

// ".middleware" does not exist in this version of yargs, so do some post-processing here.
connorjclark marked this conversation as resolved.
Show resolved Hide resolved
/** @type {(keyof LH.CliFlags)[]} */
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we're settling on Array<keyof LH.CliFlags> for complex types that aren't just string[] or w/e, though if you ask @brendankenny we're always settling on Array<*> ;)

Copy link
Member

@brendankenny brendankenny May 15, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

though if you ask @brendankenny we're always settling on Array<*> ;)

well, yes, because it's an array, not a string, so why would you put that information last? :P They could have gone [string], but they used that up on tuples

const arrayKeysThatSupportCsv = [
'onlyAudits',
'onlyCategories',
'output',
'plugins',
'skipAudits',
];
arrayKeysThatSupportCsv.forEach(key => {
const input = /** @type {string[]} */ (argv[key]);
if (input) {
argv[key] = flatten(input.map(value => value.split(',')));
brendankenny marked this conversation as resolved.
Show resolved Hide resolved
}
});

return argv;
}

module.exports = {
Expand Down
23 changes: 23 additions & 0 deletions lighthouse-cli/test/cli/cli-flags-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,4 +28,27 @@ describe('CLI bin', function() {
assert.ok(optionsWithDescriptions.includes(opt), `cli option '${opt}' has no description`);
});
});

it('array values support csv when appropriate', () => {
const flags = getFlags(
Copy link
Collaborator

Choose a reason for hiding this comment

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

once we figure out other feedback would love the --window-size and blockedUrlPattern counter examples here too :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

blockedUrlPattern just b/c comma could be in a pattern? what if we could escape it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

nvm lets not do that :)

'http://www.example.com',
'--only-categories=performance,seo',
'--skipAudits=unused-javascript,redirects',
'--skipAudits=bootup-time');
expect(flags.onlyCategories).toEqual(['performance', 'seo']);
expect(flags.skipAudits).toEqual(['unused-javascript', 'redirects', 'bootup-time']);
});

it('array values do not support csv when appropriate', () => {
const flags = getFlags(
'http://www.example.com',
'--chrome-flags="--window-size 800,600"',
'--chrome-flags="--enabled-features=NetworkService,VirtualTime"',
'--blockedUrlPatterns=.*x,y\\.png');
expect(flags.chromeFlags).toEqual([
'"--window-size 800,600"',
'"--enabled-features=NetworkService,VirtualTime"',
]);
expect(flags.blockedUrlPatterns).toEqual(['.*x,y\\.png']);
});
});