Hi, good morning. My name's, I'm getting away from the audience. I love you already. Hi, there aren't very many of you, but it's all about the quality. So hi, I'm Lorna. I'm wearing both my hats today.

So I am an open source maintainer. I'm also an open source contributor, so I sort of see that from both sides. And hi. And I'm also a team lead. So like commercially, I'm also a team lead. So I run a team, and I am responsible for maintaining code quality with them as well.

So I'm going to talk about how to get your patch accepted. This is predominantly aimed at people offering contributions or trying to do good work at work. I have been completely unable to write this talk in a way that doesn't cause me to sort of trip over and start giving advice for the reviewers as well at some point

during the talk. So we'll be looking at it from both sides of that relationship, because I think ensuring that your patch is awesome is the same as ensuring that someone else's patch is awesome. There's just an awful lot of crossover in there. So this talk is full of advice you never knew you wanted and you didn't ask me for.

I've got some stories to tell. But it all boils down to three main ingredients that you need to get your patch accepted. The first one is the code. Because if your code is rubbish, I don't want it in my project, and that's easy. Everybody pretty much does this.

The second are the words that describe what this patch is or does or fixes. Maybe it tells me why I care, which can sometimes be a missing link. And that brings me on to the third ingredient is understanding it from the maintainer's perspective or your team lead's perspective.

What's the big picture? Where is this project going? And how does your change fit into that? Does it fit into that? So three things that is very important that you cover. Before you submit your patch, bear in mind that the first thing I will do when I see it

is wonder, what does this do? I didn't write the code. I'm not already on the inside of this process. It just arrives. And I go, oh, what's that? And I kind of poke it and smell it and kind of try and understand what it is. So it's really important that that information comes in and it's something that you think about

as you prepare the patch. You write the code. And then I'd like you to hesitate before you actually submit that patch and cover a few more things. Give me some information. Are you building a feature? Are you fixing a bug? What is this? Does it relate to a ticket?

Now, some projects will require that you open a ticket. And they like to have patches which come in through tickets. A lot of the examples in this talk relate to Git and GitHub because that is the way that I mostly work. I think everything is transferable. It's not a coincidence that the GitHub issues

and the GitHub pull requests are basically the same feature. They're kind of really connected. In my projects, I don't require that you open a ticket. If you see something that you want to fix or enhance and there isn't a ticket for it, I don't think you should have to do the paperwork

to open the ticket before you can send me your awesome fix. It's just one more hurdle, and especially in open source, that's just annoying and we might lose things. However, if you think of a code review process as a horse race, so there's a series of fences

and your horse may fall at any one of these fences, the whole load of criteria which can cause your patch to be rejected, then you can think of it like that, maybe not having a ticket is a problem. I like to think of a code review more like a scoring system. So if your patch is not related to a ticket,

that's not really a problem, but you don't get any points for that. If your patch is related to a ticket, perhaps you get plus 10 points. If it's related to more than one ticket, you get minus three for each ticket it's related to. Because if there's two related tickets, they're kind of the same thing, then fine.

But if you've got two different things or four completely unrelated things, I do not want this patch. If you need some help preparing your branch correctly, then let me know and we'll turn that into four patches. The nice thing about coming from a ticket is that the ticket probably describes the original feature desired by somebody

or the original bug as it was reported. So if it does relate to a ticket, you, the contributor, and me, the reviewer, we all have more information. There might be some acceptance criteria, something which says I saw that the system did that, and instead it should do this.

If there are no acceptance criteria, you need to invent some. Think about how this feature will be used, whether it's an API endpoint or actual user interface. Think about how it will be used. Think about what's acceptable. And now try and apply those criteria to your patch.

Because I'm going to. It just saves time if you've done that already. Make sure you've got everything that you need, all of the code and nothing that shouldn't be here. So check that everything looks like it should. It's really important to understand what's included.

And if you're not terribly familiar with your source control system or you're not familiar with the workflow, you might need some assistance or you might need to make sure that you know what you're doing. Talk to the projects for help. Everybody who is responsible for reviewing patches has an interest in helping you make more awesome patches.

Never be afraid to ask. Try and check you haven't included anything that shouldn't be there. So you can check what's different between your code and master. But remember this will include anything that's come in on the master or your main branch. Adapt as appropriate. Since you started your branch. So instead try this.

Git diff master with three dots and head shows you everything since the last common commit, i.e. just the commits on your current feature branch. Get to know your diff tools. Git diff by itself will just show you that inline ASCII art diff that we're all accustomed to seeing.

