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

Default the prune option to true #472

Merged
merged 4 commits into from
Aug 29, 2016
Merged

Conversation

malept
Copy link
Member

@malept malept commented Aug 26, 2016

Have you read the section in CONTRIBUTING.md about pull requests?

Yes

Summarize your changes:

In both the CLI and API, prune defaults to true (the CLI is just more explicit about it in the code).

@zeke I know you assigned yourself to #35 but I had some time the other day to work on this.

Addresses #35. (Obviously, not the asar part.)
Fixes #235.

Are your changes appropriately documented?

The CLI docs now mentions --no-prune instead of --prune. Also updated NEWS, the readme, and the API docs.

Do your changes have sufficient test coverage?

Updated the defaults test, added a test for prune=false.

Does the testsuite pass successfully on your local machine?

Yes

TODO

  • truthy or undefined (ref)
  • --no-prune should go after ignore now to keep things alphabetical (ref)

@malept malept added this to the 8.0.0 milestone Aug 26, 2016
@@ -194,7 +195,7 @@ module.exports = {
// * Creates temporary directory
// * Copies template into temporary directory
// * Copies user's app into temporary directory
// * Prunes non-production node_modules (if opts.prune is set)
// * Prunes non-production node_modules (if opts.prune is not false)
Copy link
Contributor

Choose a reason for hiding this comment

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

truthy?

Copy link
Member Author

Choose a reason for hiding this comment

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

Pedantically speaking, either truthy or undefined.

@@ -48,7 +48,7 @@ overwrite if output directory for a platform already exists, replaces i
skipping it
platform all, or one or more of: darwin, linux, mas, win32 (comma-delimited if multiple).
Defaults to the host platform
prune runs `npm prune --production` on the app
no-prune do not run `npm prune --production` on the app
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you think this should this go after ignore now to keep things alphabetical?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep!

@zeke
Copy link
Contributor

zeke commented Aug 29, 2016

How come CI is not running on this PR?

@malept
Copy link
Member Author

malept commented Aug 29, 2016

It did in the previous commit (note the checkmark), I skipped it in the latest one because it was just a doc change. That's some interesting UX implications in the GitHub PR UI 😃

@zeke
Copy link
Contributor

zeke commented Aug 29, 2016

Cool. Is this ready to ship, then?

@malept
Copy link
Member Author

malept commented Aug 29, 2016

Yup!

@malept malept merged commit 53bbb83 into electron:master Aug 29, 2016
@malept malept deleted the default-prune branch August 29, 2016 20:38
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.

3 participants