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

Use installing package manager in README #7687

Merged
merged 3 commits into from Oct 2, 2019
Merged

Conversation

ashr81
Copy link
Contributor

@ashr81 ashr81 commented Sep 16, 2019

Fixes #7411

Replaces npm commands to yarn commands if the user uses yarn

@facebook-github-bot
Copy link

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. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

@facebook-github-bot
Copy link

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

// modifies README.md commands based on user used package manager.
if (useYarn) {
try {
const data = fs.readFileSync(path.join(appPath, 'README.md'), 'utf8');
Copy link
Contributor

Choose a reason for hiding this comment

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

Rename data to readme and modify that directly instead of creating another formatted variable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@iansu I renamed variable to the readme and modifying directly. I used variable naming reference from the same file.

@iansu iansu added this to the 3.1.3 milestone Sep 26, 2019
@iansu
Copy link
Contributor

iansu commented Sep 26, 2019

This is great. Just one small change from me and then I think we can merge it. @mrmckeb has been doing some work on this part of the code. Any concerns?

@ashr81
Copy link
Contributor Author

ashr81 commented Sep 27, 2019

@iansu I saw the PR raised by him. I have no concerns. Let me know if you want something to be done from the end.

@iansu iansu self-assigned this Sep 27, 2019
@ashr81 ashr81 requested a review from iansu September 28, 2019 06:10
@ashr81
Copy link
Contributor Author

ashr81 commented Oct 1, 2019

@iansu can you please review this PR. Is the failing test cases stopping this from getting merged? Or do you have any concerns on the PR?

@mrmckeb
Copy link
Contributor

mrmckeb commented Oct 1, 2019

I think this won't cause any issues for now, I'll need to do a rebase before we move forward on templates anyway.

@mrmckeb mrmckeb added this to In progress in v3.3 via automation Oct 1, 2019
@mrmckeb mrmckeb modified the milestones: 3.1.3, 3.2 Oct 1, 2019
@ashr81
Copy link
Contributor Author

ashr81 commented Oct 2, 2019

@mrmckeb cool thanks. If changing the base branch to feature/templates helps you, which has your template changes. Go ahead and change the base branch. I will resolve the conflicts if any arises.

@mrmckeb mrmckeb closed this Oct 2, 2019
v3.3 automation moved this from In progress to Done Oct 2, 2019
@mrmckeb
Copy link
Contributor

mrmckeb commented Oct 2, 2019

Restarting CI

@mrmckeb mrmckeb reopened this Oct 2, 2019
v3.3 automation moved this from Done to In progress Oct 2, 2019
@mrmckeb mrmckeb changed the title updates readme based on package manager used by user. Use installing package manager in README Oct 2, 2019
@mrmckeb mrmckeb merged commit 6b8fa00 into facebook:master Oct 2, 2019
v3.3 automation moved this from In progress to Done Oct 2, 2019
@mrmckeb
Copy link
Contributor

mrmckeb commented Oct 2, 2019

Thank you!

@iansu iansu modified the milestones: 3.3, 3.2 Oct 2, 2019
@lock lock bot locked and limited conversation to collaborators Oct 7, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
No open projects
v3.3
  
Done
Development

Successfully merging this pull request may close these issues.

Generated README containing npm examples is confusing, should be changed to yarn
4 participants