But get to know your tools. Git diff, everywhere you have a command that is git diff, you can replace it with git diff tool. All one word. And that allows you to start to use cleverer tools than just the inline red green line diff that git gives you by default.

You can configure your config. Configure your config, wow. With diff dot tool and say which tool you'd like to use. I'm a vim user so most of the time I use vim diff as my diff dot tool setting and that's awesome. Gives me a nice side by side and I can push changes from one to the other.

If things get really sticky, sometimes I'll use meld. Which is cross platform and has a nice configuration setting so you can get it to open rather than just pairs of files, you can get it to open the whole thing. So you can see the tree and look at where in the tree things changed and what the old and new versions are.

It is worth getting to know your tools so that you can be certain about what you're submitting and as a reviewer. So it's very easy to review other people's branches and understand what is coming in. Sometimes a patch can be too big.

There is such a thing as too much awesome incoming code. Trust me. Especially if I wasn't expecting it. This is really, really hard to review. I had a dilemma recently where I run an open source project which is made up of a few different

open source PHP projects and there's a web front end and API back end. The API back end uses a different coding standard than the front end and somebody offered me a patch which kind of mostly fixed it to use a different coding standard than the one it does.

37,000 lines of change, which I wasn't expecting and it arrived on a day where I was on the road speaking at a conference. My co-maintainer was with me at the same conference. There were eight open pull requests, all of which would have had to have been rebased.

Was it a bad patch? Not really. I mean, it wasn't complete but we could have worked on it. Did I merge it? No, I didn't. It was too much change. Giving the advice that a small patch is way more likely to be accepted

is a little bit like giving the advice that how to avoid conflicts in source control systems commit before the other person, right? This is the same thing. There's nothing to say that you can't build a big feature but you need to build it in such a way that it's digestible by whoever has to retrieve it

and at work, we have pretty regular release cycles as in we merge it and then we just deploy it. It's not quite continuous but it's potentially continuous. We just click the next button. So by building a small change and then a small change and then a small change and then a small change, the feature still gets shipped and usually faster than it would have if we have one big step coming in

and we try to launch that. There are some common code mistakes that I see all the time. I have two hates relating to comments, actually I have more than two if you talk to my colleagues but let's talk about the two mainstream ones that I see all the time.

Really exotic code with no explanation. Like if I have to go and look up the operator precedence rules in order to understand what your code does, I probably don't want that in my code base, okay? So we need to write code in the way that we want to read it at some future point.

The opposite is also true. So missing comments, commented code. Right, source control's been around for a while, don't know if you've heard of it but it has been invented and it means that we do not need to leave commented code in code bases. It litters the place, it's hard to read

and in the straight diff view it's not marked as a comment. So if you comment at the beginning of the line I basically can't see it when it's indented and I rejected a pull request because I thought it was nonsense recently and it was because it was commented code and it just wasn't clear to me that it was commented. Don't leave things lying around in your code. If we need it later we'll just get it back,

that's how this works. I'm not sure this really belongs here but it genuinely happened to me last week. Pull request came in, I'm part time in my day job as principal developer so things kind of queue up and wait for me to come back into work and then try and mop up what a number of full time people do when I'm not there. And so I popped in and there was a pull request waiting,

client very urgent, we contacted the client so we're just waiting for Lorna, she'll be in in the morning. My colleague had added a single variable called x, I'm a PHP developer so $x, single variable undocumented as an optional final parameter to a method.

We specialise in legacy systems, it's not like he's messing up an otherwise beautiful code base but really we can do better, there's no need to sync to the lowest common denominator around you. And we were under pressure and I took a deep breath and it wasn't the right thing

and I rejected the pull request. A bit later he came back and I also mentioned one other thing that seemed a bit weird but I hadn't really understood. When the pull request came back in later on in the day, it looked completely different, there was no single character variable $x, in fact that was completely refactored away

was to do with the type of file that he was sending with an email and he had just refactored it in a much, much better way. And so my code smell and my doubt was the right thing to do there. Our second attempt, as is so often the case, was really so much better than the first attempt

and that is bravery on the part of the reviewer to call it and say that is not how we do things. On GitHub projects, you'll find a file called contributing.md. Helpfully GitHub doesn't show you this until you try to open a pull request at which point it seems too late because you probably need to know what your branch should have been called and things.

So have a look in the projects that you are contributing to because this will tell you do you need a ticket, the branch should be called something completely should it be rebased or squashed or any of those other things. It's also worth checking in directly with the project maintainers. Most of them have chat channels or mailing lists or there'll be conversation going on the issues

if you're out of time zone or they're just not real-time people. And it is worth talking to them, just taking their temperature, letting them know what you're working on. It helps the projects to keep moving. Something that is omitted too many times

