How To Get Your Patch Accepted
CC Attribution 3.0 Unported:
You are free to use, adapt and copy, distribute and transmit the work or content in adapted or unchanged form for any legal purpose as long as the work is attributed to the author in the manner specified by the author or licensor.
Mitchell, Lorna
Open source is built on volunteer hours. Using best practice in creating and reviewing patches means we're making the most of those volunteer hours - both from contributors but also from those overworked maintainers. Lorna Mitchell
 Hi, good morning. My name is, 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, you know, 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 leads 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 it's 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? Like, 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 have to do a lot of different 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 it is a problem. I like to think of a code review more like a scoring system. 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 ten points. If it's related to more than one ticket, you get minus three for each ticket it's related to. If there's two related tickets, they're kind of the same 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. Just save 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 coming on the master or your main branch adapt as appropriate since you started your branch. So instead try this. Get 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 tool. Get 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. Get diff everywhere you have a command that is get diff, if you can replace it with get diff tool or one word. And that allows you to start to use cleverer tools than just the inline red, green line diff that 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. It gives me a nice view of my 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 has made them 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-maintaining was with me at the same conference. There were eight open pool 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. 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 shit 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. I have two hate comments. Let's talk about the two mainstream ones that I see all the time. Really exotic code with no explanation. 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 my code base. 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 has been around for a while. I 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. If you comment at the beginning of the line, I basically can't see it when it's invented and I rejected a pull request because I thought it was nonsense recently and it was because there 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. So I popped in and there was a pull request waiting, client very urgent, we contacted the client so we're just waiting for law and 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 specialize 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 think 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. It was to do with the type of file that he was sending with an email. 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 you will 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, must the branch be called something completely? Should it be rebased or squashed or any of those other things? It is also worth checking in directly with the project maintainers. Most of them have chat channels or mailing lists or there will be conversation going on the issues if you are out of time zone or they are just not real time people. And it is worth talking to them, just taking their time to let them know what you are 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 half way back in the room. Hi. I can always break your code. I think my developers at work, the people that I work with think 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 is not magic and it is not even rocket science. It is not hard. I do things like is the data missing or is it overlapping or is it a little bit ambiguous because you have allowed me to edit it in too many different ways and I have deliberately tried to confuse the system or omit something. I will sometimes log in as a different user. That seems to throw everybody, too many developers building things logged in as the admin user and 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 the poor 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. It is never a good thing. Quite an angry QA person. And she said I am very angry. I cannot break your code. She is a QA person. It is 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 is a threat. I think that is a compliment because I have always tried to think about what could go wrong and that is something which will enormously help. Both to get your patch accepted if you are contributing to a project but just to keep things moving at work to get features shipped if you are 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. Joindyn 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. Guess what? He committed a change and I reviewed and merged the change and between us we broke the live platform. Well done. Now I have a syntax check that runs on build. GitHub has this commit API, commit status API. You open a pull request and it will run the builds for you. It integrates with Travis. Really helpful. This is fabulous because it means bad news like your code might be fine but you are in contravention of our coding standards therefore we won't accept your patch. That is why we are doing this. This gets delivered nice and early. Like the syntax check. That is worth having and then you know. Half the time it is conflict marker that somehow made it into a file. One thing to really look out for and it is 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 archaeology. I work on legacy systems so things like that. I dig about to try and understand how on earth we ended up where we are today. This matters. This does matter. If you get to the end of your branch and it is 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 un-staged change. Now you can make me some new commits from all that change that you just put in on the branch. It was a single commit and I complained or it was a bit of a long journey of commits that went a bit round the house. Sometimes we can't tell at the start of a feature quite how that is going to look when we are finished. If it was all very straightforward and we knew exactly what we would do, none of us would be doing it because we would be bored. We all need the challenge and the entertainment. Sometimes can be a very scenic journey to eventually arrive at the working solution. So clean it up. While I am 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 use my slides. I work with a 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 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 not 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, I will still have no idea why that happened. And that's why this matters. Don't tell me what's in the Diff. I'm going to read the Diff anyway. Every commit has the apparent, the Diff, who did it, when they did it. The 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 going to 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 slapped. You are the stranger reading your own commit messages and hating yourself. Do yourself a favor. 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 described 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 do for a sane format and it's here. This article is so good that you've just have 20 minutes of rant. You can all just read the article. To summarize, a headline that isn't very long, which is going to fit on Git log with one line, it's going to 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 want to 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 need just 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 apologize. 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 apologize, 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 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, web page. I don't have a good relationship with the green button. But anyway, there isn't one because it's going to conflict. So it's worth checking if it will merge. Now, you can check by just opening 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 going to be any 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, and then do get merge, get your repo out of the merging status and back to being normal, it's as if nothing happened at this point. If it doesn't merge, then fix it. And that might be, there's a couple of ways you can do that. Some projects are happy if you 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 developer, 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 going to see it somewhere along your rebasing journey. I've seen some situations where there's actually a lot of conflicts along the rebase journey because you're applying, 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. If 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 mainline development branch is. So usually it will default correctly, but it's worth a check. And then scroll down and 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 realize that you branched from completely the wrong place or something else terrible has happened. And 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 opened 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 is on the list. I want you to 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. It'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 cash function. This is a bit like implements the dancing bear feature. Wasn't really on my roadmap. Dancing bear. Got to maintain it. This is the get cash function. I'm thinking about that. And then thinking, well, it's come from a co-maintenor, actually. It's not even a trusted contributor. This came from someone who definitely should have known better. So in the diff, there's a new get cash function and inside the get cash function, there's code that looks like cash code and nothing else. It's not called from anywhere. I have literally got a commit that implements a get cash function. I really could. This does not help. Write me some words. What had actually happened was we had somehow eliminated the get cash 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 cash function, which is what he had done. But later night in a huge hurry and then it opened me a pull request, which did not 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 there. I said 27,000 one compatible and they like lots and lots and lots of paperwork. And it's really interesting because it means 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 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 measure 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 response. All the questions lies there in limbo for ever. 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 there's it changes a bit depending on what exactly you're reviewing and what the context is, but not a lot. I've been an open source maintainer now for a number of years and I was contributed before that and still a contributor to a bunch of other projects that we 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 code reviewer because sometimes that's my perspective to 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 to 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 with a 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 requested a copy change. Okay, so viewed the diff and the diff had quite a complicated SQL statement in it. 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 is. 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. Because I had always 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'd 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. That's a tiny grammatical point. Please don't merge someone's poor request. Please review it. I want you to keep that attitude of skepticism right until you can't prove that it shouldn't be it shouldn't be accepted. This is really important. I come to work and people say I need 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 is a really small company. The same junior developer said I'm not even sure this this merits a merger quest. But we made these rules for a reason. So I will as I was like good man. So I opened this measure question. 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. Like this piece and then there was a typo and then oh my goodness 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 accept things and then it probably gets to the test platform and I realize 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 conference organizer 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-maintenance. This is a mistake. One piece of advice do not travel with your co-maintenance. I once had the other co-maintenance and the only other person with server access all on the same transatlantic flight to Oscar with me. I just logged into channel and says if anything breaks you need to wait 24 hours and we're on this flight. I go sorry. Look out for the flight number of anything terrible happens. I mean anyway so I go to San Francisco with Rob and I woke up at 5 am local to emails Twitter stream everything about some users being unable to log in on joined in. So Robin I was sharing an apartment and I very optimistically logged into ISE and he was not obviously awake because he was not logged in. I was 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 in a channel and I was like I see a problem I see a poor request where we are 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 under 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 read me. 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. On this occasion I click the green button and then I click the go button on Jenkins and then we're back to bed. Because somebody else had done that code review. And if you want a feature. Accepted tested come back and say hey I really liked it worked for me on this platform yada yada. I am slowly learning. I need to check the pull request queue. Before I open them a pull request because half the time the thing I'm going to fix has already been fixed but sometimes it's still sitting in there for a question. If that happens pull that branch by all means but test it and feed back on that pull request said hey I tried it really looks awesome to me. On this version on my platform etc. 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. My developers have stopped saying to me I wasn't my code. 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 death and maybe the death looks good. But it is the things that are not in the death 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. I'll add you 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 they missing database patches does somebody make a local database change that we need to make this code work and it's not in the patch maybe because it's a new file maybe they forgot to add it that happens. Deploy ability 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 on your platform okay works for me. Is not how we ship software. Well works for me is really good starting point but works for me doesn't do the rest of it has to run on your death platform but then on my death platform and on the test platform and on the live platform. And you have to be able to see that I did recently which came in which change the conflict and it's like you just change 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 only runs on one platform the dev platform that's going to 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 remember to change the templates reporting systems 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 like beautiful and amazing refactors and then we update their emails and all their templates the website looks great and then they're reporting breaks at the end of the month. You've got to be able to see that this change is not in your death. Monitoring changes this is sometimes invisible to developers. I'm in the middle of doing a big refactor of game and workers because we're running like 20 different ones and. I'm going to group them up so that 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 game and jobs are running at all times. So more things to bear in mind cron jobs can be really hard to remember especially the conflict 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 reject. 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 dancing bear feature because you don't want it. Close it reject it early. It's kinder to everybody just 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 really important to include constructive feedback. I had one situation where a new employee had had you know he was really new so is poor request never got accepted first time in fact or third time or fifth time right. So he had quite a long 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 request and I merged it and I was on my comment back to him was like it was cool but I really didn't like the comment syntax that you use the C comment syntax is valid in PHP what we typically use to slash. Came straight back to me and 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 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. They are very much more like it 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 is a lot of first time open source contributors. If I know those people 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 and 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 toast masters which is a public speaking organization 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 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 for request thanks for taking the time for this. Quite like it if it actually worked and 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 organization or a group or anything that you can contribute to which don't make you feel victimized. 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 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 road map of the project think of it fits. And with these three things you will get your patch accepted. Thank you. Thank you. I appreciate your feedback about rejecting patches if there is an apology in the commit message. I wonder 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 would you 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 let it 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 license that will violate other licenses and so on. So that the developer has to check license before you use libraries that are allowed. Okay, yeah, that's a really good point. Do you hold it? 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? I do want commenting code. I don't want commented code. So I don't want code that isn't operational that's commented out. I do want 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. Any more questions? I saw two more points. What you see in outside US is lots of code where comments and variable names are in different languages. So OpenOffice spend years in changing all German and Russian comments into English. I think that's very important. Use just one language. And 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 I think it's very important that you really reject code where the tab not has four spaces, eight spaces. 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 rejects it. 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 mails 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, Loreen. Please give her a nice applause.
 Hi, good morning. My name is, 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, you know, 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 leads 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 it's 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? Like, 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  have to do a lot of different 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  it is a problem. I like to think of a code review more like a scoring system. 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 ten points. If it's related to more than one ticket, you get  minus three for each ticket it's related to. If there's two related tickets, they're kind of the same  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. Just save 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 coming on the master or your main branch adapt as  appropriate since you started your branch. So instead try this. Get 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 tool. Get 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. Get diff everywhere you have a command that is get diff,  if you can replace it with get diff tool or one word. And that allows you to start to use cleverer tools  than just the inline red, green line diff that 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. It gives me a nice  view of my 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 has made them 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-maintaining was with me at the same conference. There were eight  open pool 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. 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 shit 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. I have two hate comments. Let's talk about the two mainstream ones  that I see all the time. Really exotic code with no explanation. 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 my code base. 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 has been around for a while. I 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. If you comment at the beginning of the line, I basically can't see it when it's  invented and I rejected a pull request because I thought it was nonsense recently and it was because there 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. So I popped in and there was a pull request waiting, client very urgent, we contacted the client so we're just  waiting for law and 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 specialize 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 think 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. It was to do with the type of file that  he was sending with an email. 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  you will 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, must the branch be called something completely? Should it be rebased or squashed or any of those other things?  It is also worth checking in directly with the project maintainers. Most of them have chat channels or mailing lists or there  will be conversation going on the issues if you are out of time zone or they are just not real time people. And it is worth  talking to them, just taking their time to let them know what you are 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 half way back in the room. Hi. I can always break your code. I think my  developers at work, the people that I work with think 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 is not magic and it is  not even rocket science. It is not hard. I do things like is the data missing or is it overlapping or is it a little bit  ambiguous because you have allowed me to edit it in too many different ways and I have deliberately tried to confuse the  system or omit something. I will sometimes log in as a different user. That seems to throw everybody, too many  developers building things logged in as the admin user and 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 the poor 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. It is never a good thing. Quite an angry QA person. And she said I am very angry. I cannot break your code.  She is a QA person. It is 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 is a threat. I think that is a compliment because I have  always tried to think about what could go wrong and that is something which will enormously help. Both to get your patch  accepted if you are contributing to a project but just to keep things moving at work to get features shipped if you are  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. Joindyn 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.  Guess what? He committed a change and I reviewed and merged the change and between us we broke the live platform. Well done.  Now I have a syntax check that runs on build. GitHub has this commit API, commit status API. You open a pull request and it  will run the builds for you. It integrates with Travis. Really helpful. This is fabulous because it means bad news like  your code might be fine but you are in contravention of our coding standards therefore we won't accept your patch. That  is why we are doing this. This gets delivered nice and early. Like the syntax check. That is worth having and then you  know. Half the time it is conflict marker that somehow made it into a file. One thing to really look out for and it is  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 archaeology. I work on legacy systems so things  like that. I dig about to try and understand how on earth we ended up where we are today. This matters. This does matter. If you  get to the end of your branch and it is 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 un-staged change. Now you can  make me some new commits from all that change that you just put in on the branch. It was a single commit and I complained  or it was a bit of a long journey of commits that went a bit round the house. Sometimes we can't tell at the start of a  feature quite how that is going to look when we are finished. If it was all very straightforward and we knew exactly what  we would do, none of us would be doing it because we would be bored. We all need the challenge and the entertainment. Sometimes  can be a very scenic journey to eventually arrive at the working solution. So clean it up. While I am 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  use my slides. I work with a 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 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  not 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, I will still have no idea why that  happened. And that's why this matters. Don't tell me what's in the Diff. I'm going to read the Diff anyway. Every commit has  the apparent, the Diff, who did it, when they did it. The 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 going to 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 slapped. You are the stranger reading your own commit messages and hating yourself. Do yourself a favor. 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 described 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 do for a sane format and it's here. This article is so good that you've  just have 20 minutes of rant. You can all just read the article. To summarize, a headline that isn't very long, which is  going to fit on Git log with one line, it's going to 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 want to 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 need just 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 apologize. 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 apologize, 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 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, web page. I don't have a good relationship with the green button. But anyway, there isn't one  because it's going to conflict. So it's worth checking if it will merge. Now, you can check by just opening 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 going to be  any 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, and then do get merge,  get your repo out of the merging status and back to being normal, it's as if nothing happened at this point. If it  doesn't merge, then fix it. And that might be, there's a couple of ways you can do that. Some projects are happy if you  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 developer, 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 going to see it somewhere along your rebasing journey.  I've seen some situations where there's actually a lot of conflicts along the rebase journey because you're applying, 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.  If 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 mainline development branch is. So usually it will default correctly, but it's worth a check.  And then scroll down and 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 realize that you branched from completely the wrong place or something else terrible has happened.  And 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 opened 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 is on the list.  I want you to 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. It'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 cash function.  This is a bit like implements the dancing bear feature.  Wasn't really on my roadmap. Dancing bear. Got to maintain it.  This is the get cash function. I'm thinking about that. And then thinking, well, it's come from a co-maintenor, actually. It's not even a trusted contributor.  This came from someone who definitely should have known better.  So in the diff, there's a new get cash function and inside the get cash function, there's code that looks like cash code and nothing else.  It's not called from anywhere. I have literally got a commit that implements a get cash function.  I really could. This does not help. Write me some words.  What had actually happened was we had somehow eliminated the get cash 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 cash function, which is what he had done.  But later night in a huge hurry and then it opened me a pull request, which did not 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 there. I said 27,000 one compatible and they like lots and lots and lots of paperwork.  And it's really interesting because it means 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 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 measure 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 response. All the questions lies there in limbo for ever.  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 there's it changes a bit depending on what exactly you're reviewing and what the context is, but not a lot.  I've been an open source maintainer now for a number of years and I was contributed before that and still a contributor to a bunch of other projects that we 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 code reviewer because sometimes that's my perspective to 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 to 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 with a 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 requested a copy change.  Okay, so viewed the diff and the diff had quite a complicated SQL statement in it.  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 is.  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.  Because I had always 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'd 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.  That's a tiny grammatical point. Please don't merge someone's poor request. Please review it. I want you to keep that attitude of skepticism right until you can't prove that it shouldn't be it shouldn't be accepted.  This is really important.  I come to work and people say I need 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 is a really small company.  The same junior developer said I'm not even sure this this merits a merger quest.  But we made these rules for a reason.  So I will as I was like good man.  So I opened this measure question.  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.  Like this piece and then there was a typo and then oh my goodness 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 accept things and then it probably gets to the test platform and I realize 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 conference organizer 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-maintenance.  This is a mistake.  One piece of advice do not travel with your co-maintenance.  I once had the other co-maintenance and the only other person with server access all on the same transatlantic flight to Oscar with me.  I just logged into channel and says if anything breaks you need to wait 24 hours and we're on this flight.  I go sorry.  Look out for the flight number of anything terrible happens.  I mean anyway so I go to San Francisco with Rob and I woke up at 5 am local to emails Twitter stream everything about some users being unable to log in on joined in.  So Robin I was sharing an apartment and I very optimistically logged into ISE and he was not obviously awake because he was not logged in.  I was 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 in a channel and I was like I see a problem I see a poor request where we are 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 under 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 read me.  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.  On this occasion I click the green button and then I click the go button on Jenkins and then we're back to bed.  Because somebody else had done that code review.  And if you want a feature.  Accepted tested come back and say hey I really liked it worked for me on this platform yada yada.  I am slowly learning.  I need to check the pull request queue.  Before I open them a pull request because half the time the thing I'm going to fix has already been fixed but sometimes it's still sitting in there for a question.  If that happens pull that branch by all means but test it and feed back on that pull request said hey I tried it really looks awesome to me.  On this version on my platform etc.  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.  My developers have stopped saying to me I wasn't my code.  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 death and maybe the death looks good.  But it is the things that are not in the death 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.  I'll add you 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 they missing database patches does somebody make a local database change that we need to make this code work and it's not in the patch maybe because it's a new file maybe they forgot to add it that happens.  Deploy ability 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 on your platform okay works for me.  Is not how we ship software.  Well works for me is really good starting point but works for me doesn't do the rest of it has to run on your death platform but then on my death platform and on the test platform and on the live platform.  And you have to be able to see that I did recently which came in which change the conflict and it's like you just change 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 only runs on one platform the dev platform that's going to 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 remember to change the templates reporting systems 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 like beautiful and amazing refactors and then we update their emails and all their templates the website looks great and then they're reporting breaks at the end of the month.  You've got to be able to see that this change is not in your death.  Monitoring changes this is sometimes invisible to developers.  I'm in the middle of doing a big refactor of game and workers because we're running like 20 different ones and.  I'm going to group them up so that 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 game and jobs are running at all times.  So more things to bear in mind cron jobs can be really hard to remember especially the conflict 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 reject.  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 dancing bear feature because you don't want it.  Close it reject it early.  It's kinder to everybody just 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 really important to include constructive feedback.  I had one situation where a new employee had had you know he was really new so is poor request never got accepted first time in fact or third time or fifth time right.  So he had quite a long 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 request and I merged it and I was on my comment back to him was like it was cool but I really didn't like the comment syntax that you use the C comment syntax is valid in PHP what we typically use to slash.  Came straight back to me and 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 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.  They are very much more like it 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 is a lot of first time open source contributors.  If I know those people 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 and 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 toast masters which is a public speaking organization 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 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 for request thanks for taking the time for this.  Quite like it if it actually worked and 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 organization or a group or anything that you can contribute to which don't make you feel victimized.  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 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 road map of the project think of it fits.  And with these three things you will get your patch accepted.  Thank you.  Thank you.  I appreciate your feedback about rejecting patches if there is an apology in the commit message.  I wonder 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 would you 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 let it 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 license that will violate other licenses and so on.  So that the developer has to check license before you use libraries that are allowed.  Okay, yeah, that's a really good point.  Do you hold it?  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?  I do want commenting code. I don't want commented code.  So I don't want code that isn't operational that's commented out.  I do want 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.  Any more questions?  I saw two more points.  What you see in outside US is lots of code where comments and variable names are in different languages.  So OpenOffice spend years in changing all German and Russian comments into English.  I think that's very important. Use just one language.  And 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 I think it's very important that you really reject code where the tab not has four spaces, eight spaces.  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 rejects it.  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 mails 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, Loreen.  Please give her a nice applause.
