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 2 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
28 changes: 24 additions & 4 deletions lighthouse-cli/cli-flags.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,12 +12,21 @@ 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;
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

return y.help('help')
.version(() => pkg.version)
.showHelpOnFail(false, 'Specify --help for available options')
Expand Down Expand Up @@ -152,6 +161,17 @@ function getFlags(manualArgv) {
.default('enable-error-reporting', undefined) // Undefined so prompted by default
.default('channel', 'cli')
.check(/** @param {LH.CliFlags} argv */ (argv) => {
// ".middleware" does not exist in this version of yargs, so do some preprocessing here.
Copy link
Collaborator

Choose a reason for hiding this comment

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

still not 100% sure that I love mutating the arguments in a .check method

WDYT of just saving the .argv to a variable and mutating that before we return in getFlags?

I think that might help make it more explicit that we are doing some non-standard yargs mutations

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

for (const _key of Object.keys(argv)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't love that this is here and I'm not sure we can actually do this across the board.

(for example --chrome-flags="--window-size 800,600")

WDYT about just doing this to our specific .array properties minus blockedUrlPatterns?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ah ok. sounds like either way we need a hard coded list (either a blacklist of a whitelist). Whitelist is easier to reason about so let's do that :)

const key = /** @type {keyof argv} */ (_key);
const input = argv[key];
if (Array.isArray(input)) {
// These values are guarenteed to be strings.
const strings = /** @type {string[]} */ (input);
argv[key] = flatten(strings.map(value => value.split(',')));
}
}

// Lighthouse doesn't need a URL if...
// - We're just listing the available options.
// - We're just printing the config.
Expand Down
10 changes: 10 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,14 @@ describe('CLI bin', function() {
assert.ok(optionsWithDescriptions.includes(opt), `cli option '${opt}' has no description`);
});
});

it('array values support csv', () => {
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']);
});
});