can you break your code? Try, because I can. I can break your code. One contributor laughing at me from halfway back in the room, hi. I can always break your code. I think my developers at work,

the people that I work with think that I have some kind of magical mystical power that causes their code to just malfunction spontaneously as soon as I get my hands on it. And the truth of the matter is this isn't magic and it's not even rocket science, it's not hard. I do things like is the data missing or is it overlapping or is it a little bit ambiguous

because you've allowed me to edit it in too many different ways and I have deliberately tried to confuse the system or omit something. I'll sometimes log in as a different user. That seems to throw everybody, too many developers building things logged in as the admin user strangely,

or not logged in as the admin user, or. Those things are easy to try. The truth of it is the user is an idiot and I have extensive experience of writing code that was broken by idiot users and so now I do a really good impression of an idiot user. The reason that I can break your code

is because my code has been broken so many times before. So try, see if you can beat me to it because a pull request that I can't break so impresses me. I remember being on site with a media company

in London doing some work and quite an angry QA person showed up at my desk. This is never a good thing. Quite an angry QA person, woohoo. And she said, I am very angry. I cannot break your code. Right, she's a QA person. It's her job to ferret about and pull apart code

and find the bugs and in my code she couldn't find the bugs. And she said, but I will. And off she went. And maybe it's a threat. I think that's a compliment because I have always tried to think about what could go wrong and that's something which will enormously help.

Both to get your patch accepted if you're contributing to a project but just to keep things moving at work to get features shipped if you're thinking about these things. If there are automated ways to break your code maybe there are tests that you can run. Coding standards checks which if failed

will cause your patch to be rejected. Joined in has a syntax check ever since the time that a co-maintainer and I both were running development platforms on a different version of PHP to the live platform. So guess what? He committed a change and I reviewed and merged the change in between as we broke the live platform.

Well done. So now I have a syntax check that runs on build. So GitHub has this commit API, commit status API. You open a pull request and it will run the builds for you as calls out to Jenkins, integrates with Travis. Really, really helpful. This is fabulous because it means that bad news like your code might be fine

but in contravention of our coding standards therefore we won't accept your patch. That news gets delivered nice and early like the syntax check. That's worth just having and then you know. Half the time it's a conflict marker that somehow made it into a file.

One thing to really look out for and it's something that I think as developers we occasionally underestimate the importance of. And that is your commit messages. The commit messages that you write on your journey to hack around to end up with the world's

most perfect patch do matter. I do see those. I do read the logs of my project. I call it archeology because I work on legacy systems so things arrive and I dig about to try and understand how on earth we ended up where we are today.

And this matters, this does matter. If you get to the end of your branch and it's a bit of a mess and you don't know what to do figure it out. This I use a lot. All it does is takes everything in your branch since you branched off master and makes it an unstaged change. So now you can curate me some new commits

from all that change that you just put in on a branch either it was a single commit and I've complained or it was a bit of a long journey of commits that went a bit round the houses. Sometimes we can't tell at the start of a feature quite how that's gonna look when we're finished. If it was all very straightforward

and we knew exactly what to do actually none of us would be doing it because we'd be bored. We all need the challenge and the entertainment. So sometimes can be a very scenic journey to eventually arrive at the working solution. So clean it up. While I'm on the topic of commit messages

I need more information than I get in the diff. Please don't write me a one liner that describes what the diff says. I work with a lively and soon to be talented junior developer who at work I use a slack channel with git, yeah.

Yeah, sorry. That's fine, you don't need the slides for the story. I'll tell you the story, you can grab this. I will also upload my slides. I work with a, like I say, soon to be talented junior developer and he put through a commit message. We commit to gitlab, gitlab calls the Hubot, the Hubot talks to us in slack channel

and the commit message said, shuffling the JavaScript files around which caused me to simultaneously say do you call that a commit message and also pop open the diff to see what he had done. He had very much shuffled the JavaScript files around but I need in the commit message,

I need more information than I get from just the diff. So tell me something about, I never really did find out why we had shuffled the JavaScript files around and if we ever have a problem with the day that we renamed all the JavaScript files in one commit, we'll still have no idea why that happened and that's why this matters.

Don't tell me what's in the diff, I'm gonna read the diff anyway. Every commit has the apparent, the diff, who did it, when they did it. Commit message is the missing magic.

It tells you potentially why something was done, the context of this change and if you just write, fix all the things, I'm gonna hate you. Worse than that, commit messages read by other people

