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

doc(cli): list valid categories #9399

Merged
merged 2 commits into from
Jul 31, 2019

Conversation

olore
Copy link
Contributor

@olore olore commented Jul 18, 2019

Summary
Small doc tweak to fix #9209 by listing the valid categories in an example

readme.md Outdated
@@ -113,6 +113,8 @@ Examples:
lighthouse <url> --quiet --chrome-flags="--headless" Launch Headless Chrome, turn off logging
lighthouse <url> --extra-headers "{\"Cookie\":\"monster=blue\"}" Stringify\'d JSON HTTP Header key/value pairs to send in requests
lighthouse <url> --extra-headers=./path/to/file.json Path to JSON file of HTTP Header key/value pairs to send in requests
lighthouse <url> --only-categories=performance,pwa Only run specific categories. Valid categories: accessibility,
best-practices, performance, pwa.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
best-practices, performance, pwa.
best-practices, seo, performance, pwa.

Copy link
Contributor Author

@olore olore Jul 18, 2019

Choose a reason for hiding this comment

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

Thank you, I totally overlooked "seo"!

Is there a particular reason for your suggested rder, or is it ok if I maintain alphabetical order?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added "seo" in alphabetical order.

Copy link
Collaborator

@connorjclark connorjclark Jul 18, 2019

Choose a reason for hiding this comment

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

alphabetical is fine. the only other acceptable order would be the order in which the categories are rendered, but that isn't appropriate here.

Copy link
Collaborator

@patrickhulce patrickhulce left a comment

Choose a reason for hiding this comment

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

thanks for jumping in on this @olore!

readme.md Outdated
@@ -113,6 +113,8 @@ Examples:
lighthouse <url> --quiet --chrome-flags="--headless" Launch Headless Chrome, turn off logging
lighthouse <url> --extra-headers "{\"Cookie\":\"monster=blue\"}" Stringify\'d JSON HTTP Header key/value pairs to send in requests
lighthouse <url> --extra-headers=./path/to/file.json Path to JSON file of HTTP Header key/value pairs to send in requests
lighthouse <url> --only-categories=performance,pwa Only run specific categories. Valid categories: accessibility,
Copy link
Collaborator

Choose a reason for hiding this comment

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

this is periodically updated from the output of lighthouse --help do you think you could add the description to the actual cli-flags.js file itself? :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Sorry I should have figured that out. Thanks for your patience.

Copy link
Collaborator

@patrickhulce patrickhulce left a comment

Choose a reason for hiding this comment

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

thanks very much @olore!

this LGTM but a few suggestions I'd want your thoughts on first

@@ -129,7 +129,7 @@ function getFlags(manualArgv) {
'precomputed-lantern-data-path': 'Path to the file where lantern simulation data should be read from, overwriting the lantern observed estimates for RTT and server latency.',
'lantern-data-output-path': 'Path to the file where lantern simulation data should be written to, can be used in a future run with the `precomputed-lantern-data-path` flag.',
'only-audits': 'Only run the specified audits',
'only-categories': 'Only run the specified categories',
'only-categories': 'Only run the specified categories. Valid categories: accessibility, best-practices, performance, pwa, seo',
Copy link
Collaborator

Choose a reason for hiding this comment

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

the only tweak I might make is

Suggested change
'only-categories': 'Only run the specified categories. Valid categories: accessibility, best-practices, performance, pwa, seo',
'only-categories': 'Only run the specified categories. Available categories: accessibility, best-practices, performance, pwa, seo',

Copy link
Collaborator

Choose a reason for hiding this comment

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

if the user has a config that adds other categories those will be valid too, so the slightly looser Available makes sense to me too

Copy link
Collaborator

Choose a reason for hiding this comment

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

also, sorry the example snippet you edited in the README comes from line 61 of this file if you'd want to update that description too :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yup, good call. I like this better

@olore olore force-pushed the readme-list-categories branch from 1b5342a to b67a40f Compare July 28, 2019 00:23
@brendankenny brendankenny merged commit bb26780 into GoogleChrome:master Jul 31, 2019
@olore olore deleted the readme-list-categories branch August 1, 2019 11:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CLI only-categories documentation
5 participants