[MUSIC PLAYING]
RACHEL FENICHEL: Here to talk about how
to write a good pull request.
Apparently PR means press release
to some people in this room.
That's not me.
[LAUGHS]
And sweet.
Who needs speaker notes?
Definitely not me.
It's fine.
This is stuff that I think about a lot.
OK, so this is just really quickly,
why write a pull request?
You probably all know, but I'm still going to say.
You can do this because you want to fix bugs.
You found something that's a bug in Blockly
and you fixed it in your--
you're like, hey, I could push that back over to Blockly.
That's great.
Also, pull requests for features.
Sometimes people add blocks.
So we have a bunch of list manipulation blocks
that someone added.
And when they did that, they added the blocks
and the generators for all of those blocks
in all the languages.
And now those are out there for anyone to use.
Also adding tests.
So that can be adding JS units tests.
That can be adding generator tests
as you add blocks, or for instance, the blocks
that we have in that test category that
are just for testing rendering corner cases.
Those are useful too.
And then finally, the feel-good part of this--
contribute to open-source, because open-source is great,
and that's why we're here.
What makes a good pull request?
It's self-contained, it's easy to review, and you tested it.
These are the things that make me happy
when I look at your code.
These are the things that make me say at conferences,
oh yeah, Jolly [? Toad is ?] really good to work with.
You totally want to be called out by your GitHub username
when you're not in the room.
That's helpful.
Five easy steps here.
So step one is talk to us.
Solve one problem.
Explain your code.
Test it.
And then finally, make sure handling the basics.
And I'll just go through each of these.
So first off, talking to us is useful for figuring out
whether the code that you're about to write is a good idea,
whether we've already tried to write that code
and run into insurmountable problems,
whether we know what's hard and what's easy
and how it should be done, but haven't had time to do it.
We can unblock you on problems that you don't know
exist if you talk to us first.
Filing issues is a great way to do this.
You say, I have this problem.
You say, it does this.
We expected it to do this.
This is a problem for us.
Those issues also help because other developers find them.
Other developers say, me too.
That's part of how we get the signal that this is not
an isolated problem.
You can talk on the mailing list, especially for features.
We'll see them come in often through someone saying, hey,
I kind of wish I had this.
Sometimes someone else on the mailing list will say,
I actually already implemented that and here's my code.
Or someone will say, yeah, I really wish I had that too.
And we might look more into actually doing that.
And then finally, providing use cases
is a really strong way to get us to want to have a feature.
If we know that it's going to be used a lot, if we know
that it's going to be used by a lot of different projects
that use Blockly, that's an incentive for us.
Solve one problem.
Small PRs are a lot easier to review,
and they're a lot easier to merge,
and it's easier to debug them.
This may seem obvious, but there's often--
if you're trying to get this in and you're
trying to make it work in Blockly so that you can do
a pull from Blockly so that you can get
your project out the door, and so you're like,
I'm going to write the entire thing in one
go so that I can just ship it over there,
and they'll approve it, and then we'll be ready,
this can add a lot of latency as we go back and forth if we're
trying to correct things.
So the smaller, the better.
Really easy to review and merge might be easier for us also
to just do quick edits in line.
Debugging is easier.
Not that you'd submit pull requests with bugs.
You wouldn't do that, obviously.
None of us would do that.
But if you have something smaller,
future debugging gets a lot easier
because we can actually see what broke when we go back
and look through commits.
And it's also easier to revert something
if we actually need to do that.
Test it.
We've talked about automated tests.
So we have the JS unit tests.
We have those generator tests.
And one of the things that I'm interested in doing
moving forward is revamping how we do some of our testing,
because JS unit is an OK framework,
and there are better ones.
That's a bigger project.
Anyone wants to talk to me about that,
we can do that separately.
But write tests.
And if you can't write tests because, hey, it
turns out testing rendered stuff is hard,
you can manually test it.
But when you do that, we need you to actually tell us,
I set up this scenario.
I tested it in these ways.
And these were the results that I
got, because if you're not doing that,
we have to do that for your pull request.
And that adds time that we're going
to be spending on that, because at the end of the day,
we're responsible if it goes out with a bug
and we didn't test it.
So manually testing is good for things
that you otherwise can't test.
If you're testing things that have a lot of rendering,
screenshots are really helpful--
screenshots, screen captures, GIFs
as ways of actually showing what the interaction is.
And that can save your time on write-ups.
Explain your code.
So you've written your code, and you're happy with your code,
and you've tested your code, and you're happy with your tests.
And you ship it up to GitHub, and you click Make
Pull Request, and you click Go.
And you hand it over to us, and now it's our problem, right?
At that point, we don't actually know what you've done.
So you ship your code over to us,
and then we have to go read it and figure out
what you intended, and what you actually did.
So obviously, inline comments and function documentation.
So Blockly requires that you have
[? JS doc ?] on all functions.
I find that really helpful for actually specifying what
my function is doing as well.
So those are just general good code.
Also, in your pull request description,
telling us what you intended to do
and how you went about doing it is good,
because it's not necessarily the case
that the code that you wrote does what you intended.
So if you can save us the trouble of figuring out
the intent, we can just look at, does the code actually
do the thing?
In case it wasn't obvious, these are things that we--
this is for all of our pull requests.
This is for pull requests within the team, too.
So you'll see that the pull requests
that we make, we're also filling all that out,
even though we're usually sitting near each other
and can just lean over and talk to each other.
This is still documentation that we
can look back at in the future and see, why was a change made?
What were we thinking when we did that?
Because it's all on GitHub.
Last one here is cover your bases.
So branch from develop.
Make your pull requests against develop.
Small things, but if you don't do that,
then you're going to have to go rebase,
and then everything is going to be terrible forever.
So just don't do it.
Check the Travis results.
We have a couple automated tests running.
If they're working well, you get to see those happy little check
marks at the bottom that say, all checks have passed.
If those checks aren't passing, we
tend to not consider it to be a high priority pull
request to review, because that can say,
like, the code is already broken.
The other person knows that they haven't fixed it.
If the tests are broken and you don't know what's wrong,
or you know what's wrong but you don't know
how to fix it, that's a great time to put a comment on
and say, hey, here is the state of this.
Can I get some help figuring this out?
If the tests are broken, and you're looking into it,
but it'll take you a couple days, also communicate with us.
Just say, hey, I'm working on this.
I'll be back to it in a couple of days.
That saves us the time of trying to go review your code,
and then have something completely change underneath us
after we've reviewed it.
Style guide.
So consistency is the goal here.
We try to have consistent code across all of the code base.
I have accidentally become the person
who stands there and yells at people about code style.
I promise you, it's not my favorite thing
in the world to do.
I also promise I will keep doing it
until all the code is in the same style.
We turned on Eslint.
This is really nice.
It used to be that Neil was individually responsible
for making sure that every single tab in every file
was in the correct location.
He happens to be really good at this.
But I got really bored doing this, and turned on Eslint,
and it's helping.
You'll get that-- yes.
We started, I think, using that because of Scratch team,
actually, when we started working together.
It's turned on as one of the tests that runs on Travis.
And so you should be [INAUDIBLE] dive into that
if there's a problem and see exactly what's broken.
It's usually, like, you missed a semicolon.
Things like that.
The robots are doing those checks
so that we don't have to have as much
back and forth where time zones come into play and all of that,
where the entire content of a review is actually,
you missed a semicolon and I won't merge it.
So again, not my most favorite thing to do.
I'd rather have Eslint do it all.
I assume that I'm talking fast.
That's a thing that I do.
But I can't actually tell when I'm up here.
So if all of that was way too much to say in one
go and way too many do this, do this, do this,
we have templates for the pull requests.
We have templates for issues.
They ask you questions like, what did you intend to do,
or what problem were you solving,
or what was the expected behavior?
If you're filling out the templates,
you've probably done all of these steps.
If you're at the point where you're filling out the template
and you don't know how to answer those questions,
please don't just delete that portion of the template
and move on.
Please think about that question and figure out
how to answer it.
Those of you who are contributing, thanks.
Those of you who are going to contribute, thanks.
It helps.
And I can take questions.
SPEAKER: I'm going to add one minor thing
and that is, be prepared for follow-ups on your pull
requests.
There are things that we might call out
and they might seem simple, but it's really helpful
if we can get your help to go back [INAUDIBLE] add
that semicolon or whatever it is, or alternatively,
mark your pull request as editable by us
when you make that in GitHub.
RACHEL FENICHEL: Yeah, there's a check box that's
Editable by Maintainers, or something
like that, that's useful for letting us go in and make
the small changes immediately.
Think we are close to--
yes?
AUDIENCE: So suppose you have a change that--
are there good ways to determine this appears
to be a concise change, but it actually
has big downstream impacts?
Is there any good guidance for that
in terms of how you guys look at a change and go, oh-ho-ho.
You touched that module.
Uh-uh.
[LAUGHTER]
Are there things that keep you up at night?
I don't know.
SPEAKER: Coordinate systems.
RACHEL FENICHEL: Coordinate systems, right to left mode
will be the death of me.
Honestly, just talk to us.
Just put it out there.
Yeah, just saying, like, I have this,
and is this going to be a thing that'll work?
And we're happy to answer that.
And if you're changing block rendering code,
I'm going to have questions for you.
AUDIENCE: It's not a question.
I was just going to say, I think one of the things that
was really successful when Sam was contributing
or is still contributing a bunch-- but he would solve
a problem in his code base, and then point us to, hey,
this is how I fixed this problem.
Do you want this?
Which gave us a way to digest it without,
I think, you having to do a bunch of work
to try and see whether it was the right thing.
RACHEL FENICHEL: Yeah.
SPEAKER: And this is very much in line
with what Neil was saying about host
a copy of the playground with your block errors,
or your bug fix, or something like that.
Then we can really play with it without having to do
a lot of overhead on our part.
AUDIENCE: Yeah.
And I was just going to say that Scratch Blocks also
has a playground.
So similar.
SPEAKER: [INAUDIBLE].
AUDIENCE: This is a pretty broad question,
but you mentioned that part of the future
plans to make subtle UI changes to Blockly core.
How would you see the open-source community
contributing to that effort?
RACHEL FENICHEL: I think in large part
through the discussions of what those changes are going to be,
and through telling us if those are going to be
especially disruptive changes.
And if we can specify them into small enough things,
then we'll have help wanted issues that people can just
do stuff on.
Similarly, creating an issue and having
that be a place where we start to talk about what it should
look like means that later, however long it takes when
we've hashed out this is what we want,
then we can just point someone at that issue and say,
go ahead and do this.
So I think people can definitely be helping out with that.
AUDIENCE: I do want to add to the comment about the Scratch
Blocks playground.
If anyone here is considering contributing to Scratch Blocks,
it is important to not only test your changes in the playground,
but also link them back up to the Scratch GUI
and make sure they work there, because the Scratch GUI has
a different workflow of the VM generating, your blocks
workspace, and workspace conversions,
and things like that that the playground doesn't test.
[MUSIC PLAYING]