but sometimes you are the stranger. You come back to your code in three months and you've slept. You are the stranger reading your own commit messages and hating yourself. Do yourself a favor, some nodding, yeah. Do yourself a favor and write yourself a really helpful note that says this seems like a really strange change but there's a logic and then explain the logic.

Please write me an accurate commit message. I see too many which are, describe the most recent commit and kind of forget what they did in the branch before. Again, a lot happens in open source and people can be kind of chipping away, doing a bit more work, a bit more work on a feature.

I need all of it. Please try and use some kind of sane format. I have a favorite list of things you should do for a sane format and it's here. This article is so good that you've just saved yourself 20 minutes of rant. You can all just read the article. To summarize, a headline that isn't very long which is gonna fit on Git log with one line,

it's gonna fit in the chat ops notifications, right? Headline. If you need to write more words, blank line, more words. And write as many words as you like. Give examples of command use, give bullet points, give links, give anything you want to.

I am either famous or infamous, depending on your perspective, for writing commit messages which are longer than the diff they relate to. I don't think that's always a bad thing. Sometimes the change can be very elegant but the reason and the journey to get there can be a bit longer than that. Don't be afraid to write it all down.

Please keep your commit messages safe for work. It doesn't matter how hipster or hobbyist or cool or out there you think your library is. Hello, I am the consumer of your open source library.

I build things on what you've made and sometimes I wanna do that in a commercial setting. I don't want to read your gender-biased, homophobic, racist comments. It's unnecessary. I don't even really need your swear words, although sometimes that's justified.

So please, safe for work. There's one thing you can do in a commit message which will automatically get your patch rejected. If I see an apology, right, I don't need it. I'm not taking it and the reason is, if it's good enough, there's no need to apologise

and if it's not good enough, well then it's not good enough and maybe we can get together and try again but if you apologise, not just commit messages, I see this in code as well. No, not ready to go in, yeah. I'm so sorry about the state of this patch.

I'm so sorry this is the best I could do. Any kind of sorry makes me go, I probably don't want this commit in my code. So I think this is a real, what's the equivalent of a code smell? Commit message smell. Like, it's not really good enough but just know.

When you're working on a patch, it's worth checking if it will merge. If it won't, somebody will probably ask you to do something about that. GitHub has like a no green button scenario webpage. I don't have a good relationship with the green button but anyway, there isn't one because it's gonna conflict.

So it's worth checking if it will merge. Now you can check by just, open the pull request, but you can also check by seeing how that merge will look. So you can switch over to the master branch and do this with no commit and no FF. So it won't complete the commit, it won't complete the merge but you can see if there's gonna be conflicts, it will tell you

and if not, there aren't any. So you can use this or something like this to do a dry run on whether your branch will merge into whatever the target branch is. If it doesn't merge, oh and then do git merge dash dash abort to get your repo out of the merging status

and back to being normal. It's as if nothing happened at this point. If it won't merge, then fix it and that might be, there's a couple of ways you can do that. Some projects are happy for you to just merge the master branch into your branch and deal with the conflicts. The more textbook and occasionally required way is to rebase your feature.

So if you've worked on a feature on a branch, things have happened on master which have caused conflicts, they'll say oh you should rebase this. So take your branch and reapply it on the tip of master or develop or whatever your main branch is. If you're rebasing because there are conflicts

when you merge, there will be conflicts when you rebase. Like there is still a conflict between these two branches and you're gonna see it somewhere along your rebasing journey. Have seen some situations where there's actually a lot of conflicts along the rebase journey because you can be applying a lot of commits. So sometimes the merge is easier

because it avoids all the crossing over that might have happened in the middle of the branch. So rebasing. Opening a pull request on GitHub is not just the action of throwing code over the hedge.

Done, here you go. It's the beginning of getting your patch accepted. When you're opening a pull request, check the source and the target are correct. This is surprisingly easy to get wrong and I still do it sometimes.

The source will be your branch if you're working as an open source contributor, that's probably on your fork. You're sharing a repository in a commercial project, that's very likely on the shared repository. And the target branch will be either master or develop or whatever. GitHub has a setting for what the main line development branch is.

So usually it'll default correctly, but it's worth a check and then scroll down, have a look at what diff that's generated. This will tell you if you're vaguely on the right lines. If the diff looks like nothing you've ever seen before, something is terribly wrong. And you should just close this web browser window and try again, check you pushed the branch,

check it's called the right thing, I don't know. Something is terribly, terribly wrong. This is usually the moment at which you'll realise that you branched from completely the wrong place or something else terrible has happened. Your pull request needs a title. Most people will view a list of pull requests and so I will only see the title.

