Tip:
Highlight text to annotate it
X
Tony Pipkin: Alright, thanks everybody. Is everybody enjoying the conference so far?
Anyone have any issues?
[applause]
That's awesome, yeah. This has been a really good talk for me to write up. I've kind of
been stumbling my way through Git for the past couple of years, so this will be a couple
of tricks that I've learned along the way, and hopefully I can help other people out
with it.
In order to create a good pull request, first you have to branch the repository and then
fix the code. The first thing we want to do is update remotes. The way we do that with
YUI, we have a concept of origin, which is typically your personal GitHub, and then you
have upstream which is the YUI maintained GitHub. All of these are going to be git commands.
If it starts with git you can run this in your console. But git fetch upstream will
pull down any changes in the upstream that's happened since the last time you either cloned
it or the last time you've done a fetch. The same thing with origin. What this does is
that we don't want to create a branch from last year's code, or last week's code, we
want to create a branch from last minute's code.
After you pull down the code we want to create the branch. The way we do this is we use git
checkout -b, and then we put the branch name in. Then you do a space remote slash branch.
You have to decide which branch you want to do the pull request against. If it's docs,
you use live-docs. If it's a small change, a minor bug fix, something that doesn't need
to be beta tested, something that you don't want to go out in a beta necessarily, a hot
fix, use dev-master. If you're not sure, use dev-3.x. That's kind of the way I go. Most
of my code goes into dev-3.x, unless it's a feature that I want to get out right away.
Then this may be pretty obvious but you want to reproduce the issue that you're trying
to fix. Most notably it's kind of difficult to know what you're fixing unless you've seen
it before. You can read an issue on GitHub or do an email or an IRC channel or something
like that, and kind of understand the problem, but until you actually see it and reproduce
it, that is kind of your way to go.
Then you write a test. This is very important because the test will actually prove that
it's broken. You want to write a test of what you would assume would work, and then fix
the code, and then your test will pass. What this does is the issue that we have in our
sum function is that somebody wants to pass in a string of a number instead of just a
number, and they're getting out the wrong code, or the wrong response, whereas the first
line would give you 4, the next line will give you 0, 2, 2.
Then we fix the code. We start out with this. Can anybody tell what's wrong with the code,
the reason why it's concatenating strings whenever you sum it instead of just adding
the numbers in? Anybody?
Audience member: To keep the type.
Tony: Yeah, it's going to keep the type. So in order to fix this, whenever we do val +=
num we want to cast that string, or make sure every num that we get in is not a string,
that it's actually a float. This would be, you know, somebody passes in, I don't know,
elmo into the variable. It will come back as a zero so elmo doesn't get concatenated
into it.
Then the key thing is that after you write the fix for the code, you test it again. If
the test still doesn't pass, you keep fixing until the test passes.
Then you update the documentation. Once we've had our change, there may not necessarily
be anything that needs to change in the documentation for this example because some we would assume
that most people are going to be passing in integers so you don't want to necessarily
make documentation saying that hey we now allow strings to be passed in. But if it's
something that requires a documentation change, go ahead and do it before you start doing
your commits and your pushes. That way the documentation is in the pull request with
it. Yeah?
Audience member: For that documentation update, would you want to note that you're parsing
numbers with parseFloat as opposed to any other method like a times one or plus zero?
Jenny: Can you repeat the question please?
Tony: Yeah, the question is do you want to make a note in the documentation that you're
parsing the number with parseFloat instead of times one or a plus sign or some other
format. Yeah, that would be something that I would say would probably go in the API docs
as more of a description of the function itself rather than like in YUI's case with the user
guide. That way whenever you're writing the code you can just go ahead and put it up in
your doc block at the top instead of having to go into the documentation that's talking
about say if it's a math object or something like that.
Then whenever we go to do the commit we want to make sure you do as many commits as possible
locally. This isn't commit push, commit push, commit push, this is just committing to your
local machine. The more commits that you have, the easier it is to maintain it later on if
you have to back track something. Maybe there's a typo in the documentation, maybe the test
broke something else and you want to just roll the test back but you don't want to really
mess anything else up, the code still works fine. You can roll back just that one commit
without undoing all your documentation changes and the code fix.
Then the next thing to do would be to push the code, right? Before we push we want to
make sure that we compare our local diff to what's upstream. The reason why you want to
do this is to make sure that you didn't commit your build files or you're not going to push
a whole bunch of other changes up. You only want to do a pull request on, in our case,
the three files that we changed. That would be our JavaScript files, the test, and the
documentation.
Git diff --stat –color. Stat will give you just the code, or just the file itself that
changed. Color just kind of makes it a little pretty. Then upstream/dev-3.x, we're using
dev-3.x because that's the branch we did it on. If you want to do a pull request against
dev-master or live-docs you would put that there. Then the dot dot does the compare against
whatever your branch name is.
Then we're ready to push to GitHub. Is everybody with me so far? You've reproduced it, created
a test, fixed the code, passed the test, committed, checked, and now we push to GitHub. This is
getting pushed to our origin, which is our personal GitHub page.
Now we get the process of doing the issue the pull request. I don't know where the button
is now on GitHub because it seems like they change their interface about once a week,
but usually whenever you go there you'll have a green button once you get to the branch,
you have a green button that says hey you just made a push to this, do you want to issue
a pull request from there?
Again, we want to check to see where we want to do the pull request to, because GitHub's
interface doesn't know exactly where you pulled it from, what the upstream, it doesn't do
anything like that. In the upstream the drop down you want to make sure that you change
where you're doing the pull request to. Right now we have five branches unless there is
a frozen branch out there. But right now I have five branches, and that's live-docs,
dev-master, dev-3.x, master, and then 3.x, where master and 3.x you never want to do
a pull request against. We've been talking about trying to simplify it so this may change
later on, but for right now that's where it's at.
Once you change the branch that you want to do the pull requests against, you do your
title. It's pretty common to see the component in square brackets at the beginning and then
the fix and the number of the issue that you're wanting to fix, and then a small recap of
what it is. If it's not associated directly to a component you can leave that out. If
it's not fixing an issue that's been created so you don't have an issue number, you can
leave that out. But generally what you want to do is you want to make something that's
easy to scan across. People who are getting emails for the GitHub pull request, if you're
just scanning the git issues or the git pull requests, you want to just be able to read
the subject line and not be really confused about what it is that you're about to see
the pull request of.
And then in the body there are a few key points that I'd like to focus on. There's restating
the issues, or issue numbers. What this does is this creates the cross link in GitHub from
the pull request to the issues page so you don't have to have two of them open. You can
link back and forth. And also on the issues page you'll see a link at the bottom saying
that this issue's been mentioned in this pull request.
And then restate the issue again, kind of in your own words. You can copy and paste
directly from the issue if it was well written, but generally you want to restate what the
issue is and then follow it with a statement of how your code fix solves that problem.
Then you ping the people that you want to alert. I generally try to alert the reviewers,
anybody who owns the module. Say if it was against charts then I would ping Tripp. If
it was against something else then I would ping them. Then I would also ping the person
who originally reported the issue.
Then I personally do a TODO list at the bottom. What this does is it gives you the ability
to say okay, on this one I need to do my test, I need to do my code fix, I need to do documentation,
I have to update my history file, I need to get signed off on it or wait three days. I'll
create a whole list of that. What that does is whenever you go back to see your pull request
you can see what your progress on the pull request is, and other people can see that
as well.
Then once you've gotten all of that done, that's it. You just hit send, it processes
through GitHub, everybody gets emails, reviewers can review it, make comments. Sometimes they
start a really big conversation where things get heated, it can kind of go over into a
different topic, all sorts of other things like that.
I've got a couple of extras. This is kind of a sample TODO list that I do, minus the
things in the parentheses. I will normally issue the pull request first. That way I can
get the pull request ID and then go back and update the history file, and then commit the
history file to the pull request. It's important to note once you have a pull request on a
branch at all, any changes that happen after that pull request is submitted will also be
added to that pull request. That's another big key thing to know about why you do a feature
branch and why you don't just say okay on master we'll do this, because then everything
else that gets submitted to master is going to be a part of that pull request, and that
could get really ugly.
Then I'll do a sign off, or three days. The three days for me is kind of subjective. I'll
do either three days after the last comment or if it's something that I know that I want
to make sure that someone reviews then I'll wait till I get sign off and I'll bug people
on the IRC channel until I get someone to sign off on it or tell me what to fix.
I don't really have time to go over this, but writing good code commit messages is kind
of an art form. There are a lot of links out there and these are a couple of them. The
top one is a really, really good one. I highly suggest you go out and read those articles
about what a good commit message actually includes.
That's it. I'm @apipkin throughout most of the internet. If you join the YUI IRC channel,
I'll be on there most of the time. That's all I have. Anybody have any questions?
[applause]
Audience member: I have a question for you.
Tony: Yes.
Audience member: So why do you have to wait three days?
Tony: The three days, in my opinion, is to kind of let it air out and let everybody see
it and make sure that it has time to be reviewed or have questions or something like that brought
up to it.
Audience member: Does it have to do with the contributor model saying 72 hours?
Tony: Oh yeah, there's that. But that's the reason why it's in place in the contributor
model.
Yes?
Audience member: How do you know who's in charge of a project to ping? Looking into
the documentation it's easy to find out which branch to push to, as far as who's in charge
and who should review it...
Tony: Yeah, it's kind of... There used to be an authors list but we've kind of all started
maintaining all the modules now so there's not really a descriptive owner of it. We still
kind of have our baby projects. But if you're on the IRC channel you can ask anyone on the
IRC channel. You can send it to the mailing list and ask them. You can just submit the
pull request without pinging the author, and then if the author doesn't see it, or someone
who knows who the author is or someone like that, you can also ping later on to make sure
that people see it. It's not bad to issue it without pinging them, but if you know then
you can ping them.
Jenny: It does kind of speed things along. If you don't have the ping then Andrew will
help corral it to the right person. Also if you look at the Git history you can sometimes
tell who's had the most commits for a certain component.
Tony: Yeah, that's true. Any other questions? Excellent.
Audience member: I have a quick question for you.
Tony: Yeah.
Audience member: What do you do if you've issued... Can you talk a little bit about
why you would issue a pull request against one branch versus another?
Tony: The live-docs branch is kind of self-explanatory, I think, because if it's documentation it
goes against live-docs. For something like what we were doing here with the sum, I would
probably do that against dev-master only because it's a simple fix that doesn't really make
changes to other components. It also doesn't really make a big use case of having a beta
go out for it. If it's a new component, something that needs to be vetted against other people,
which means going out in a beta and letting the community try it out, then that would
be on 3.x. Stuff that I've released for data table, like the data table highlighter, I'll
do that on 3.x just to make sure that it hits a beta first before it gets issued without
anyone knowing about it.
Andrew: Do we have any more questions?
Jenny: Do we have anyone who might issue a pull request now and perhaps prior they might
not have known how? That's the goal here, so I definitely want to encourage you guys
to get this set up and have the repo forked in your GitHub account and have access to
it that way. As I think it's come up a couple of times, there are areas of the code base,
especially documentation, whether it's a typo or a tutorial that you might feel is missing,
that the community can absolutely contribute.
It's not necessarily source code but it's really, really valuable. Especially if you've
hit something that you've stumbled upon that is a pain point for you, likely it's going
to be a pain point for someone else, so for you to go ahead and fix that right then and
there and then issue the pull request I think will live on in the project for the next person
that might come upon that problem. I definitely encourage everyone to participate however
you can. It doesn't always have to be code. Even filing a bug on GitHub is definitely
a great way to participate.
Tony: Yeah.
Audience member: Is it better to make an issue request and then do a pull request? Or would
it be better to just do the pull request?
Tony: I would say it's according to the context of it. Sometimes the issue request filing
first might let other people who are working with it also make comments to it or say oh,
I've fixed this locally, this right here is something you might want to try out. That
could get the ball rolling towards the pull request and maybe alleviate some of the pain
points on that. But if you've already fixed the code and want to issue a pull request,
you can do an issue if you want to and then that way you have an issue number and conversation
can be going. But it's not really necessary to have the issue if it's a documentation
change. If it's something like this and I just see it internally then I might just do
the pull request and fire it up.
Audience member: Thanks.
Jenny: One good rule of thumb is how fast you think you can fix the problem. What really
tends to happen in real life is you have a problem and then it might take you an afternoon
or a week for you to get around to fixing it. If it's something where you have a quick
fix, you can go ahead and issue that pull request because it's immediately available.
If it's something that you might not get to for a little while then go ahead and file
the issue first to let everyone know hey, I found something, we can start talking about
it, and then you can issue the pull request whenever that happens to be.
Audience member: A problem I've found is prior to pushing my commits, the issues and pull
requests correlate to the same number.
Tony: Yeah.
Audience member: Like you file an issue it has a number, and if I file a pull request
it's one increment of that. When I'm writing my commit or my pull request, I don't know
the issue yet until I push it. Then I know the number.
Tony: Yeah, but if you do the issue first then you'll have an issue number for that
issue, but whenever you do the pull request you don't have the pull request number for
that. GitHub does... It's all issue numbers based on what type of issue it is. It will
show up under pull requests or it will show up under issues. But whenever I was saying
that putting the issue number in there fixes this issue number, that's if you have an issue
that's already up and you're fixing. The pull request's number would be something that would
go into the history file saying that this is the pull request number that the...
Audience member: Is it fine if the issue does not match the pull request number then?
Jenny: They never do.
Tony: Yeah.
Audience member: Okay.
Tony: Every once in a while you will get the case where it is the one increment, like the
issue is 260 and the pull request is 261, but that just means that another issue or
another pull request wasn't created between the time the issue was created and the pull
request was created.
Jenny: That's usually just a coincidence.
Tony: Yeah.
Audience member: I have a question for you. Do you have any tips on people filing bugs?
Tony: I would say follow the same principles where you would do... Let's see. With the
title, with filing the bug, the component, you don't have an issue that you're fixing
because you're creating the issue, then a small recap. And then for the body the same
way, you don't have the first line because you're creating the issue. You're not restating
anything unless it's another issue that maybe relates to another or it's an issue that relates
to another issue.
You could state briefly or really extensively how the issue was found, why it was created,
go to JSBin or JSFiddle or create another sort of reproduction of the issue so that
you can link to it. I've seen some pull requests or some issues created that have an animated
gif of the issue. I think it was a resize that was created. The JQuery, that one? I
love that one. [laughs] Still haven't fixed the issue but I love the gif.
Any way to recreate the issue or demonstrate the issue or identify the issue in text. I'm
a very visual person, I don't read a whole lot, so whenever it comes to pull requests
I would probably go to a link and start playing around with something versus trying to read
someone's interpretation of what the problem is.
Audience member: I have a new question. What do you do when your pull request has been
sitting around for a while and you haven't gotten any feedback yet?
Tony: I will usually ping someone again. Ping the reviewers.
Jenny: Andrew, ping Andrew.
Tony: Ping you. You want everybody to start bothering you. Alright, so everybody hears
that? Whenever you first create the pull request, ping Andrew on it so his inbox is full. [laughs]
Jenny: Well, I mean when it's stale.
Andrew: When it's stale, yeah.
Jenny: That's kind of his role is to shepherd these stale pull requests through.
Tony: Yeah, and then we also see these stale pull requests based on the sort order in GitHub.
If you feel like it has been neglected you can ping Andrew and you can go into the IRC
channel or you can bring it up in the Google... What's that thing called? Yeah, the mailing
list.
Andrew: Yeah, support. You can also add @triptych on your comments, that'll include me.
Tony: Ping us on Twitter. Most of us are all over the place.
Jenny: Thanks everyone. I want to encourage you to keep your questions and feedback coming
because this is an evolving process and the better we understand your pain points the
better we can document the process and clarify the things that are maybe opaque to you guys
even though we've kind of done it in a specific way for a while. We want to keep making it
better, keep making it easier.
Thanks Tony.
Tony: Yeah, thank you.
[applause]