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

Fix npmignore #1251

Merged
merged 5 commits into from
Sep 18, 2017
Merged

Fix npmignore #1251

merged 5 commits into from
Sep 18, 2017

Conversation

maael
Copy link
Contributor

@maael maael commented Sep 18, 2017

This fixes the npmignore to also add a rule to exclude the children in js/ from being ignored, so that they will be added when npm packs the package.

.npmignore is based on the same rules as .gitignore, meaning !js/ will only exclude the immediate children in js/, not any others. This just adds a glob of !js/**/* to include the children, and the files needed for installing this package.

Fixes #1247.

This fixes the npmignore to also add a rule to exclude the children in
js/ from being ignored, so that they will be added when npm packs the
package.
@simondel
Copy link
Contributor

My bad sorry! Thanks for fixing it

@maael
Copy link
Contributor Author

maael commented Sep 18, 2017

No problem, these things happen. 👍

Copy link

@Kingwl Kingwl left a comment

Choose a reason for hiding this comment

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

LGTM

@ob1
Copy link

ob1 commented Sep 18, 2017

@simondel @bitwiseman To fix the internet (by merging this PR and republishing), this needs an approval review from someone with write access

@simondel
Copy link
Contributor

@ob1 @bitwiseman So, any idea who would have permissions to merge PR's?

@maael
Copy link
Contributor Author

maael commented Sep 18, 2017

@simondel from what I can see it looks like only @bitwiseman can merge (although I could be mistaken). If I'm right, this probably won't get looked at for a while at it's 5am in Seattle.

@andreineculau
Copy link

isn't this a pure liability - that in a project with 5k stars, which so many depend on, only one person has write access? I can literally feel how everyone is keeping their fingers crossed that @bitwiseman doesn't have a hangover :), or his broadband cut off, or, or, or.

https://felixge.de/2013/03/11/the-pull-request-hack.html

@irvingswiftj
Copy link

@andreineculau , you are right, its like the leftpad story, just by accident this time!

@stevenzeck
Copy link

stevenzeck commented Sep 18, 2017

Hmm, wonder how the Travis and Appveyor builds still completed without any errors. Is it a specific version of node and/or npm where it fails?

@evocateur
Copy link
Contributor

@bitwiseman: I locally bumped the package.json version to 1.7.1 and published it (with these changes) to unblock folks, so you'll need to update that in the source, too.

.npmignore Outdated
!js/
!js/**/*
!CONTRIBUTING.MD
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if we really need js/test/** included in the published artifact, but it only adds ~50kb so I didn't bother re-ignoring it.

@bitwiseman
Copy link
Member

@evocateur You beat me to publishing by about 1 minute. I've published 1.7.2 that also reverts.

@bitwiseman
Copy link
Member

@maael - Thanks for providing the fix. I've updated it with an additional admonition. 😄
@evocateur - Thanks for taking action to mitigate problem. I've added you as a contributor to the project

@shawninder
Copy link

@bitwiseman You da man!

@bitwiseman bitwiseman merged commit f573e28 into beautifier:master Sep 18, 2017
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.

Installing js-beautify fails