Pull requests have to be open quite a long time on the list before I get to know them by their ticket number. So if you could possibly include any extra information, maybe some words to go with, that ticket number, that would be awesome because I'll know what thing it is on the list and I might remember why I care enough to, you know.

I have 45 minutes before my next phone call, I'm looking at a list of pull requests and the one that says copy change on the navigation will make me go, oh, I've probably got time to merge that. And the one that says ticket 42, you're unlikely to get that merged. And it's right up there with submitting small patches.

It's about perspective, it's about remembering the maintainers. And for years I've been saying, be sympathetic to your open source maintainers. They're busy people, they're volunteers but actually it's not much better at work because I'm very busy there too. I'm sure that's true for everybody.

There's not special really. Always try and lower that acceptance barrier and that's how you'll get your patch over it. Description, I need some words. I really need some words. I had one pull request opened and I think it had a single commit and the title was the same as the commit message

which was implements the get cache function. This is a bit like implements the dancing bear feature. Wasn't really on my roadmap. Dancing bear, got to maintain it. Implements the get cache function, I'm thinking about that and then thinking,

well, it's come from a co-maintainer actually, it's not even a trusted contributor. This came from someone who definitely should have known better. So in the diff, and in the diff, there's a new get cache function and inside the get cache function, there's code that looks like cache code and nothing else.

It's not called from anywhere. I have literally got a commit that implements a get cache function. Jolly good, this does not help. Write me some words. What had actually happened was we had somehow eliminated the get cache function from the code base, I don't know how and therefore that was the reason

that the search function was broken across all platforms and all we needed to do was implement the get cache function which is what he had done. But late at night in a huge hurry and then it opened me a pull request which did not allow me to understand the urgency of the situation because I had no idea what I was fixing.

So tell me what it does. Tell me not necessarily exactly how to test it but what are we changing about this? I'm working with a client at the moment that likes paperwork, their ISO 27001 compatible and they like lots and lots and lots of paperwork and it's really interesting because it means

that every feature we build or every bug that we fix or every system change that we make, we have to write how to test that it isn't there and then how to test that it is there and actually that really makes me think about the big picture and what the users will see and how that will look different. And that's quite good information

that would be really helpful to be included when you're submitting a patch. Tell me why I care. I work, it's my job to care, I'll probably merge or change eventually. Like the client will shout loud enough, we'll get there. In open source, that's not true.

There's a lot of abandoned, I'm sure all of you have opened contributions or offered contributions and some of them haven't been accepted. Sometimes you don't even get a response. Call request just lies there in limbo forever. Tell me why I care. You want to get my attention if you want me to invest my time in bringing your code into my project

so that I can be responsible for it forever. Please tell me why I care. Please write me some words. Code review is, it's a process

and it changes a bit depending on what exactly you're reviewing or what the context is. But not a lot. I've been an open source maintainer now for a number of years and I was a contributor before that and still a contributor to a bunch of other projects.

Do I always follow my own rules? Probably not. But I think there are some things that we can do to prepare for code review, to make sure that our changes are ready. And as reviewers, this is the point at which I couldn't avoid tripping over and becoming a code reviewer because sometimes that's my perspective too.

And I'm sure that all of you will and perhaps should review as much code as you can. To be a good writer, you need to read. You need to be really well read. But I think that's true of development too. That you need to read other people's code and to actively participate in code review

and in evaluating code I think is a really smart way to do that. When you're looking at a patch for the first time, read the title and description and think about what shape you would expect that patch to be.

So I had one come in for a specific ticket and the ticket said the client has requested a copy change. Okay, so I viewed the diff and the diff had quite a complicated SQL statement in it. And I went, what? I didn't know the system very well.

I haven't been in my job very long. I didn't know the system very well and it turns out they have a CMS. So in order to make this copy change across all the platforms, we need to do that by editing the records that we've got stored for them in their CMS system. And we were just doing, I did mention that I deal with legacy systems. Yeah, we're just doing that by entering a line into a database because it's dynamically generated.

But because I had, there was nothing wrong with the SQL statement. I mean if you just read the code, the code looks fine. As a code reviewer, you need to do more than that. You need to think about is this the right perfectly correct piece of code or is this just a perfectly correct piece of code

that does not bear any resemblance to solving the problem in front of us. Any grammatical point? Please don't merge someone's pull request. Please review it. I want you to keep that attitude of skepticism right until you can't prove that it shouldn't be accepted.

This is really important. I come to work and people say, I need you to merge this thing. And half the time I don't merge it because it's not ready or it's not done or I can break it or all of those. I had a good one recently where the same junior developer, it's a really small company, the same junior developer said, I'm not even sure this merits a merge request.

