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

cache commands, --no-exit #1875

Merged
merged 4 commits into from
Jun 5, 2018
Merged

cache commands, --no-exit #1875

merged 4 commits into from
Jun 5, 2018

Conversation

kuceb
Copy link
Contributor

@kuceb kuceb commented Jun 4, 2018

close #1856 cypress cache <command>
close #1871 --no-exit

cli/lib/cli.js Outdated
program
.command('cache')
.usage('[command]')
.description('Manage the Cypress binary cache')
Copy link
Member

Choose a reason for hiding this comment

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

Can you update 'Manage' to 'Manages' to keep in line with other commands having plural verb descriptors, ie: 'Opens ...', 'Installs ...', 'Verifies ...'?

@jennifer-shehane jennifer-shehane requested a review from bahmutov June 5, 2018 14:53
@jennifer-shehane
Copy link
Member

Does this also close #817 about --silent flag ?

@kuceb
Copy link
Contributor Author

kuceb commented Jun 5, 2018

Does this also close #817 about --silent flag ?

no, #1855 fixes that

@@ -105,6 +106,7 @@ exports['cli help command shows help 1'] = `
open [options] Opens Cypress in the interactive GUI.
install [options] Installs the Cypress executable matching this package's version
verify Verifies that Cypress is installed correctly and executable
cache [options] Manage the Cypress binary cache
Copy link
Contributor Author

@kuceb kuceb Jun 4, 2018

Choose a reason for hiding this comment

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

optimally this should say cache <command>

@jennifer-shehane
Copy link
Member

I'm confused why there is code in like if (logLevel() === 'silent') in both PRs, is one PR dependent on the other?

@kuceb
Copy link
Contributor Author

kuceb commented Jun 5, 2018

@jennifer-shehane whoops, forgot to remove that commit

@kuceb
Copy link
Contributor Author

kuceb commented Jun 5, 2018

@jennifer-shehane fixed

@@ -74,6 +74,10 @@ const processRunOptions = (options = {}) => {
args.push('--headed', options.headed)
}

if (options.noExit) {
args.push('--no-exit')
Copy link
Member

Choose a reason for hiding this comment

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

There are no tests for this, and the logic is broken because you're not testing options.exit === false in here

cli/lib/cli.js Outdated
.option('--dev', text('dev'), coerceFalse)
.action((opts) => {
// console.log(opts)
Copy link
Member

Choose a reason for hiding this comment

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

remove this line

cli/lib/cli.js Outdated
unknownOption.call(this, `cache ${opts}`, 'sub-command')
}
cache[opts]()
// this.outputHelp()
Copy link
Member

Choose a reason for hiding this comment

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

remove this line

@brian-mann brian-mann merged commit 56e0098 into develop Jun 5, 2018
@jennifer-shehane jennifer-shehane deleted the issue-1856 branch July 24, 2018 16:56
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.

Support cypress run --no-exit Add cli commands for managing binary cache
3 participants