 Hi everyone, I just want to welcome Jess who started Core Mentoring, which is pretty important and has now travelled much of the world helping out at Core Sprints since starting Core Mentoring, which is pretty cool. And Andrea who helped make Core Mentoring scale, so she helped Jess organise things to make it scale and she could get other people involved. So welcome all. Hi, how's everybody going today? Good? Yeah? Yeah, there we go. So, as Brian was saying, this is Jess and she's amazing. I'm XJM, Jess. XJM, yeah. And I'm Andrea, also known as Zen Doodles, and I just started with the nerdery. So we're both kind of like doing a career change too right now, which is kind of exciting, but probably not relevant. I'm working for AQUIA the first week in March, so I'm not actually working for AQUIA yet, but we put it on the slide anyway because it's great. Yeah, it's a very awesome thing. The tall guy with the spiky hair, that's my boss now. So before we really get into details about Patch Review, we were kind of wondering how many people have actually done, like submitted a patch on Drupal.org? Awesome. Yeah? Okay, so has anyone applied patches? Yeah? Is there anyone here who doesn't even know what a patch is? Anyone? Okay, so essentially, we know a patch is a file that tells us which lines to remove and which lines to take out, right? And why do we need patches? Well, patches are how improvements are made. Some of this will recognize this particular animation thing. So what happens is some user says, oh my God, I have this really horrible problem and it makes me unhappy. So they go to the issue queue and they file a bug report, which is awesome and very helpful. But then Paula, the programmer, comes along and says, oh my God, I have this problem. Search is the issue queue because that's what Paula does. And says, oh, hey look, there's a bug report. I'll try to fix this, since it's not fixed yet. Uploads a patch, marks the issue as needs review. And then along comes Rachel, the reviewer, right? Now, those of you who are familiar with this particular slide from WebChick's presentation, she used to be Taty on the tester, you can't see it at the bottom here, but... So Rachel, the reviewer, comes along and says, oh my word, what? I'll just post some feedback and let everybody know that this is confusing and doesn't work. So Paula comes back and says, alright, thanks, here's another trial. Let me try again. This poor soul, Wendy, is still on Windows XP. And so she has to post in and says, you know what, this breaks everything in IA6 because everything does. Also, mind your spelling. Marks the issue again as needs work. So who comes back? Paula, the programmer and says, alright, try this. Rachel, the reviewer comes in, has a look, much better. Marks the issue is reviewed and tested, and then there's a magic thing that happens where someone comes in and actually, hopefully, commits the code that was in the patch. So what kinds of patch reviews do we do in Drupal before things go in court? Does anybody have ideas on this? Anyone? That is the first thing that reviews the code, the patches. That's the bot, the test bot, the automated testing bot, comes in. And then there's a peer co-review. That's Rachel, the reviewer, she comes in, she reviews the code and says, you know, and that's kind of what this session is about. But I just, like, that was a spoiler, so don't ignore what I just said. So Rachel reviews for accessibility, for performance, security, usability, and so on. And then, of course, whoever's going to do the committing does another review toward the end. But not all of these are important. Right now, all we're going to talk about is, of course, peer code review. So why do we review code? We review code because we need to make sure that some coding standards are followed. We want to know that people who are submitting patches and people who are reviewing patches can submit good patches in the future, too, right? That's a good thing. So it's an education and mentoring opportunity. And then we also want to make sure that we have a quality end product. The coding standards that we review for will be security, maintainability, and documentation, among other things. And then also, again, education, we want to mentor the next generation. We want to make sure the polyprogrammer can upload good patches so we have good code. And we want to make sure that Rachel, the reviewer, can learn a bunch of stuff, too, because it's a very good learning opportunity for that. And also, we want to make sure that Drupal still rocks, right? So what's important, you have to, like, if I, like, keep going over where you're going to... Yeah, that's fine. I'll just grab you by the shoulders. Exactly. That works. Yeah. Okay, so what's important when reviewing? Sorry. The most important thing, this is my opinion, and obviously there are going to be people who disagree, but is something that Jess coined as full frontal nicety. That was you, right? It was, I think it was actually Angie who said it first, but it's my opinion. It's her thing. So we want to make sure that in the issue queue that we are encouraging, especially new developers, but also even old developers. Well, when I say old, I mean, not I'm old. Experienced developers. Exactly. We want to make sure that we're encouraging mature, experienced developers. And also the new developers. We want to support improvement, and we want to make sure that we ask questions if we have them. And I'm actually just going to sort of jump in here and say that I really think this is the singly, most important part of a patch review, because this is your opportunity to reach out to someone. This might be the first time they've ever submitted a patch. It's your chance to build a relationship with someone. If they have a bad experience the first time, they're not going to come back. If you're rude, it's like, why would I waste my time doing this? But if you're positive, then they learn something from you. You can learn something from them, and you've gotten to a point where it's a positive experience, and so they'll come back and submit another patch in the future. Absolutely. Which is why when I talk about encouragement, when we talk about encouragement, some of the important things we need to acknowledge that the coder who has submitted a patch has put some effort into this. They had to think about the problem. They had to define it. They had to consider a solution. They had to solve the problem, and then they also had to upload a patch. It's so much easier for people just to fix it in my environment and move along. People have made a lot of effort when they upload a patch. There's a lot of time and effort that goes into that. Emotion. We need to make sure that we're acknowledging their effort. We're showing gratitude for their effort. Even if it is not code that we want to have in Drupal, we want to take the opportunity to acknowledge that what they're doing is effort on their part and be happy that they've done it. If it's not something that we want in Drupal, we want to support improvement. We want to give them pointers on what we need from them next time. The next time they find a problem, or the next time they upload a new patch. Remember that Paula had to upload three patches in that initial issue before she got something that was committed. We need to be specific about what changes to make, and we need to provide links, if there are, to documentation that shows why we do what we do and what the good things are about it. With regard to providing documentation, that's something you might not think about, but that's really important for that. Once upon a time, I was reviewing this really cool patch that someone in this room submitted. He had gotten a review from someone else who told him to do the wrong thing. There was a back and forth with two different people who were speaking authoritatively in this issue, one of them me telling him to do the right thing and another person telling him to do the wrong thing. That's got to be really frustrating. If either of us at the first point had taken the time to look up the documentation and say, oh, right here it says this, and then put that link in the issue, that solves the question. There's no doubt in anyone's mind in the future. That also gets people thinking about, okay, well, I can look on this page in the handbook in the future if I need to make a coding standards change, and I can look at how to write queries properly. That gets them in the mode of looking in the issue when they're creating patches in the future. It's a chance for you to help programmers program for Drupal better. Sorry. I'm not wearing shoes. Again, be specific, provide links to documentation when you can and support improvement in any other way that you can. On IRC, you can give pointers. Even sometimes, even just asking questions is supporting improvement. Because what you do when you ask a question is you make that person think about the reasons for things that maybe aren't the way that you want them to, or even the reasons for things that are awesome about their patch, right? So do everything you can and think about the patch review as an opportunity to educate again. This is a good place to go. Okay. So one of the things that we review for, it is a brand new slide deck, sparkling new for Down Under. Anyway, one of the first things that we look for, of course, the full final niceties, the most important, but one of the things that we look for when we're actually looking at the patch is does the patch actually work? Does it do what it's supposed to do? Now, obviously, the bot checks for that, but there are other things that we need to check for. Does it fix the issue? Does it fix the issue and not fix other issues or not break other things? So are we staying true to scope? A lot of times a patch will have... It will fix something and then maybe an editor will automatically change white spaces or something strange like that, which is completely out of scope, and white spaces may be a bad example. But it is actually a good example. That's why we have white space standards is because when white space... it adds noise to the patch and it's very frustrating for... you as a patch reviewer, it's very frustrating for the core committer when there's all these extra lines that have nothing to do with the change they're trying to review in the patch. So that's actually why we have coding standards to make sure that simple things like that are always consistent so they don't add absolutely. And we again want to make sure that we don't introduce regressions. We want to make sure that the patch doesn't break other things. We also check to make sure the patch makes sense. Is it readable? Do I understand what's going on? Do I understand like the rationale of behind what's going on? Are there enough comments? Is it documented well enough that even if a person is not familiar with the issue when they come in and they read the code directly because I mean when you go and look at the code there's not an issue number next to every line on the code. So if you're not familiar with the issue does the code make sense? Can you understand it? Are there enough comments to do that? And also this is a little bit dangerous but is there a better way? Can we do this more elegantly? Can we do it just better in any way? Can we reuse other APIs instead of creating new custom code? Yes. Things like that. So we want to kind of look and think about is there a better way to solve this problem? But again we have to keep in mind that when we're saying that there's a better way if we do find a better way that we are also supporting improvement by telling a person this is just wrong. Your approach is horrible or something like that which actually happens and so we want to check that. We should put a thereby dragon. You had that in and it went away. Or you could just put a picture of a dragon and this is actually something to keep in mind if you think there's a better way pose it as a suggestion I think maybe not this is wrong do this and exactly have you thought of doing it this way and that way that also gives you an opportunity to find out maybe your better way isn't better or maybe there's a third person, someone who has more experience who can give you was a testament. So I'm going to talk a little bit about something that really sets core apart from the contrib space in terms of like the patch submission process which is the core gates. So these are five gates. There's the usability gate, accessibility, documentation, performance and testing. And each is sort of a checklist of the minimum standards we have for core in each of these specific areas. They were first introduced at Drupal Con Chicago by Dries so about two years ago now and that was also right at the beginning of the Drupal 8 development cycle. So I would say that they've actually had a pretty profound impact on the way that Drupal 8 development has happened. They've increased our velocity and they've made it so that we could scale to a lot more contributors than Drupal 6. I think we ended up with like a thousand people at the end of the release cycle. Drupal 8 has already passed that and it's not going to be released for eight months a year. So we wouldn't be able to keep growing the number of contributors without having these sets of things. So to get the specifics this URL at the bottom of the screen not too hard to remember www.drupal.org I definitely recommend reading this page. It will have specific instructions in each of these specific areas about the things you really should know about the things that you as a reviewer can check to make the patch review process more distributed and to make Drupal's quality higher. How it used to work is that so there was like the usability team and the accessibility team and they were the only ones who really like took responsibility for it and the gates distribute that responsibility to developers and they also they also make it to like maybe we don't need to focus on these really really nitpicky things let's focus first on the things that are most important to make Corbetta. Yeah but okay so thank you that is what I was going to do next. So the first gate that we have up here is the documentation gate and this is something that a lot of programmers in particular tend to overlook but it's also very important and if you've worked in if you've worked with a number of other developers it's even more important. The first thing up here you might not be familiar with is issue summaries for core issues. Some of the points on this slide are things that you put in the code base in the patch itself and other things are things that go on Drupal.org and what the issue summary is is the initial post when you first created Drupal issue is actually editable now so you can edit it like a wiki page and keep it updated so it always reflects what's currently going on in the issue. Someone posts a new patch there's this problem with it you put that in the summary and looks at the patch and says you know actually there's this really really bad bug that we need to solve first you put that in the summary and say look at this other issue first and we'll postpone the second issue. This is actually how I learned core is by writing issue summaries for issues so it's actually a really really great way to do your own professional development increase your own skill set and at the same time you're providing a service to everyone else because if you take the time to fill out the issue summary and keep it up to date you've just saved every other person who ever looks at that issue all the time that you took so it's really important and it is actually required for issues like a core maintainer can at any time say okay this issue is just too complicated I need someone to write an issue summary for this issue before I'll commit this patch but we do recommend it for pretty much any issue especially any issue that becomes long or complicated today go to an issue and click the edit tab and there's instructions there it's great the second point up here the second part of the documentation gate is the in code API documentation so this includes the documentation blocks that are at the top of every function every class every constant and there's a set of standards for them that are really there's this huge document it's one of the oldest nodes on Drupal.org it's 1354 if you haven't if you're not familiar with that document you'll look at it and then you'll wish you weren't familiar with that document it's very long it's very specific but what the core gates page does is it summarizes the bare minimum things that we have to have in the patch in order for it to be accepted so that's a really great place to start if you want to learn how to document code this is used on api.drupal.org it's used by people's IDEs to describe what what parameters are available for functions and so forth another part of the code base that's used in both those places well it's used on api.drupal.org is the module.api.php file now and this is the first example of something that's not relevant in every case you only need to make a change to the module.api.php for a module when there's a change to the hook so if we add a new hook if we remove a hook we change its parameters then that needs to be changed in that file and api.drupal.org uses that to document what the hook is and the core gates page actually has an entire column that specifies specifically when is this necessary when you need to learn about it so bookmark that page put it on your dashboard or something the fourth change updating the hook.help again this is only relevant when you need to when you add a new module it needs a help hook so that doesn't come into play too often the last thing up here and this is something that's again new in the Drupal 8 cycle is change notifications every time we make an api change in the core code base we rename a function we add a new api we create one of these change notifications that are listed on this URL here Drupal.org that lists every single thing that's been changed in Drupal 8 in the past two years so the goal of this is to make it easier for people when they need to upgrade their contributed modules and their custom code on their personal sites or their company sites it actually helps them go through and figure out what the next version is second gate performance for the performance gate is not what you think it is so be sure to read the page for this so take an example the second point up here caches contrary to what you would think caches are bad what the gate says for caches is only add a cache when you need it because caches invalidation is a really complicated problem caches make the developer experience more difficult so an example a patch that I'm going to show you in a little bit it's really a big patch this one thing in the patch made the Drupal.org front page 15 times slower we added caching in that case but in most cases if you don't already know there's a performance problem you don't add that and so there's a bunch of specific areas where you look for it the performance gate isn't about saying no we don't make Drupal slower it's about let's document where the performance regressions are so that later in the release cycle we can find a consistent way to fix them because premature optimization is bad in the performance world and just premature optimization premature optimization is really bad and the key there is make sure that you have that you either profile it yourself or you have someone do that for you and don't guess don't say I think this will make it slower because Mark Sonnenbaum will come in here and educate you if he weren't actually presenting on this right now he'd be in here shaking you already just because you were thinking about it so the what am I on next gate is the accessibility gate accessibility issues in Drupal are bugs if someone can't use Drupal it's a bug and the accessibility gate is really clearly documented additionally all of these most of these things up here have tools that you can use to test them you don't need to know anything about accessibility go read this page you'll learn something about accessibility that you can then take and use on your own sites and then the usability gate is a bit more vaguely documented because usability it turns out is hard it's hard to make sites easy who would have guessed the most important things for usability changes is like when something changes the user interface acknowledge that it has an impact on usability so you tag the issue with usability and that gives people from the usability team an opportunity to look at it give us feedback and you should follow patterns that are already in core maybe you want to invent a really cool new interface but if you are inventing a really cool new interface that needs to be documented and if it's the only thing like it in core that makes core harder because people then have to learn two different patterns for how to navigate this site and then finally the simple part to understand of the usability gate is when you change the user interface provide a screenshot show what it looks like before show what it looks like after and then anyone can look at the issue and evaluate the change alright so this is the last gate, fifth gate and this is probably the most important gate and it's also my favorite testing gate so I have notes over here I wanted to make sure I didn't miss things I would say of these five areas this is the part that has accelerated core development the most and it's also a really great way to get started as a core developer my first patch was an automated test and I highly recommend that everyone in this room go to Lee's Leer Allen's automated testing session it's tomorrow at 1pm I'm not sure what room it is in but it's called show me the tests and then there's a subtitle that's less clever the go to that session it's very clever it's just longer I didn't remember it it's less memorable but it's a really good session I definitely recommend checking that out you will learn a lot that you can take if you're interested in reviewing tests there's some fundamentals from testing that you'll want to learn first but koojee room, thank you thank you Shannon and that's where we are so it's easy to remember so no this is Bronte so the first thing you have to do is check the test coverage and make sure the tests pass we have a 100% test pass policy in core what this means is that we do not we do not commit any patches to core that make the test suite not pass anymore and once in a while something slips by that causes intermittent failures or two patches that change similar things that happen at the same time and that's like an all hands on deck situation that's disruptive so you as a reviewer don't need to run the test because the bot does that for you so keep in mind all you have to do is look and say hey is it pink, is it green if it's green it passed that's taken care of however the trickier part is checking the test coverage so it could be that test bot is not magical right if there isn't already a test in the code base that says make sure this thing happens when you click here it doesn't happen when you click there you won't know and the patch will still be green so as a reviewer you should check and make sure that whatever the patch changes already has coverage also make sure that the person who creates the patch isn't deleting a test I've had the experience a few times where someone had a test that was failing get rid of that because they didn't understand what was going on there was a problem with the test they changed the test to assert a different result and it's like well we kind of put that there for a reason so if you see a little minus sign in front of a test line stop and read the issue make sure that you understand why it's being changed and ask the person for feedback because always something to look at very carefully and finally not finally add new tests for new features and for bugs upload a test case that fails sounds doesn't really make sense the idea is that when there's a bug in core core tests have 100% test pass policy there's a discrepancy there because there's a bug so something should be failing so the first thing that someone should do is create a patch that only has the test in it so when they upload that patch to the issue that fails and it will show you exactly why it failed and hopefully explain here's this bug so then the person takes that patch the test only patch combines it with a bug fix and attached to the issue will pass demonstrating that the fix that they have actually resolves the bug and the test that they have actually provides coverage for it so it's something that's difficult for people to get used to but it's actually if I don't see a test only patch and a combined patch I can't review it's like you know please do this and then I send them a screenshot of the pink stripe followed by the green stripe which is my favorite thing in the world the test module not everything can be tested with just within a test sometimes you need a test implementation of something if you add a new API there needs to be something in core that uses that API to provide coverage for it manually test, not everything in Drupal is testable the installer right now can't have automated tests JavaScript there's work being done to add automated testing for JavaScript but it's not possible in core right now so if you have something that makes user interface changes changes the installer you can test that manually and provide screenshots when it's a front end change of what happens before and what happens after and then finally I have up here this is something that's not actually on the core gates page but it's something we've been discussing with the core maintainers and with what's his name, nod underscore I think his name is Theodor he's the JavaScript maintainer for core in Drupal 8 and so we've sort of agreed tentatively that until we have automated testing we want to document the manual testing steps that need to happen so if there's a JavaScript bug here's how you test to reproduce that bug and so in the time being that gives us like a template for the automated test that we will write in the future when that's testable so it's not on the page yet probably will be soon and that's again go to this session really it's the coolest thing ever so now just to recap a little bit things that we look for in my city we look for I'm sorry does it actually work is it a document we look for the gates we also want to look at coding style we touched on this a little bit before the coding style things like the two spaces that there's a period at the end of sentences that spaces on the side exactly these kinds of things we follow the pair coding standards in general and then we can you can also find the documentation on our doxygen excuse me the doxygen coding style doxygen we use to generate the documentation that you see on api.druple.org there it is api.druple.org slash 1-3-5-4 and you can find all of that information there so when we're reviewing a patch we are reviewing for these things as well now some tools that you can use to review patches I think maybe my favorite is dreadedor and we're going to talk about that a little bit more you can also use your IDE PHP lint which is when you type into your command line in the file name and it will just test to make sure that there are no syntax errors I'm notorious at like I'll be like, oh I'm going to fix this and I'll accidentally type a comma and so I've learned the hard way to type PHP dash l on my code base before I upload that patch because I look like an idiot I look like someone who should not be working on the kinds of problems I'm working on because I don't know what's going on she really doesn't look like that ever but when I upload patches like that I feel like that I think that all of us recognize that which goes back to always encourage so anyway PHP lint there's also a tool called JS lint for JavaScript and CSS lint there are tools that you can just feed the code through and see if it works there was a cleanup I'm not sure that everything works in JS lint and CSS lint but just make sure that new things do to review there were issues did everyone hear that JS lint complains about a semicolon with dribble behaviors it exists and then the coder module will review your code for coding standards and a lot of other awesomeness code sniffer and the grammar parser module are some other tools that you can use when you are reviewing patches and need like maybe more information on the patch itself and one thing that I would point out about the coding standards is that don't sweat it and if you see someone else screw up their white space don't be a jerk about it it's something that ideally a machine should be able to fix these problems for you and that's something we're working on adding coding standards testing on the bot and so forth all of those tools are something that eventually should be condensed into one thing on dribble.org that just takes care of it for you and says fix this and that so it's way less important than everything we've talked about so far it makes it difficult to read someone's patch if they have errors but at the same time it's not their goal it's not to have perfectly formatted code their goal is to fix a problem now it's yours mine's the demo sorry so those are the things we're looking for when we're reviewing a patch what does that translate into as far as the review workflow but let's start before we actually start reviewing a patch and that sort of thing we should maybe talk about a few little hints the first thing you can't actually unread the issue so if you're going to go in and you're going to review a patch sometimes it makes sense to not read the issue and just read the patch and follow the workflow and then go back and read the issue to see if it makes sense with the actual complaint or the bug report and those sorts of things also perfect is the enemy of the patch and done sometimes so if we go in there and we say this patch doesn't have the right spacing and we go in and we nitpick things and then eventually what happens is people just abandon patches they I've done it I'm sorry but I have and we can't expect complete perfection in fact there's someone in the room a d.o user who instead of requiring people to make their patches perfect will go in and fix the silly little things and then upload a new patch which is something you can do and you don't need to have a separate user account for it yes and that's actually a legitimate contribution to the issue but someone in the room is a little bit embarrassed by having her commit mentions than anyone else for cleaning up spelling and stuff so didn't want that to happen someone in the room did that and so that one would be no commit credit if you haven't seen that what that is that's just a silly little clean up and don't even worry about it that someone shouldn't actually worry about it because it's a relevant helpfulness that someone has two thirds of Tim's credits now so anyway oh okay anyway so and then to also remember that everyone is different when we are going through and when Jess is demonstrating a patch review and how dreaded are works and those sorts of things after you've reviewed a few patches what will happen is you will get your own process it will be different and it won't be exactly the same we're trying to provide you with the tools and some kind of an idea of what to look for but you should get your own process alright so now what I'm going to do is I'm going to demo a tool called Dreaded Written by a Drupal named son you've probably seen his name before especially if you contribute patches to Drupal he's tall, he's German and he wrote this amazing JavaScript utility that basically it's like Drush is to Drupal except for Drupal.org so it takes Drupal.org and adds all kinds of functionality to it and the major functionality that most people use is this little in-browser editing tool called Dreaded and Drupal.org slash project slash Dreaded so what I'm going to do is I'm going to demo a patch review using that tool or at least not the whole patch review process but sort of what I do the patch that I'm using incidentally is the largest patch I've reviewed in my career it converted the block system in Drupal 8 to use plugins it's the biggest patch in the world and it was an epic process I spent a month of my life reading this this by the way is an example of why patches need to stay within a certain scope because if you end up with a 400K patch not many people are crazy enough to spend a month of their life keeping and going back and reviewing it section by section anyway it's a very important change though it's fundamental for the blocks and layouts initiative the goal of which is eventually maybe not in this release cycle but in Drupal 9 or incontribute and so forth to make it so that core can do what panels does but better essentially so I'm going to switch now to our pre-arranged little so here's an example oh how do I make it show me aha okay good there we go oh yeah can you switch it to mirror we need to see we didn't rehearse this part of the presentation in case you didn't notice so assistant preferences display there we go sorry yay okay and that's big I'm going to press the little button sorry it's different it's on the other side now yay I can use a Mac okay so this is one of my 18 bazillion in five comments posted on this issue I'm not even looking at the right one anymore so I'm going to hit enter so this is one part of the patch review that I did is review this new custom block module Drupal 8 custom blocks that you create yourself are a separate module from blocks that are added from other modules so this is an example of what a review post looks like and I'm getting I'm getting a little silly in this post because I've been reviewing it for days but it allows you to paste these little code snippets with a comment about each one so I'm going to show you what Dredditor actually looks like here up a little bit earlier on the issue Tim Plunkett re-rolled the blocks patch here and Dredditor it's just a browser script that you install gives me this little blue review button here so I click this and it opens this patch is 400k I mentioned that already right if I try to scroll and find things it's not going to happen fortunately on the left hand side there's a little browser here that shows me all the different functions and files that are changed it's amazing it also shows you the diff stat this patch changed 146 files 5137 new lines 3456 deleted lines so it's a lot so I'm going to scroll through the custom block module it's under modules here we go found it I'm just going to go here one of the first files in the custom block module and so what I did is I just basically read through the module and inspected all the different lines in the patch and looked for things that looked out of place you can see that it shows inserted lines in blue, deleted lines in red comments are green so you can scan and see what's documentation versus functional code and what you can do is you go through and if you see something that seems out of place like I can't think of an example right now but say that there was a problem with this class name this class name is not specific enough or it's confusing I double click on the line and it opens up this little box on the side so I would say I would click so I would just put in a comment and say oh this isn't really a standard class name in this situation and then I would insert a documentation link here and then I would click paste and that actually inserts in a new comment on the issue five years from now because the wired internet is broken and it's very slow well we'll pretend that you now see the markup oh yeah that's good of you so anyways it pastes what makes this I'll actually edit the comment and show you wait do you have editing permissions on view or not okay I can't do that either but yeah it just inserts markup for code tags and then your comments are after them so it's a really convenient tool and what I do is I start from the bottom of the patch and read up and then I might navigate around through that browser what that does is it helps it so it makes it so that I'm not just like reading a story it helps me see things that don't make sense and I read every single line starting from the bottom and going up so that's how I I'll make note of coding standards violations I'll make note of things that are just like that's really strange it's a great tool but you know I mean I don't know of anyone I know of one person who does not use Dredditor and reviews a ton of patches but the majority of people who work in the cork you use Dredditor you know what scope is right where else sweet okay so to review a little bit of what just went on there so that we can get bullet points on it the first thing that we do when we open up Dredditor is we scroll down the issue right I didn't tell you there was an intro diff there was another file after that which Tim uploaded a patch that only showed the difference between his patch and the previous one on the issue so that's actually if you reviewed the patch previously start with that because it takes out all the noise and says the only things that have changed since the last time I put time into this are these two lines so we look for the most recent patch and we do that by scrolling all the way to the bottom of the issue in fact in the issue that she reviewed you can scroll all the way to the bottom but then you have to scroll a little ways back up because there are so many follow up issues to actually get to the most recent patch but what we want to do is we want to look for the most recent patch and pop open Dredditor and Dredditor on that one Dredditor provides you a handy little link to do that and then while we're there we're going to note if there are any test only patches or interdifts as we talked about before where we want to make sure that a test only patch fails and we need to make sure again as she was saying about the interdifts that if something has changed then we only have to review part of it we pop it open in Dredditor just the diff stats and the modified files which as you saw when she was demonstrating those that they were there we want to see which functions are changed oh wait, I'm in the wrong place there we go and then we also would like to check and make sure that there are tests then now we've scrolled down the diff now we're going to go back up we check the API documentation if there's anything that confuses us when we're going up yeah, okay oh, I'm sorry and we note any kind of code style issues as we talked about before any unclear comments and any other oddities about the patch and then for each hunk we say okay does this function make sense does this big part change make any sense so a hunk may have several lines or only one or two lines but does this make sense isn't in scope is it documented as we're reading back down and also in addition to Dredditor in addition to Dredditor sometimes we also need to use it to actually apply the patch and review it in the ID but I need and I think I probably need it more often than just us but maybe not or your editor but because what I need to do is I need to see the code in context I have to see okay you've changed this but what else was there what was there before and it gives me handy color coding which as a person who likes colors that's very cool pretty colors and in the IDE I had step debugging for free well for the price of the editor there is that so yeah does that sum it up for everyone are there any questions so that was one of the points of that gate I actually the majority of patches that I review I do not apply in test manually or I will do it once because once there's a test written for that the test tells me what's supposed to happen on that giant blocks patch I tested the heck out of that manual I spent so much time back and forth and then I have this huge comment along with like this is broken and this is broken and this is all changes or for changes that don't directly affect the front end I don't test it manually there are other people who can do that like we actually have this core mentoring initiative that Andrea and I do sort of distributes the workload so I have a lot of experience in the core code base I can just read code and say this is what's going to happen this is related to that and there are people that are way better at it than me too that can just like read it and they execute Drupal in their brains I swear it's amazing or Daniel Vayner the views call maintainer unbelievable I swear he has all 500 files that are in views in his head but what I know I there's an opportunity when a patch does make a change that needs to be tested manually someone else can do that all they have to be able to do is apply the patch and understand what the issue is about so we try to break up the responsibility for it so that that gives them an opportunity to participate in the process and it saves me time so I can go on and renew other patch but you do want to test it manually at least once or make sure that someone else on the issue has it's a collaborative effort right you're not responsible for everything you're just helping review the patch and then it goes to the committer too and if you get Angie as your committer she'll test the patch manually too I mean you should test it before she does I actually I do like to at least test the patch once myself manually test it because it also gives me again that context that I was talking about before where I want to see where it is in the code but I also want to see where it is in the UI and within the behavior of the of the site or the yeah anyway you get what I mean any more questions anyone else absolutely yeah and we talked about that briefly earlier too where I was saying that a lot of times especially new contributors where someone will come in and be discouraging or not provide very much guidance on how to fix it or how to make it better and so then you know things just they just go it is very discouraging but it never goes anywhere right there are things you can do about it there are things absolutely there are things you can do and this is an excellent session to have attended because one of the things that I want you to do is do a patch review exchange so find someone else that's working in core or in the contrib module that you're looking for and say I'll review your module if you review mine so that then we have someone who's reviewing and then also we're working with core mentoring to try to alleviate some of that problem and obviously the more Drupal grows the harder that gets right I guess the big difference about core is that and the reason I like working in core is in contrib there might only be that one person and if they're not available you know what can you do in core there's the patch might say that it needs you for a long time but once it gets to RTBC a core committer is going to look at it so that's one of the reasons I prefer working in core in general and that's one of the reasons contrib modules are not very well maintaining them more so I'm that person I'm a module you can find on my Drupal.org profile that a lot of sites use oops but another thing to keep in mind is that we're starting to try to solve the problem but there's still a lot of patches that just sit the first thing to do all those gates that we mentioned make sure your patch meets those gates log on to IRC join the Drupal contrib channel if you're not in there use it anytime you're trying to contribute something whether it's documentation whether it's a contributed module whether it's a fix for core maintenance you can ask the question in the channel and you get to talk to all the smart people like Angie is in that web chick is in that channel all the time I mean she lives on IRC I'm on pretty much constantly although not during the conference Andrea is on a ton of the time so you get a chance to talk to people who have a lot of experience talk to the core maintainers and like find out okay so I have this patch that makes changes to the entity system who's a good person to talk to about that who knows the maintainer for that subsystem I'll try to ask him he's never on IRC you don't know how to contact him you log in IRC and say oh hey does someone know a lot about the entity system and say hey talk to Bear Deer Bear Deer knows a lot about the entity system he's going to be a co-maintenor too and so then you get that information about who to contact specifically that might give you feedback because it's their little subdomain of core also another very key thing to getting your patch reviewed is to make sure that the issue summary is up to date and well and verbose use your most verbose mode on the issue summary you're almost most verbose depends on who you are I can get really verbose sometimes but anyway if the issue summary is not clear or doesn't make any sense or any of those things or not current if it reflects the patch as of October that doesn't help me if you've changed it then what ends up happening is I go in and I scan through the issues on the area and I'm like wait I don't understand and then I get distracted and this is who I am I'm a jump around but if it makes sense and I can kind of get in the zone and I can do a review but if it's something that I have to work at in order to understand what's going on then I will I'm sorry and it may be days weeks never before I get back to you I'm sorry exactly and I've left comments like that because I could spend a bunch of my time doing research and that's actually a way if it's not a patch that you're working on but that you're interested in and the person who originally posted the patch is missing then if you do take the time to do the research and originally know what the patch has abandoned it that makes it easier for the patch to get in because then someone else can look at it and all of a sudden people say oh hey I have this problem too and now I actually understand that this is the thing that's supposed to fix it and you can also try to take advantage of this but you can also ping and IRC and we'll tell you what to do next we have this workflow in our minds of like this is what I would do and we can also throw new contributors at it sometimes if it's something that makes sense to have a new contributor fix the issue summary or review or any of those kinds of things yes absolutely make lots of friends at Co-Strengths which is a great segue we should probably go to the slide right yes we're at the end of the time for our session now I'll hang around a little bit afterwards if you want to come and ask questions so yeah so some things that we can do next is definitely come on Saturday for our Co-Strengths they will start at 9 a.m. Addy's going to be here and she's going to do some awesome community tools workshop which will be from 9 a.m. until everybody is amazing and awesome with their setup or 1 p.m. whichever comes first and then we'll be doing the mentoring sprint all day I'll be handling that and there will also be a big kid's sprint in the upstairs I don't actually know where they're going to be I keep saying upstairs because the past two events had downstairs was where the core mentoring sprint was and upstairs was where the in a different room yeah did we explain what the core mentoring sprint is so yeah we didn't say that basically you come and we explain a little bit about our core mentoring initiative and say hey we have all these tasks that need to be done in the core queue want to give it a shot and then you work with someone else and we'll help you through the process of working on a patch and if you want to do this for a patch review that's awesome we can help you mentor you through like oh this is what I would do to review the patch or say you know what I wouldn't start with that one that actually can be a really helpful thing because when you're first getting involved you know what's difficult and scary versus what's straightforward and someone can give you that advice and then that also so what that sprint came out of is actually I do this core mentoring thing in IRC Andrea is one of the facilitators for it I'm a facilitator for it we have about eight people and twice a week I have actually there's three different times a week the second time is for wind sprints which Andrea leads by herself but in this time so in Sydney it's 1pm on Tuesday and Lee Rowland he lives here in Australia so he's a mentor so he's not like falling asleep because it's the late at night for him and then 9.30pm on Friday you'd get Andrea and she would have just woken up because we live in the States that's what your little thingy said but if she's not there at 9.30pm she'll be there I mean she'll be there it's a two hour block so she'll be there even if that's not exactly when it starts yeah and we use the same so it's the same thing but over IRC you come you say hey I want to do core mentoring you try it you can ask us questions we're there for the whole two hours and then we'll try to give you feedback on it and there's a website we use to track it and then like I said anytime Drupal-contribute on IRC that channel is the place to go and the coders launch here there were not that many people here now but yes that's probably what excellent and conferences are a great chance to meet people who have ideas that's fine we're done are we done does anyone else have questions or should we just turn the microphone on yeah we should probably let people not feel bad for leaving because it is snack time the penguins the penguins are Andrea's obsessive and she wanted to have three by three and we only had eight thank yous I killed the joke we could think of dozens of people to put in there but it would be like a collage of names overlapping and you wouldn't be able to read anything that would have, yeah I got to Petticoala that was like the highlight of my trip aside from this second, second best part alright well thank you oh wait one more thing don't forget to go and review our session at it's a survey forum it'll go to SurveyMonkey and just tell everybody how awesome we are they did fix it it's in bullets now I mean it's not thanks for coming