But we make these rules for a reason, so I will. I was like, good man. So he opened this merge request and it took him five attempts to get me to merge this thing. Because first of all, the database patch didn't have the right numbering in it and then he forgot something else and then I didn't like this piece and then there was a typo and then, oh my goodness,

so he spent all day going around on this thing. All of these things he could have picked off before I got it and hopefully that will happen next time. I feel like he learned something. But just keep that cynicism a little bit longer and review it, be open to it, but don't try to merge it. When I try to merge code,

I find that I accept things and then it probably gets to the test platform and I realise it's not complete. I think anyone can be a code reviewer. I don't think this is the responsibility of the team lead

or the maintainers exclusively. At work, I operate a system of peer review. So I don't mind who writes the code and I don't mind who merges the code as long as those are not one and the same person. Which means that the most junior person in the company

also reviews everybody else's code. And I review his and we all kind of share ideas. So he sees my changes and often picks up bugs in them and that's really important too. He sees how I would approach a problem.

In open source, the merging can't be done by just anybody because we tend not to just open up right access to our repositories. But having input from other people on an open pull request makes me way more likely or way more confident that this is actually correct.

I mostly work on joined in, which is an open source event feedback site. So if I've got a conference organiser running a local dev copy, pulling a feature and saying this looks great, or I'm not sure this would work for us, that really influences whether or not

I merge and accept that. We had a bug last year. Unfortunately, I had flown to San Francisco, even more unfortunately, with my co-maintainer. This is a mistake, right? One piece of advice, do not travel with your co-maintainers.

I once had the other co-maintainer and the only other person with server access, all on the same transatlantic flight to OSCON with me. I just logged into channel and it says, if anything breaks, you need to wait 24 hours, and we're on this flight, so sorry. Look out for the flight number

if anything terrible happens. I mean, anyway, so I got to San Francisco with Rob, and I woke up at 5 a.m. local to emails, Twitter stream, everything about some users being unable to log in on joined in. So Rob and I were sharing an apartment, and I very optimistically logged into IRC,

and he was not obviously awake because he was not logged in, so it's like, okay, so now I know who is fixing this thing. However, due to the time zone difference, one of our contributors in Europe had written a patch, and so I logged into channel and I was like, I see a problem, I see a pull request, where are we at with this?

I have the ability to put anybody's branch onto the test system, so I did that. Some people tested that, and someone else pulled it onto his dev platform and tried it there. Because people were able to make it work on someone else's dev platform, and on the test platform, I pressed the big green button in GitHub, which never happens because I can't,

I don't believe that code review can be done, other than like on a readme, without pulling the code, looking at the code, running the code, poking the code, right? I don't have a good relationship with this button on a web interface, because I don't think that's how this works. But on this occasion, I clicked the green button, and then I clicked the go button on Jenkins,

and then I went back to bed. Because somebody else had done that code review, and if you want a feature accepted, test it, come back and say, hey, I really liked it, it worked for me on this platform, yadda yadda. I am slowly learning that I need to check

the pull request queue on a project, before I open them a pull request, because half the time, the thing I'm gonna fix has already been fixed, but sometimes it's still sitting in that pull request queue. If that happens, pull that branch by all means, but test it and feed back on that pull request, and say, hey, I tried it, it really looks awesome to me

on this version, on my platform, et cetera. There's no reason that says you can't get involved. The merging will actually be done in different ways for different kinds of projects. I like the peer review, I enjoy getting the whole project involved, by its very nature on an open source project, we have the gatekeepers,

we have people who take that responsibility. And at work, if something breaks, I will say, who merged this? And my developers have stopped saying to me, oh, well, it wasn't my code. They've learned to just realize that I think that's a responsibility thing, we take it quite seriously.

As a code reviewer, there are some key skills that you need, and in particular, the ability to see what you are not looking at is absolutely key. It's very easy to eyeball the diff,

and maybe the diff looks good, but it is the things that are not in the diff which cause you all the problems. And this is what makes you both an awesome reviewer, but what will really get your patch accepted, what will really enable you to get a complete patch

and make that work. Does this need documentation? This is my number one reason for closing work pull requests. We've changed something fundamental, and there's no documentation to go with it. Close. Oh, I'll add it to the wiki later. No, it's part of the patch. If it's not in, close.

Maybe that's harsh, but it is improving our quality, and on those legacy projects, that really matters. Tests, do you require tests? Do you think this feature could have needed a test? I will sometimes, not close, but delay a pull request. Be like, this will be acceptable when it also has documentation, tests.

