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

Setup script #160

Merged
merged 3 commits into from Jun 8, 2016
Merged

Setup script #160

merged 3 commits into from Jun 8, 2016

Conversation

gargrave
Copy link
Contributor

@gargrave gargrave commented Jun 7, 2016

Setup script to perform the following actions:

  • Install npm packages
  • Remove the original Git repo
  • Prompt the user to update package.json where appriopriate
  • Remove the tools/setup folder containing the script when finished

I did add two devDependencies in package.json: prompt and replace, for handling the package.json updates. If you would prefer that I not save them as dependencies, I could look at having the script just manually install then uninstall them.

Let me know if you have any questions or issues.

Thanks,
Gabe

@coveralls
Copy link

coveralls commented Jun 7, 2016

Coverage Status

Coverage remained the same at 98.904% when pulling 1023302 on gargrave:setup-script into 586c9c8 on coryhouse:master.

@coryhouse
Copy link
Owner

This is awesome! Brilliant work! 💯

I'm making a few tweaks and will get this merged today. Thanks so much! You nailed it! 👍

@coryhouse coryhouse merged commit c1aaba9 into coryhouse:master Jun 8, 2016
@coryhouse
Copy link
Owner

I made some tweaks:

  1. Converted to ES6
  2. Fixed linting issues
  3. Tweaked messaging to use chalkConfig

I'm torn on whether to remove the git repo. It actually bit me as I was trying your PR. I had made some tweaks and then wiping out the repo removed my changes.

I'm also trying to remove the keywords from package.json, but am struggling with the proper regex (not my strongsuit). Certainly open to a suggestion there!

Thanks again!

@gargrave
Copy link
Contributor Author

gargrave commented Jun 8, 2016

Yeah, I feel you on the Git issue--I can safely say I have never had to try to deal with writing a script that deletes Git and itself--it certainly made for an interesting time trying to keep it tracked. It seems like the only time it would really be an issue is for someone who is planning on contributing, and thus needs to keep the original repo--otherwise, I think it's a really handy feature to have in there. (Maybe just adding a cautionary note to the relevant line in the README file would be enough?)

Keywords: I started looking at that as well, and felt like it was probably more effort than it was worth in that case--mostly, like you said, because it doesn't use the same simple regex that the other prompts do. Are you thinking you would like to prompt the user for new keywords, or just clean it out and leave an empty array for the user to populate manually?

@coryhouse
Copy link
Owner

I was just going to clean out the existing keywords, or, if easier, remove the keywords section altogether.

@gargrave
Copy link
Contributor Author

gargrave commented Jun 8, 2016

Cool, that should be easy enough. I think I have it already, but I just need to test a full run of it when I have a minute. I'll send another PR later today.

@gargrave
Copy link
Contributor Author

gargrave commented Jun 8, 2016

Hey Cory,

One quick thing I noticed in trying to re-clone from scratch and run this. Maybe it's something wrong on my end, but when I am starting from a clean clone of the repo, the script won't run using babel-node. I ran into this before, and it's why I was originally sticking with mostly ES5 syntax that "vanilla Node" will run, since the npm script itself is actually launched before the babel tools are installed locally.

Am I maybe missing a global install that is causing this, or do we need to switch it back to the original ES5-ish syntax, since the assumption will be that anyone running this is starting from a clean clone with no packages installed?

I do fully admit that this could be user error on my part, but I just wanted to check before proceeding with anything.

Thanks again!

@coryhouse
Copy link
Owner

Ah! Great point. I didn't consider that. Since babel-node isn't installed until npm install is run, it indeed needs to be in ES5. If your next PR has it back in ES5, I'll just merge it in to complete the rollback of my mistake. 👍

@gargrave
Copy link
Contributor Author

gargrave commented Jun 8, 2016

Ok, cool. I will send a PR later on today. (Incidentally, this is the same reason I didn't use chalk in the initial setupMessage.js file.)

@coryhouse
Copy link
Owner

Yep, that makes sense.

I also realized we need a bit of validation on the project name. No spaces are allowed.

@gargrave
Copy link
Contributor Author

gargrave commented Jun 8, 2016

Yeah, I had considered that possibility as well. It's actually pretty trivial to add regex checking to user input with prompt, so it shouldn't be too big of a deal to enforce npm's naming conventions. Pretty confident I can work it in!

@coryhouse coryhouse mentioned this pull request Jun 28, 2016
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.

None yet

3 participants