Skip to content

Adds .gitignore file to default template - #79

Merged
vjeux merged 4 commits into
react:masterfrom
npverni:add-gitignore-to-template
Jul 22, 2016
Merged

Adds .gitignore file to default template#79
vjeux merged 4 commits into
react:masterfrom
npverni:add-gitignore-to-template

Conversation

@npverni

@npverninpverni commented Jul 22, 2016

Copy link
Copy Markdown
Contributor

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

@ghost

ghost commented Jul 22, 2016

Copy link
Copy Markdown

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 cla@fb.com. Thanks!

@npverni

npverni commented Jul 22, 2016

Copy link
Copy Markdown
ContributorAuthor

done

@ghostghost added the CLA Signed label Jul 22, 2016
@gaearon

gaearon commented Jul 22, 2016

Copy link
Copy Markdown
Contributor

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

@ghost

ghost commented Jul 22, 2016

Copy link
Copy Markdown

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

@ghostghost added the CLA Signed label Jul 22, 2016
@npverni

npverni commented Jul 22, 2016•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

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

@vjeux

vjeux commented Jul 22, 2016

Copy link
Copy Markdown
Contributor

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

@jaredpalmer

jaredpalmer commented Jul 22, 2016•
edited
Loading

Copy link
Copy Markdown
Contributor

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

@ghostghost added the CLA Signed label Jul 22, 2016
@npverni

npverni commented Jul 22, 2016

Copy link
Copy Markdown
ContributorAuthor

Fix coming.

@npverni
npverniforce-pushed the add-gitignore-to-template branch from 905a696 to 5330a29CompareJuly 22, 2016 18:12
@npverni
npverniforce-pushed the add-gitignore-to-template branch from 5330a29 to e7642aeCompareJuly 22, 2016 18:13
@npverni

npverni commented Jul 22, 2016

Copy link
Copy Markdown
ContributorAuthor

This is working now. Ready for review. Thanks.

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

@ghostghost added the CLA Signed label Jul 22, 2016
@eanplatter

eanplatter commented Jul 22, 2016

Copy link
Copy Markdown
Contributor

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

vjeux
vjeux reviewed Jul 22, 2016
View reviewed changes
Comment threadscripts/init.js Outdated

// Rename files
[
['gitignore', '.gitignore'],

@vjeuxvjeuxJul 22, 2016•
edited
Loading

Copy link
Copy Markdown
Contributor

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

@npverninpverniJul 22, 2016

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Done

gaearon
gaearon reviewed Jul 22, 2016
View reviewed changes
Comment threadscripts/init.js
fs.copySync(path.join(selfPath, 'template'), hostPath);

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

@gaearongaearonJul 22, 2016

Copy link
Copy Markdown
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

@npverninpverniJul 22, 2016

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Good call 👍

@npverni

npverni commented Jul 22, 2016

Copy link
Copy Markdown
ContributorAuthor

Simplified the code and responded to questions.

vjeux
vjeux reviewed Jul 22, 2016
View reviewed changes
Comment threadtemplate/gitignore Outdated
# See http://help.github.com/ignore-files/ for more about ignoring files.

# dependencies
/node_modules

@vjeuxvjeuxJul 22, 2016

Copy link
Copy Markdown
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

vjeux commented Jul 22, 2016

Copy link
Copy Markdown
Contributor

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

@npverni

npverni commented Jul 22, 2016

Copy link
Copy Markdown
ContributorAuthor

All set

@vjeux
vjeux merged commit e395498 into react:masterJul 22, 2016
@vjeux

vjeux commented Jul 22, 2016

Copy link
Copy Markdown
Contributor

Thanks!

@chibicodechibicode mentioned this pull request Jul 22, 2016
Merged
@gaearongaearon added this to the 0.2.0 milestone Jul 25, 2016
@gaearongaearon mentioned this pull request Jul 27, 2016
Merged
nwolverson pushed a commit to nwolverson/create-react-app that referenced this pull request Jun 29, 2017
@wmonk
Merge pull requestreact#79from zinserjan/fix-jest-caching
0b2821f
Jest cache busting for ts/tsx files
@locklockBot locked and limited conversation to collaborators Jan 22, 2019
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Assignees

No one assigned

Labels

CLA Signed

Projects

None yet

Milestone

0.2.0

Development

Successfully merging this pull request may close these issues.

5 participants

@npverni@gaearon@vjeux@jaredpalmer@eanplatter