Are there missing database patches? Did somebody make a local database change that we need to make this code work and it's not in the patch? Maybe they, because it's a new file, maybe they forgot to add it, that happens. Deployability, I'm not sure if this is a word, but it's a really important issue.

The ability to run this code somewhere other than on your platform, okay? Works for me is not how we ship software. Works for me is a really good starting point, but works for me doesn't do the rest of it. It has to run on your dev platform, but then on my dev platform and on the test platform

and on the live platform, and you have to be able to see that. Had a diff recently which came in, which changed the config and it's like, you've just changed the path with no respect for what platform you're on. This code we've just inherited only runs on one platform, the live platform. So now it only runs on one platform, the dev platform, that's gonna be a problem when I deploy this.

Looking out for those things that are hard to see because they're not there. The forgotten changes to templates, the follow-on changes to emails when you've remembered to change the templates. Reporting systems, oh, I have a client who I think we must have broken their reports three times in the last six months because neither they nor we

can remember that we have reports, and so we do these beautiful and amazing refactors, and then we update their emails and all their templates, the website looks great, and then their reporting breaks at the end of the month. You've got to be able to see that this change is not in your diff. Monitoring changes. This is sometimes invisible to developers.

I'm in the middle of doing a big refactor of gearman workers because we're running like 20 different ones, and I'm gonna group them up so that it can do more than one thing, so I'll have like five different ones, but we'll have 20 of one of them. This is going to break the monitoring system completely, which checks that certain gearman jobs are running at all times.

So more things to bear in mind. Cron jobs can be really hard to remember, especially if the config for that or for anything is not in your repo. These can be really difficult things to remember. As a reviewer, this is really difficult to identify

and then can be controversial to deal with if there's something missing rejected, and this can upset people, particularly if you are the junior developer reviewing for the senior developer, she doesn't want her patches rejected,

but if they're not good enough, then it's the right thing. So don't be afraid to reject it or at least to push it back and say, I also want this. I think this would make it complete. We should also include the following. Push it back, and if a change comes in, like my big coding standards thing that you don't want,

you know you're never going to merge the dancing bear feature because you don't want it, close it, reject it early. It's kinder to everybody, just do it. And we can iterate or not, but we have to be up front and we have to be able to communicate about this.

Sometimes the rejection will come automatically because you're running those automated builds and it failed on coding standards. If you're not accepting a patch, it's really important to include constructive feedback. I had one situation where a new employee had had, you know, he was really new, so his pull request never got accepted first time,

in fact, or third time, or fifth time, right? So he had quite a long go of just everything being hard work. I have to work with this guy, right? So this is fabulous because he's now much better at all these things. But one day, I think I merged it,

but I commented, he sent me a perfectly good pull request and I merged it, and my comment back to him was, like, it was cool, but I really didn't like the comment syntax that you used. The C comment syntax is valid in PHP, but we typically use two slashes. He came straight back to me in channel and said, so my patch was awesome, yeah?

Because all you can say is what's wrong with my comment syntax. Yeah, he was right and I didn't say that. I did not say, well done, your pull request is otherwise perfect and you have worked hard at this. I said, a bit of a problem there. That's not very constructive and it didn't recognize the work that he had done.

The feedback that I give does depend a little bit on who it is. For an open source contribution, people have taken the time, we probably don't know them, they're volunteers and the lag between iterations can be quite long. I am more likely to add one more commit onto the branch

to fix whatever is missing or whatever small thing I don't like and then merge it myself. With colleagues, I have to work with these people and I don't want to be picking up after them all the time so they are very much more, they're also paid to be at work when I'm at work, so they're very much more likely to get the,

no, do this. Whereas in open source, unless I know, so joined in has a lot of new contributors, it's a lot of first time open source contributors. If I know those people are involved because they want some experience and they want some feedback, then we'll do it like that and they can improve. Otherwise, I'll comment what I did

and fix it myself and merge it. So my approach is quite pragmatic and I'll do often some combination of those things. There is such a thing as too much feedback. Please, I would never ever have mentioned the thing about I don't quite like your commenting syntax

if there had been anything else wrong with that patch. If there had been anything where I actually couldn't accept it, I'd have mentioned all of those things, but nothing else. For something which is pretty good, one thing to change and in general, I'm not a fan of ternary operators because I think they're hard to read, I probably would say one thing and offer that extra feedback.

Otherwise, I'll just fix it as it comes past. So it's about finding the appropriate feedback that gets the patch accepted, but that also gives that contributor or that colleague what they need to work with you. I spend a lot of time at Toastmasters,

