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

Adds .gitignore file to default template #79

Merged
merged 4 commits into from
Jul 22, 2016

Conversation

npverni
Copy link
Contributor

@npverni npverni commented Jul 22, 2016

Might be nice to give the users a .gitignore file to start with ala rails, ember, etc.

@ghost
Copy link

ghost commented Jul 22, 2016

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at [email protected]. Thanks!

@npverni
Copy link
Contributor Author

npverni commented Jul 22, 2016

done

@ghost ghost added the CLA Signed label Jul 22, 2016
@gaearon
Copy link
Contributor

gaearon commented Jul 22, 2016

Looks reasonable to me. @vjeux What do you think? Do/should we do this in RN too?

@ghost
Copy link

ghost commented Jul 22, 2016

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@ghost ghost added the CLA Signed label Jul 22, 2016
@npverni
Copy link
Contributor Author

npverni commented Jul 22, 2016

Testing this a bit more, the .gitignore file is ending up as .npmignore in the created project due to npm/npm#1862

@vjeux
Copy link
Contributor

vjeux commented Jul 22, 2016

This was something in my mental todo list. Happy with the concept (haven't reviewed the pull request yet)

@jaredpalmer
Copy link
Contributor

jaredpalmer commented Jul 22, 2016

@ryanflorence did this in the react-project repo to handle this issue.

@ghost ghost added the CLA Signed label Jul 22, 2016
@npverni
Copy link
Contributor Author

npverni commented Jul 22, 2016

Fix coming.

@npverni npverni force-pushed the add-gitignore-to-template branch from 905a696 to 5330a29 Compare July 22, 2016 18:12
@npverni npverni force-pushed the add-gitignore-to-template branch from 5330a29 to e7642ae Compare July 22, 2016 18:13
@npverni
Copy link
Contributor Author

npverni commented Jul 22, 2016

This is working now. Ready for review. Thanks.

screen shot 2016-07-22 at 2 08 21 pm

@ghost ghost added the CLA Signed label Jul 22, 2016
@eanplatter
Copy link
Contributor

Is there a specific advantage to programmatically creating the .gitignore file rather than just adding it to the template directory?

@@ -37,6 +37,13 @@ module.exports = function(hostPath, appName, verbose) {
// Copy the files for the user
fs.copySync(path.join(selfPath, 'template'), hostPath);

// Rename files
[
['gitignore', '.gitignore'],
Copy link
Contributor

@vjeux vjeux Jul 22, 2016

Choose a reason for hiding this comment

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

Please don't over abstract things. There's only one case right now so you can just write

fs.move(path.join(hostPath, 'gitignore'), path.join(hostPath, '.gitignore'));

which is going to be clearer

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

@@ -37,6 +37,9 @@ module.exports = function(hostPath, appName, verbose) {
// Copy the files for the user
fs.copySync(path.join(selfPath, 'template'), hostPath);

// Rename files
fs.move(path.join(hostPath, 'gitignore'), path.join(hostPath, '.gitignore'), []);
Copy link
Contributor

Choose a reason for hiding this comment

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

Would be good to link to npm issue here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call 👍

@npverni
Copy link
Contributor Author

npverni commented Jul 22, 2016

Simplified the code and responded to questions.

# See http://help.github.com/ignore-files/ for more about ignoring files.

# dependencies
/node_modules
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you remove the leading /, it looks like you're not putting it for npm-debug.log

@vjeux
Copy link
Contributor

vjeux commented Jul 22, 2016

Arg, okay so if you remove the / I'll merge it in, thanks!

@npverni
Copy link
Contributor Author

npverni commented Jul 22, 2016

All set

@vjeux vjeux merged commit e395498 into facebook:master Jul 22, 2016
@vjeux
Copy link
Contributor

vjeux commented Jul 22, 2016

Thanks!

@gaearon gaearon added this to the 0.2.0 milestone Jul 25, 2016
@gaearon gaearon mentioned this pull request Jul 27, 2016
nwolverson pushed a commit to nwolverson/create-react-app that referenced this pull request Jun 29, 2017
@lock lock bot locked and limited conversation to collaborators Jan 22, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants