Improve the setup to recognize our own repo before clearing it.

[Aftabnack]

Fix #760, #1719

React Boilerplate

Thank you for contributing! Please take a moment to review our contributing guidelines
to make the process easy and effective for everyone involved.

Please open an issue before embarking on any significant pull request, especially those that
add a new library or change existing tests, otherwise you risk spending a lot of time working
on something that might not end up being merged into the project.

Before opening a pull request, please ensure:

• You have followed our contributing guidelines
• double-check your branch is based on dev and targets dev
• Code is well-commented, linted and follows project conventions

[coveralls]

Coverage remained the same at 99.242% when pulling 6feee40 on Aftabnack:improve-setup into 37a0523 on react-boilerplate:dev.

[Aftabnack]

@gihrig Any comments on this?

[gihrig]

I'm not going to have time to put this through it paces anytime soon :-/

Maybe @KarandikarMihir or other community members can try it out.

[coveralls]

Coverage remained the same at 99.242% when pulling 59f6724 on Aftabnack:improve-setup into 5c495e7 on react-boilerplate:dev.

[coveralls]

Coverage remained the same at 99.242% when pulling b4a7234 on Aftabnack:improve-setup into ea1e1c4 on react-boilerplate:dev.

[coveralls]

Coverage remained the same at 99.242% when pulling b4a7234 on Aftabnack:improve-setup into ea1e1c4 on react-boilerplate:dev.

[mihir0x69]

Taking a look at this today!

[Aftabnack]

Finally!

[Aftabnack]

@KarandikarMihir I forgot. Here is a synopsis of what happens. Link

[mihir0x69]

@Aftabnack finally got a chance to test this! And it works really great! 😄 Thanks for submitting this PR! And sorry for the delay..

There's just one more thing that I'd like to have fixed. It could be outside of the scope of this PR. But maybe we can squeeze it in.

Can we get rid of the confusing logs in the terminal? If you see the screenshot, it kind of goes like this-

1. Cleaning old repo.
2. Lol, not really. (If there's a repo other than rbp)
3. Wait, now initializing a new repo!

@gihrig Do you have any thoughts on this?

[Aftabnack]

I will cleanup the console logs in another PR. Is that fine? @gihrig @KarandikarMihir

[Aftabnack]

Hey BTW, do you guys have slack integration with this repo? That would be really great.

[Aftabnack]

Never mind. It makes sense to be part of this PR only. I'm working on it now.

[coveralls]

Coverage remained the same at 99.242% when pulling eef7a61 on Aftabnack:improve-setup into 6ef43f5 on react-boilerplate:dev.

[coveralls]

Coverage remained the same at 99.242% when pulling eef7a61 on Aftabnack:improve-setup into 6ef43f5 on react-boilerplate:dev.

[coveralls]

Coverage remained the same at 99.242% when pulling eef7a61 on Aftabnack:improve-setup into 6ef43f5 on react-boilerplate:dev.

[gihrig]

@gihrig Do you have any thoughts on this?

Yes, personally I would like to never have the local repo removed automatically, but ask the user specifically only when it makes sense. My perspective is tainted by being a maintainer of repos, but that's why I never run setup and advocate for its removal as in this #760 comment. ;-)

If we're going to do this can we only prompt the user if it looks safe to delete the repo, and then ask "Delete git history and start fresh (Y/n)" ?

[coveralls]

Coverage remained the same at 99.242% when pulling c1cd73a on Aftabnack:improve-setup into 6ef43f5 on react-boilerplate:dev.

[Aftabnack]

Finished doing the changes for cleaning up the console messages.

It now shows these:

• No message if it doesn't find a git repo/git config file
• Removing old repository if it has a proper git config with only one URL pointing to rbp url.
• Leaving your repository untouched if it either finds other URLs or more than one URL (as is the case of the forks)

@gihrig I don't think we should prompt the user, since we know for sure that he will want to remove this repo info from his project. And if he somehow doesn't want it, he will want to keep it the way it is. So no point in asking.

• Clear the repo if only rbp link is present.
• Else leave it untouched.

Any thoughts?

Couple of issues I found from the last test to now.

• After showing the sequence of messages, it always ended in git init which is wrong. Fixed it.
• If it was not able to find .git or .git/config it directly went into catch, so used fs apis with callbacks to make it work properly.

[coveralls]

Coverage remained the same at 99.242% when pulling 2621bbf on Aftabnack:improve-setup into cfe0c7c on react-boilerplate:dev.

[Aftabnack]

Can someone give this a proper run this time. I believe the two things I mentioned above should have been captured before in the review.

[Aftabnack]

Scenarios I have tested

• No .git folder - No message about the repository. Starts with installing dependancies.
• No .git/config file - Same as above
• Config with rbp url - Removing old repository
• Config with more than one url (as is the case with forks) - Leaving your repository untouched
• Config with non rbp urls - Leaving your repository untouched

[gihrig]

@Aftabnack:

@gihrig I don't think we should prompt the user,

since we know for sure that he will want to remove this repo info from his project.

That is an assumption you just can't make, as you observed in #1719. That's how we got the present behavior.

And if he somehow doesn't want it, he will want to keep it the way it is. So no point in asking.

Did you mean "if he somehow doesn't want it removed..." ? But again how do we know this ?

I believe the fundamental issue here is that permanent action is being taken based on an assumption of what the end user will want after setup. That decision may not have been made by the end user at the time setup runs.

Action based on what the end user might want, is an impossible decision to make in an algorithm.

IMO the best we can do is to provide the end with info to make an informed choice and easily put that choice into action. That is, prompt the user to remove the repo when it might make sense and do nothing when it obviously does not.

[Aftabnack]

@gihrig You got me wrong. In #1719, I wanted it to leave my repository untouched.

So in essence what I wanted was, clear git only if the project is cloned from this repo. Otherwise leave it intact. And that is what I have achieved here.

[Aftabnack]

Apologies, I didn't get any time to work on this till now.
Added the prompt for the user.

@gihrig @KarandikarMihir Can you guys take a look at this now?

[coveralls]

Coverage remained the same at 99.242% when pulling 9132441 on Aftabnack:improve-setup into cfe0c7c on react-boilerplate:dev.

[coveralls]

Coverage remained the same at 99.242% when pulling 9cccc71 on Aftabnack:improve-setup into cfe0c7c on react-boilerplate:dev.

[Aftabnack]

No update on this? @gihrig @KarandikarMihir

[Aftabnack]

Is there any maintainer on this repo actually looking at PRs?

[justingreenberg]

this is awesome, thank you very much!

some minor nits:

• consistent spacing (add spaces after anonymous function declarations)
• single quotes (setup#L42)
• hoist complex "isCloned" condition into variable (setup#L38)
• prefer arrow functions for anonymous callbacks

please check out inline comments :)

[justingreenberg]

Using fs.stat() to check for the existence of a file before calling fs.open(), fs.readFile() or fs.writeFile() is not recommended. Instead, user code should open/read/write the file directly and handle the error raised if the file is not available

ref: https://nodejs.org/api/fs.html#fs_fs_stat_path_callback

eg....

function cleanRepo(callback) {
  fs.readFile('.git/config', 'utf8', (err, data) => {
    if (err) callback(err)

    let isClonedOrigin = typeof data === 'string'
      && data.match(/url\s*=/g).length  // note: no need for || []
      && /react-boilerplate\/react-boilerplate\.git/.test(data); 
    
    if (isClonedOrigin) {
      // ...
    }
    
    dontClearRepo('\n', callback);
  });
}

removing the need for nested if-else blocks will simplify control flow and make execution paths a little easier to grok :)

[Aftabnack]

• Initially I had checking for .git folder and config file because I was logging onto the console why I'm doing blind git init, I can remove that.
• About removing || [] is a foolproof way for me to ensure that it won't fail when there are no urls to be found in your config. It is a possible edge case. I don't think it should be removed

[coveralls]

Coverage remained the same at 99.242% when pulling c13851f on Aftabnack:improve-setup into cfe0c7c on react-boilerplate:dev.

[Aftabnack]

@justingreenberg Do you also want me to rebase & squash my commits?

[justingreenberg]

@Aftabnack no worries, will squash and merge... thank you for quick update! this will ship in v3.5

[Aftabnack]

Just a note. This closes #760 #1719

[mihir0x69]

@Aftabnack Apologies for not being available. Thanks for putting in the work! :) @gihrig Can you take a final look at this and "do the honors"? 😄

[mihir0x69]

@Aftabnack Just tested it out. And it works great! LGTM from me ✨

[coveralls]

Coverage remained the same at 99.231% when pulling 3c12e78 on Aftabnack:improve-setup into 88616ce on react-boilerplate:dev.

[gihrig]

@KarandikarMihir

@gihrig Can you take a final look at this and "do the honors"? 😄

I'm 'up to my eyeballs' in backend work lately, but if you and @justingreenberg approve, I'm sure it's good to go.

@Aftabnack Thanks for the contribution, I know it's been a long time. Thanks for hangin' in there! ✨

[Aftabnack]

Cool. Anything else I can work on?

[gihrig]

Anything else I can work on?

Look through open issues and PRs, find something that interests you and get involved. 😄

Especially anything related to the upcoming 3.5 release at #1776

[lock]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.