which is a public speaking organisation, helps you to improve your skills. So you give a talk and then you get some feedback from the evaluator and they use an approach called the sandwich technique, where you start with something positive, the filling of the sandwich is something that you can improve and you go on to end

with more bread on a high note. And I think this is something that we could do in open source because we don't do very well at saying thank you and we could start with thanks for offering me this pull request, thanks for taking the time for this. Quite like it if it actually worked.

Looking forward to seeing more code, I don't know. There's also something called the modified sandwich, which again starts really positive, mentions what isn't working and then gives constructive advice on how to change that. Your commit messages are awful. Please read that link that I mentioned earlier.

So thinking about the feedback that we give, sometimes you'll have to deal with harsh feedback. This will happen. More likely, a lot of people don't offer patches because they think they're going to get flamed. It's actually massively more likely that you will get nothing at all.

There's an awful lot of abandoned pull requests on the internet. If you get harsh feedback, it isn't personal. It's about your code, it's not about you. But also, take your efforts somewhere else.

Open source has a place where you are wanted, has a place where you are appreciated, it has a place where you belong and if you haven't found it, then your open source journey will continue until you find a project or an organisation or a group or anything that you can contribute to

and which don't make you feel victimised. Harsh feedback. The official advice is always grow a thicker skin and I don't think that's fair. Find a place where you feel comfortable offering your changes and interacting with that community.

Pull requests are not the throwing of code over the hedge. It's done, here you go. It's the beginning of a conversation. They have a life cycle, you may need to iterate, other people may contribute before this feature is finished but by starting the conversation, you become a contributor.

To get your patch accepted, you only need to remember the three main ingredients. Get your code correct, complete, test it, make sure it's ready. Write me some words to go with it. Explain to me why I care.

Remember my perspective as a maintainer or as a development lead. My colleagues are very much getting to know the things that I am going to reject without even really looking at them. Get to know that, look at the roadmap of the project, think if it fits. And with these three things, you will get your patch accepted.

Yeah, we have about seven minutes for questions. Yeah, first, thank you from our side. It was a great talk. Thank you.

Yes, there's a question that come over to you. Microphone coming. I'm so used to being loud. So I appreciate, see, there we go, yeah, very nice. So I appreciated your feedback about rejecting patches if there's an apology in the commit message but I'm wondering how you balance that out with first-time contributors who may have imposter syndrome

who are going to apologize for breathing let alone for the quality of their patch and what advice you would give people in that situation? That's a really good question. I do see a lot of first-time contributors. Sometimes I don't actually close it, I just mock them in a really friendly way. I try and let them know that they don't need to apologize.

Sometimes I'll edit that message and just try and make it a bit more factual around what's actually happening because there isn't a problem. But yeah, I still try and take it on its merits. My colleagues, however, yes, will get their patch rejected with a commit message that apologizes.

I think you forgot something. Most important in the code is checking licenses, that you don't use licenses that will violate other licenses and so on so that the developer has to check licenses

before you use libraries that aren't allowed. Okay, yeah, that's a really good point. Do you hold it? Yep. You said documentation is a must-have, but you also said that you don't want comments in the code.

So where do you want to have the documentation? Okay, I do want comments in code. I don't want commented code. So I don't want code that isn't operational, that's commented out. I do always want comments in code, and I normally like to have additional documentation. There might be API documentation, for example, or user documentation that would also need updating.

So that needs to be considered if you're changing a feature. I saw two more points.

What you see outside US is lots of code where comments and variable names are in different languages. So OpenOffice spent years in changing all German and Russian comments into English. I think that's very important, use just one language.

He's laughing, he knows it. I forgot to say. And it's very important for the project itself that they have very strict coding guideline,

coding style guideline. I'm from PostgreSQL. You get a patch rejected when the layout of the code is not okay. So and I think it's very important that you really reject code where the tab not has four spaces, eight spaces, and so. That's why I mentioned the GitHub status API.

So we have that automated that you submit the pull request, the builds run. If it's not compliant with the style guide, it just projects it. So and that's one less piece of bad news that I don't need to deliver myself. I have on my slide, because if it's not on my slide, my brain is so empty right now, I would never remember. I'm speaking in the PHP room tomorrow about PHP 7. So feel free to pop in and see me.

It is really early though, sorry. Anybody else? Well, we have some mates here in this room. Just to mention, it's quite funny that only women are speaking at the moment. Anyway, I like it. And I think it's a good thing to have a talk like this

because a lot of people don't know why their stuff is rejected and how to get it improved. So thank you for this talk, Lorraine. And yes, please give her a nice applause.