I’ve never in my life submitted a “Pull Request” on GitHub. I wanted to give it a shot, so this video is capturing the moment of me attempting to figure it out. Fair warning, this isn’t a succinct, quick tutorial. This is me thinking to myself and struggling through it all.
My idea was that I wanted to put FitVids.js onto cdnjs so people can link it up through there. Their process for doing that is by forking their GitHub repo, follow their instructions for adding a new library, and submit a pull request.
So we do it!
Things I struggled with:
- I didn’t understand if a pull request was a “Git” thing or a “GitHub” thing. I’m still not 100% sure but it seems like it’s a GitHub thing, as we did it 100% through GitHub.com.
- I didn’t understand what exactly composes a “pull request” – I thought it would be more like you move a specific commit over to another repo. We learned that it’s actually the entire repo that you move together (all commits made since your fork).
- One of their requirements was that it pass the “npm test” thing locally. It didn’t seem to pass that even with an unchanged fork of the repo. Ultimately I did screw up the first pull request also with an incorrect file name. This failed the “Travis build” on GitHub.com, which I also don’t understand (is it the same as the npm test?), but after fixing the file name the second request worked.
As a follow up, the owners of the cdnjs repo asked that I include the non-minified version in the repo and add a “tag” to the FitVids repo with the correct versions. So I had to learn another brand new thing, but ultimately got it done.
You shouldn’t edit the current branch. You can mix things up if you do every pull request in the current branch.
What you should do is creating a new branch like
git checkout -b adding_fitvids) and do your commits in that branch. Before pushing to github, it is usefull to do fetch the upstream branch and see if things are changed. If so, you need to rebase your branch to be up to date with the upstream branch. After that, you push to github and create a new Pull request.
In code it become:
Good to see more people getting into Git and GitHub.
Pull requests are a GitHub thing however most repository hosts with a GUI will use their own terminology for it.
Also when you submit your new branch with the changes to your fork of the repository, a button should appear in the base of the repository (at the directory viewer) but the way you did it is also acceptable.
The idea of pull requests is that you are requesting a set of changes via commits that you would like to have changed so using the branching model mentioned above makes more sense to use.
“There’s only one way to learn this stuff, and it’s by screwing it up, getting yelled at by someone, then you fix it. Then you see someone else being dumb and you yell at them. And that’s how this works”
This needs to be a poster.
Hahaha – that’s right though isn’t it?
I would order that poster right now… Brilliant!
I second this! It’s so true hahaha
Hi Chris, i’ve noticed that you use a nice theme for your sublime editor with a cool animation of open/close folder on the sidebar. Would you be nice to tell me the name of the theme??
Hi Frank – The theme comes from here:
I use it and really like it, too. I’m not sure if the animation is part of it or not, although mine does animate as well, so it must be.
Thanks a lot Matt. I ve installed the theme and it works great very helpfull for dev workflow
Hey Chris, “npm test” failed because the dependencies for the test weren’t installed (vows). The package.json (for the tests) lists those dependencies. However, they’re not automatically installed. You’d have to shell to the tests dir and do an “npm install” first. – provided you have Node.js and npm installed.
You have to do this because dependencies aren’t normally packaged with the rest of the repo because of versioning and repo bloating.
The whole tests thing is a great tool, but not easy or useful unless you’re a Node.js developer. Should send cdnjs a pull request to add documentation for running tests.
A pull request in itself is not a hard thing, as you have seen. Not in GitHub at least. Maybe this case with cdnjs was kinda tricky, but in general, it’s not hard to achieve.
Good job, Chris.
Node has 2 different types of dependencies: “production” and “dev”. To run
npm testyou want to install the dev dependencies with
npm install --dev(which also installs prod dependencies btw).
I actually got an error while installing modules from npm for cdnjs, but
npm testran ok, so I’m calling that good.
It seems like others have explained your other troubles. Don’t get discouraged, everyone was where you are now.
This will come in very handy when I finally get around to doing a pull request. I like watching your videos because they aren’t fast and cut to perfection, we actually get to see your thought processes behind a new thing, which I think is much more helpful even if sometimes it does leave us yelling at the screen! That said it’s easy to say that as a third party, it’s a lot harder to figure stuff out by yourself the first time. Great job, thanks Chris!
Chris, this was awesomely helpful. I’m still somewhat new to git and have been slowly introducing my team to all its particulars. Keep the videos coming!
Hey Chris, Just watched the video and wanted to give you some Sublime Text 2 tips! There is a great package for Sublime Text called “SideBarEnhancements”. It allows you to right click and give you the option to rename the file. It also has a ton of other different features. You can find it here
Another great package I found that might help you out is a JS minifier. I’m sure you take care of minifying your JS through WordPress but if you wanted to do it right through Sublime Text you could install this package..
There definitely needs to be more git video like this out there. Thanks for sharing, Chris!
Looks like you made it in.
npm install -g vows should allow your npm test to pass.
vows is a testing framework for nodejs
The -g argument allows you to install it “globally” allowing you to type ‘vows [arguments]’ at the shell.