Lorna_Mitchell__How_To_Get_Your_Patch_Accepted_0_47.png Lorna_Mitchell__How_To_Get_Your_Patch_Accepted_48_95.png Lorna_Mitchell__How_To_Get_Your_Patch_Accepted_96_155.png Lorna_Mitchell__How_To_Get_Your_Patch_Accepted_156_203.png Lorna_Mitchell__How_To_Get_Your_Patch_Accepted_204_419.png Lorna_Mitchell__How_To_Get_Your_Patch_Accepted_420_2975.png Lorna_Mitchell__How_To_Get_Your_Patch_Accepted_2976_3059.png Lorna_Mitchell__How_To_Get_Your_Patch_Accepted_3060_12455.png Lorna_Mitchell__How_To_Get_Your_Patch_Accepted_12456_15659.png Lorna_Mitchell__How_To_Get_Your_Patch_Accepted_15660_16547.png Lorna_Mitchell__How_To_Get_Your_Patch_Accepted_16548_17591.png Lorna_Mitchell__How_To_Get_Your_Patch_Accepted_17592_27263.png Lorna_Mitchell__How_To_Get_Your_Patch_Accepted_27264_28199.png Lorna_Mitchell__How_To_Get_Your_Patch_Accepted_28200_29495.png Lorna_Mitchell__How_To_Get_Your_Patch_Accepted_29496_30071.png Lorna_Mitchell__How_To_Get_Your_Patch_Accepted_30072_31583.png Lorna_Mitchell__How_To_Get_Your_Patch_Accepted_31584_31691.png Lorna_Mitchell__How_To_Get_Your_Patch_Accepted_31692_33647.png Lorna_Mitchell__How_To_Get_Your_Patch_Accepted_33648_35099.png Lorna_Mitchell__How_To_Get_Your_Patch_Accepted_35100_36863.png Lorna_Mitchell__How_To_Get_Your_Patch_Accepted_36864_36911.png Lorna_Mitchell__How_To_Get_Your_Patch_Accepted_36912_37247.png Lorna_Mitchell__How_To_Get_Your_Patch_Accepted_37248_62663.png Lorna_Mitchell__How_To_Get_Your_Patch_Accepted_62664_63647.png Lorna_Mitchell__How_To_Get_Your_Patch_Accepted_63648_68183.png Lorna_Mitchell__How_To_Get_Your_Patch_Accepted_68184_69539.png Lorna_Mitchell__How_To_Get_Your_Patch_Accepted_69540_69587.png Lorna_Mitchell__How_To_Get_Your_Patch_Accepted_69588_78035.png Lorna_Mitchell__How_To_Get_Your_Patch_Accepted_78036_78539.png Lorna_Mitchell__How_To_Get_Your_Patch_Accepted_78540_79679.png Lorna_Mitchell__How_To_Get_Your_Patch_Accepted_79680_80075.png Lorna_Mitchell__How_To_Get_Your_Patch_Accepted_80076_80123.png Lorna_Mitchell__How_To_Get_Your_Patch_Accepted_80124_80243.png Lorna_Mitchell__How_To_Get_Your_Patch_Accepted_80244_80723.png Lorna_Mitchell__How_To_Get_Your_Patch_Accepted_80724_80783.png Lorna_Mitchell__How_To_Get_Your_Patch_Accepted_80784_80831.png Lorna_Mitchell__How_To_Get_Your_Patch_Accepted_80832_80879.png Lorna_Mitchell__How_To_Get_Your_Patch_Accepted_80880_80927.png Lorna_Mitchell__How_To_Get_Your_Patch_Accepted_80928_80975.png Lorna_Mitchell__How_To_Get_Your_Patch_Accepted_80976_81047.png Lorna_Mitchell__How_To_Get_Your_Patch_Accepted_81048_81095.png Lorna_Mitchell__How_To_Get_Your_Patch_Accepted_81096_81503.png Lorna_Mitchell__How_To_Get_Your_Patch_Accepted_81504_81551.png Lorna_Mitchell__How_To_Get_Your_Patch_Accepted_81552_82151.png Lorna_Mitchell__How_To_Get_Your_Patch_Accepted_82152_82199.png Lorna_Mitchell__How_To_Get_Your_Patch_Accepted_82200_82655.png Lorna_Mitchell__How_To_Get_Your_Patch_Accepted_82656_82703.png Lorna_Mitchell__How_To_Get_Your_Patch_Accepted_82704_83147.png Lorna_Mitchell__How_To_Get_Your_Patch_Accepted_83148_83195.png Lorna_Mitchell__How_To_Get_Your_Patch_Accepted_83196_83243.png Lorna_Mitchell__How_To_Get_Your_Patch_Accepted_83244_83291.png Lorna_Mitchell__How_To_Get_Your_Patch_Accepted_83292_83339.png Lorna_Mitchell__How_To_Get_Your_Patch_Accepted_83340_83459.png Lorna_Mitchell__How_To_Get_Your_Patch_Accepted_83460_83507.png Lorna_Mitchell__How_To_Get_Your_Patch_Accepted_83508_83651.png Lorna_Mitchell__How_To_Get_Your_Patch_Accepted_83652_83711.png Lorna_Mitchell__How_To_Get_Your_Patch_Accepted_83712_85055.png Lorna_Mitchell__How_To_Get_Your_Patch_Accepted_85056_85103.png Lorna_Mitchell__How_To_Get_Your_Patch_Accepted_85104_85151.png Lorna_Mitchell__How_To_Get_Your_Patch_Accepted_85152_85199.png Lorna_Mitchell__How_To_Get_Your_Patch_Accepted_85200_86423.png Lorna_Mitchell__How_To_Get_Your_Patch_Accepted_86424_86471.png Lorna_Mitchell__How_To_Get_Your_Patch_Accepted_86472_86531.png Lorna_Mitchell__How_To_Get_Your_Patch_Accepted_86532_86579.png Lorna_Mitchell__How_To_Get_Your_Patch_Accepted_86580_86627.png Lorna_Mitchell__How_To_Get_Your_Patch_Accepted_86628_86672.png