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

README: update CLI help with latest. Give common examples #2182

Merged
merged 2 commits into from
May 8, 2017
Merged

Conversation

ebidel
Copy link
Contributor

@ebidel ebidel commented May 8, 2017

  • Provide usage examples in CLI --help
  • Update README with latest output from --help
  • Also changed additionalFlags -> chromeFlags so we're consistent. It's confusing that we use --chrome-flags in some places (LH CLI) and additionalFlags in other places (chrome-debug, ChromeLauncher). --chrome-flags better signals that these flags are Chrome specific and not Lighthouse flags.

@paulirish
Copy link
Member

it's super tall:
image

can you add this guy into the yargs chain?

.wrap(yargs.terminalWidth())

image

@@ -52,6 +52,12 @@ const cliFlags = yargs
.showHelpOnFail(false, 'Specify --help for available options')

.usage('$0 url')
.example('$0 url --view', 'Opens the HTML report in a browser after the run completes')
Copy link
Member

Choose a reason for hiding this comment

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

IMO we should just use "lighthouse" rather than whatever $0 resolves to.

how about with brackets on the url too: lighthouse <url> --view

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

@@ -209,7 +216,7 @@ function getDebuggableChrome(flags: {skipAutolaunch: boolean, port: number, sele
chromeFlags: string}): Promise<ChromeLauncher> {
const chromeLauncher = new ChromeLauncher({
port: flags.port,
additionalFlags: flags.chromeFlags.split(' '),
chromeFlags: flags.chromeFlags.split(' '),
Copy link
Member

Choose a reason for hiding this comment

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

this is a breaking change and some other folks rely on the API, but @samccone's work here will break them too and hopefully have some docs so its easy to do the right thing.

all that said, +1 to this change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea, I figure we're covered under the 2.0 veil

readme.md Outdated
--interactive Open Lighthouse in interactive mode [boolean]

Examples:
lighthouse-cli/index.js url --view Opens the HTML report in a browser
Copy link
Member

Choose a reason for hiding this comment

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

i'd repeat this paste with the new terminalWidth change, for less wrapping.

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

@ebidel
Copy link
Contributor Author

ebidel commented May 8, 2017

PTAL

@paulirish paulirish merged commit d926f32 into master May 8, 2017
@paulirish paulirish deleted the readme branch May 8, 2017 21:55
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.

None yet

2 participants