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

Add gitignore and npmignore #58

Closed
wants to merge 1 commit into from
Closed

Conversation

brettz9
Copy link
Contributor

@brettz9 brettz9 commented Nov 26, 2019

No description provided.

@kemitchell
Copy link
Member

@brettz9 if you do Node/JavaScript development, I'd strongly recommend you put user_modules in user-global .gitignore.

This package probably should exclude tests from distribution tarballs. Would you like to land that PR yourself, or should I go ahead and do it?

@brettz9
Copy link
Contributor Author

brettz9 commented Nov 26, 2019

Sorry, are you asking to just add user_modules (along with node_modules) in .gitignore?

I am behind the Great Firewall in China, and can't get around it atm, so not able to Google user_modules, but thanks for the tip.

@kemitchell
Copy link
Member

I do:

echo >> ~/.gitconfig <<EOS
[core]
excludes_file = ~/.gitignore_global
EOS
echo node_modules >> ~/.gitignore_global

@ljharb
Copy link
Member

ljharb commented Nov 26, 2019

I’m not familiar with user_modules; node_modules should be gitignored and i believe needn’t be npmignored since npm will always ignore it unless bundledDependencies are specified.

@kemitchell
Copy link
Member

user_modules was a typo. Might've been my phone.

@brettz9
Copy link
Contributor Author

brettz9 commented Nov 26, 2019

@kemitchell : Re: global gitignore, while I understand the appeal, I think the advantage of a local one is consistency among project developers. Of the good many projects I've submitted PRs for, I've come across quite a few who have strong opinions about whether to include or exclude package-lock.json and such, but I don't think I've come across any others who expected a .gitignore_global. But if you don't want it, I can amend to only include the .npmignore in this PR.

@kemitchell
Copy link
Member

@brettz9 I don't think we need an .npmignore. The package.json file for this package uses a files array. And indeed there aren't any test files in the latest tarball:

mkdir tmp
cd tmp
npm i -D licensee
ls node_modules/licensee

I hear you on .gitignore. The argument that got me years ago boils down to this: Should nearly npm package project have node_modules in .gitignore? If so, and I think so, then node_modules should be ignored by default, rather than cluttering every repository with the same .gitignore.

@kemitchell kemitchell closed this Nov 26, 2019
@brettz9
Copy link
Contributor Author

brettz9 commented Nov 27, 2019

Re: npmignore, yeah, sorry, I seem to have a mental block in forgetting to check files.

Re: gitignore, assuming Git, which is of course not Node specific and has no mechanism for detecting the language, would agree to such a default, it could still cause problems with those using bundledDependencies who actually include node_modules in their repositories.

@kemitchell
Copy link
Member

git add --force node_modules overrides the default.

@brettz9
Copy link
Contributor Author

brettz9 commented Nov 27, 2019

So where is your proposal to Git to allow node_modules by default, so we can all benefit from this? :-)

@brettz9
Copy link
Contributor Author

brettz9 commented Nov 27, 2019

I guess it is a reasonable practice and even if it causes me to forget adding it where explicit .gitignore files are needed, I can justify it, since it is really hardly project-specific for Node users. Thanks!

lencioni added a commit to lencioni/licensee.js that referenced this pull request Oct 26, 2022
These files materialize after running `npm install`. Adding them to
`.gitignore` to reduce friction for developers and to help ensure that
they are not accidentally committed.

I found this discussion from a few years ago that seems relevant:

jslicense#58 (comment)

While I agree with the sentiment of something so universal being a
ignored by default globally, since it is not the default git behavior in
practice it makes it more difficult for people to contribute to this
repo. The cost of adding these lines to `.gitignore` is low, so this
seems like a reasonable change to make.
lencioni added a commit to lencioni/licensee.js that referenced this pull request Oct 26, 2022
These files materialize after running `npm install`. Adding them to
`.gitignore` to reduce friction for developers and to help ensure that
they are not accidentally committed.

I found this discussion from a few years ago that seems relevant:

jslicense#58 (comment)

While I agree with the sentiment of something so universal being a
ignored by default globally, since it is not the default git behavior in
practice it makes it more difficult for people to contribute to this
repo. The cost of adding these lines to `.gitignore` is low, so this
seems like a reasonable change to make.

I was surprised that this was not already the case, and since this seems
to be a common issue in this repo, it seems that other developers are
also surprised by this.

- jslicense#23
- jslicense#26
lencioni added a commit to lencioni/licensee.js that referenced this pull request Oct 26, 2022
These files materialize after running `npm install` and `npm run test`.
Adding them to `.gitignore` to reduce friction for developers and to
help ensure that they are not accidentally committed.

I found this discussion from a few years ago that seems relevant:

jslicense#58 (comment)

While I agree with the sentiment of something so universal being a
ignored by default globally, since it is not the default git behavior in
practice it makes it more difficult for people to contribute to this
repo. The cost of adding these lines to `.gitignore` is low, so this
seems like a reasonable change to make.

I was surprised that this was not already the case, and since this seems
to be a common issue in this repo, it seems that other developers are
also surprised by this.

- jslicense#23
- jslicense#26
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