 So by the way, you see I'm splitting up the screen here. I'm going to try and experiment here, which is we're going to try to keep a summary in real time of this meeting. And I'll try to keep that centered on the screen. And then on this side of the screen, we're going to have the detailed notes. And so the detailed notes are sort of the traditional transcript of what everybody said and when they said it. It's not going to try to be court stenography level, hopefully, but basically, if you want to make sure that you were recorded as having said something, make sure that it shows up on the detail side. But then the summary is going to be something that really should only be, it definitely should not take up the full screen by the time we're done. And so make sure if you see something that's really important that somebody shouldn't have to dig into the detailed notes to find, make sure that it shows up in the summary. And I want to just see how this works if we can summarize this in real time, in addition to taking the more detailed notes. All righty, I guess it's 2 o'clock now. So let's start. Hi, everyone. We're here to talk about code review, as I'm sure many of you know what that is, hopefully. So first of all, to just talk about the agenda, I'm hoping to kind of separate it into problems that people have with code review or if they have a problem and what brainstorming on what the root cause is, followed by a part on what we could do differently. And then at the end kind of concluding bit. So especially focusing on social conventions and things like that, less focusing on technology like fabricator or whatever, starting from the point of, what do we ideally want? And then later we can go ahead and get there. And brainstorming ideas, not final solutions. So I guess I should talk like where I'm coming from to begin with to introduce the problem. Personally, I've found sometimes things are frustrating. It seems like with code review, it can be very random. One day a patch gets reviewed in an hour. The next one takes two months. One time some gets trivially looked over. The other time it gets looked over with a microscope. And then people disagree on what the correct way of doing something is. And it feels very inconsistent and random. And like playing roulette, how easy it is to get code intermediary. And personally I just like things to be consistent. So yeah, the point of the session is do other people feel the same way? And why do we think it is that code review situation is unideal? And then later on, what can we do about it? So I guess to begin with, since brainstorming, I should open it up to the floor about what do other people find frustrating about code review? Nobody? Do people think that it's accurate? Description of the problem. Hello. I can start. What I find most frustrating is that when I do have free time for code review, it can be hard to find patches that I'm suitable to review. And when I actually need reviews, usually it's just ping people on IRC and get the reviews that way. If I add people through Garrett and I think they're good reviewers, there's a good chance they won't even see it until I ping them a few days later. Yeah. Do you want to? It's really tall. As a third party contributor, I would say that the most frustrating part for me is not knowing who's responsible for a particular piece of code. Typically, the things that I want to contribute are in places where there is not a specific person to point to who's responsible. There's no one person who or even a group of people. Nobody's responsible. And then, as you know, differing opinions about how to implement features. And so there's nobody to arbitrate and to say that this is the right way to do it. So you get people disagreeing about really trivial implementation things and not making any progress. And it's very frustrating as somebody who's a third party developer trying to contribute to core. Thank you. One other thing I'd like to, sometimes it feels like there just isn't enough. It's not code views and it seems like it's shared enough. And it's not something that everyone does equally. Somehow it seems like we need to do a better job of sharing it amongst all of us. I don't know. OK. Do you want to have any other thoughts? So there's a bunch of media. Oh, there's two microphones. Oh, please. Oh, OK. You were first. OK. There's a bunch of media wiki code that's not really owned anymore because there's no core team. And so suddenly, if you want to patch that and get a review, in fact, no one is responsible. Not because everyone is responsible because literally no one is. Yeah. And that's a big problem. That's bad to put it mildly. Because we still need to make changes to these sections. Do you want to go? So a couple of things. I wanted to talk very briefly about my own experiences. One thing that I wanted to share is I'm not a very good code reviewer. I'm working on improving, but it's something that I think I will never be particularly excellent at. But at the same time, I try very hard to be available for ad hoc debugging of issues as they occur in production or elsewhere. And there are many times where I'd like to sort of green light something that seems stalled and would be more than willing and happy to sort of put myself on the hook for dealing with any fallout or anything like that that comes from it. But it seems that there's no currently socially acceptable way to take risks on patches, even when they're from highly valued contributors with a good track record. And that seems unfortunate and needlessly conservative to me. The other thing that I wanted to share, and I've brought this up into discussion on the task. So I forgot to introduce myself, by the way. My name is Ori. I'm with the performance team. I joined the Wikimedia Foundation to work as a MediaWiki and as an extension developer. But if you look at my commit record on two different repositories on MediaWikiCore and on Operations Puppet, you see two very different pictures. In MediaWikiCore, I have 300 patches touching about 21,000 lines in Operations Puppet. I have 1,500 changes touching 115,000 lines. And why is that? I'm much more interested in MediaWikiCore. It's much more related to what I'm supposed to be working on. So why this disparity? And I think it's because I experience MediaWikiCore code review as an incredible drag. People are not willing to take a chance on an idea if it's at all unusual or deviates in any way from established patterns. And it just does not allow you to build the sense of momentum and energy that you can with some other projects where you're actually allowed to merge your own changes under certain circumstances. So I've talked a lot. But the point that I was trying to make is that I think if you look at Operations Puppet against all expectations, it's actually a far more lively project. And I hope that that would motivate us to be more open to risk and to not discount the risk that comes with neglect and putting sort of barriers to contributions. So to summary, you're basically saying code review kind of saps creativity in a sense and risk taking and makes things un-fun. Hey, I'm Andre. I also had making code reviews suck less, especially when it comes to volunteer-contributed patches on my list for a while. And then I saw Ryan's talk, and I was very thankful to see this. So in general, I see social, organizational, and technical aspects. And we already said we want to exclude the technical ones here. We have aspects that are referring to the reviewers and aspects that refer to the contributors. And looking at what I see mostly as things to discuss, of course, those ones that where not everybody will agree anyway, like, hey, let's improve the documentation. Hey, we need more reviewers. Should we have a more structured approach when it comes to reviewing code, like three steps and not starting with the smaller parts, like code style guidelines and so on? There was a blog post by Sarah Sharpe about that. How to increase our reviewer base if we need more skillful and confident reviewers. And also how to decrease exactly what was already mentioned, these areas where nobody really feels responsible for, and maybe having a role for media wiki core, a person that goes through all these patches that are rotting and tries to identify a person who should review that and feel responsible within a time frame that makes sense. So I think these are the most important ones coming to my mind. I think, yeah. OK, so I have plus two in skins, and I don't know how to review things. Is there anything telling people how to actually do it? I think that's a big problem is that code review is essentially instructions that basically don't screw up. There's some guidelines for obvious things you should check to make sure the patch works and stuff. But there's no deep guidelines about what's considered acceptable and what's unacceptable. Whereas the line, is there stuff that's considered unideal but OK, and stuff that's considered very minor? And if it really bothers you, make a follow-up patch, but it's not a reason to not merge a patch. There's not very good guidelines for what we want to code reviewers do. And the unknown-ness of it kind of makes it less fun to do code review because you don't want to be the one who plus twoed something that shouldn't have been plus twoed because you were mistaken about how thorough your review was supposed to be. And when other people don't have time to review things themselves, they definitely don't have time to teach other people how to review things. So yeah. Yeah, just I'm Dan from the release engineering team. So I'm responsible for maintaining, well, officially responsible for maintaining media wiki vagrant, although Brian Davis actually probably maintains it more than I do. So one thing that I've thought about doing, because I'm really bad with time management, is just carving out a space in my agenda, in my weekly schedule for code review, even if I find that there's nothing to review, I leave it there as a recurring event. I've thought about going to the extent of making that like an office hours thing. So leaving some time in my schedule where people can actually create appointments with me to actually do one-on-one review synchronously. I think the importance of that is maybe specific to the project because media wiki vagrant reviews are quite intensive if you actually want to test things thoroughly. Provisioning things from scratch takes about 15 minutes. So you do that iteratively, and that can add up pretty quickly for each review. So yeah, as one maybe social idea for that is something like office hours where you really carve out some time and dedicate a chunk of your time, whether you use it or not, for those reviews. Ready? All right, I'm gonna be the spoil sport who says code review is necessary. So on the security side of things, obviously I do secure review new extensions, new servers and stuff like that, but then it's up to you guys to make sure they stay safe, otherwise you have to fix them as security bugs. And so code review is necessary. I would love, I meant to actually run stats on how many security issues get found in code review. I don't have those numbers. If it is zero, then obviously we should reevaluate. I think right now it's non-zero. I definitely know of a few that have been caught in code review, so I know it's non-zero. So I know we do find bugs there. So it is unfortunately a necessary part of our process. I think it should be made better, standardize it. People who are plus one in, plus two in should, that should be, I think we are culturally a little too hard on people when they do make, no, we're a lot too hard on people when they make mistakes. So I think definitely plus one on that. Just to talk about people making mistakes. And one thing that it kind of seems like code review, like it's kind of the job that's very thankless. Like no one gets appreciation for doing code review very much compared to making patches. Perhaps one of the problems is there's not as much like prestige around like reviewing a patch versus fixing a bug. Or like, because the code reviews take on basically all the risk involved with the patch for none of their reward. Whoa! Yeah, thanks for the segue. My name is Adam and I just wanted to mention that code review is definitely as painful as it is. It's one of my favorite parts about this job. But the aspect of code review that I like the best is the communication and even training part. And so what I've also had happen is random hallway conversations where someone comes up to me and says, hey, that thing you did in edit page, what was that? And then they sit me down and just like explain what's going on for five minutes. And that's actually, I can't say more valuable, but it's at least as valuable in a completely different way that allows me to then make more mistakes. So yeah, I feel like fitting some time into do actual training might be more effective if we even have a couple people, maybe a dozen people who are interested in a certain area of the code could show up and then whoever's been contributing the most in that could explain how that code works and the problems that they see, the ideas that they would like to follow up on, things like that. That's all. And we really don't have that for areas that are not associated with a specific team and for remote people and volunteers. As far as I'm aware, we have nothing like that. So first I wanna say I'm maybe strange and I actually really enjoy code review. I feel like I learn a lot from it. I get a lot of feel good factor out of it. Had one volunteer tell me, well I asked him like last year like why he contributes from over front and he said, could you review my code quickly? Like within like a day or so. So I think we really should be doing more than I think in the three, four years I've been here. Like I've seen very few new contributors and I see that as a symptom of a bigger problem that I'm really keen for us to rectify. I sometimes also feel like we don't retain them very well. Like we get input from like GCI and things but I feel like if we don't review things within like 72 hours, I think the chance of someone staying around reduces significantly because well like Wikipedia, right? It's built on instant changes and that's what makes it popular and models that don't have that tend to not work well. Yes, I think there's two symptoms which I think like before. I mean one like as we pointed out there's lots of repositories that don't have like maintainers like core is a very big chunk of code and it's very hard to like focus on certain areas. I mean secondly do we actually have like a definition of what's mergeable? I mean I've similarly like to already had really frustrating experiences just making code changes which all I do is refactor code and they just sit there for ages and then someone will find your review and they're minus one because I put a space in the wrong place in a for loop which is really stuff that you know it's not really helpful in code review like if there's something we can have in like some continuous integration to catch that sort of thing, that's good. Yeah, I don't know. I just kind of brain dump in my thoughts. I think the other thing about the fear of merging code is this idea that as soon as you merge code it rolls the train and it goes out into production. So I think there's something to be said around that like how can we, how can you be less concerned in our code review without worrying about break-in? So like have development branches instead? Yeah, exactly. All right, I'm gonna beat the drum I beat before about governance. Last July I wrote a blog post on mwestake.org about how Media Wiki needs a governance model and we really don't have one and I'll just quote this bit from OSS Watch, the OSS Watch Wiki, which says a clear governance model allows potential contributors to understand how they engage with the project, what is expected of them and what protections are provided to ensure their contributions will always be available to them. If you don't review people's code their code isn't used and you're failing. Again, I think that this is something that we really need to have a conversation about a governance model for Media Wiki for. I'm not sure I would classify this under governance. Well, I usually define governance model as more like how decisions are made at a high level rather than specifically like how we change the policy around code review less than what the policy actually is around code review. Well, we don't have that either. We have like a very convoluted like get consensus on Wikitech L and it's an ideal, but I don't know, that's kind of a site. The other thing is we do have the architecture docs as well which granted are in kind of drafty form but let's improve this. Although I guess ultimately I guess it's kind of a side topic already. I wanted to challenge Chris on the security angle. So Chris, you mentioned that code review finds a non-zero number of security defects and for that reason it is necessary. Well, right now our standard is to require one additional person with merge rights to merge and review the code. If we had upped that requirement to two there would undoubtedly be security defects that are then identified that we're not caught with a more relaxed process. If we upped it to three then we're liable to see the same sort of increase but I think we have to be very careful not to be biased in favor of what is easy to notice and what I mean by that is it's incredibly easy to say look, on line 35 of that patch somebody introduced a security defect and it was caught in code review. What's not so easy to pinpoint is there are vast sprawls of PHP code which no one currently involved with the project understands or is interested in maintaining and that of course comes with security risk as well and when our process becomes, when it drives off contributors then we incur security risk that way. See your counterpoint. No, I think that is a fair point. I do want to just clarify, I'm not saying that because we're finding a non-zero number of bugs that therefore we must do it. I think right now it is the only practical way we have identifying security issues post-release into production. We've looked at SAC analysis tools. They can't really look through our code very well. As a best practice, if you look at some of the security best practices, be some other things like that. Code review is actually like a very mature level of security. If you're actually doing code reviews that's actually a fairly mature level of security within your organization. So I think having a security review is a reasonable set in my perspective. I will totally evaluate it. I just think right now I think it is a reasonable trade-off between the security and just letting anything get merged. But again, I think we should do better about it and we should take more risks. But I think it is necessary. I just want to say, yes, there is a trade-off if we get rid of it. And I think it's a very reasonable one right now. Personal opinion. Also like from a security perspective, like I see if anything improving it would be to further probably standardize what you have to check for would probably help a lot with making reviews actually catch security issues. There's like some, where code review guidelines are actually around security, which most currently, but still it's not, like we can do that. Yeah, hi, this is Katie, fundraising tech. From my perspective, we cannot get rid of code review for PCI rules. So if the decision was made for MediaWiki to not do these things anymore, we would have to probably stop using MediaWiki or get someone to review the whole thing anyway, so. My personal opinion is that, like I think ultimately we have to, well, I don't know the summary, but I personally, I'm a fan of keeping code review, but I think modifying how we do it in possibly radical ways, and including a lot more structure and things is the way we'll help. But ultimately, it sounds like mostly people want to keep, I don't know. I think there's a difference between moving fast, breaking things, and letting other people clean up afterwards, and after a review, consciously taking a risk. That's a good point. Cool. Should we move on to talking about what we should do differently? Alrighty. So, so far there's summary. Does anyone have any specific solutions they really feel strongly about, begin with, that they wanna talk about? Looks like Lego does. So on the fabricator task, Brian proposed this great solution of making sure every patch to MediaWiki Core had a specific person assigned to it based on what area of the code it was in. And I think that's pretty key, that one person has to be, one or multiple people have to be in charge of a patch and going to shepherd it through the review process. And right now, there are places of core that have no reviewers, and we need to find reviewers for others, or reviewers and maintainers for those areas. One thing I'd point out also is even in areas where there are maintainers, there's lots of times where patches kind of seem to fall through the cracks. What I'd personally like to see is a structured system where each patch has a specific person responsible for it. And then, no matter what happened, it's like always someone is, there's someone who you know who to talk to if something is stalled. And someone who is ultimately responsible for getting the patch through, no matter where it is in MediaWiki. And how are you proposing to enforce this? That's a good question. Ultimately, I think first step is dividing up MediaWiki core into multiple sections and then allowing people who are interested to sign up for individual sections. And then this signing up and specifically saying that you're gonna be responsible comes with some responsibilities. And if you don't meet them, you get removed as the maintainer for that section. And we hope there's enough people who sign up to make it work and that they don't all get removed. I don't know if that's the best answer, but that's maybe, but maybe people would do it. If it comes with some procedure, yeah, like people want to be in positions of responsibility. I think people do lots of stuff for free on the internet because it has social value. Any other thoughts? So I never see the microphone there on that other side. One user interface problem that I think has been pointed out before and that we still have not solved is that there is currently no effective way of making non-blocking review comments on a patch in a way that both gets the author's attention but doesn't block it from getting merged. So I often find myself thinking, okay, I would do this a little bit differently or I think you might be better off doing this thing that way rather than that way. But it's not necessarily like, this isn't going to production on my dead body. I've found a defect. The problem is if you just leave those comments without a score, people don't see it and the interface is having been reviewed. And in the past, in fact as recently as today, gave a patch of minus one just to ensure that the person sees my comments rather than because I thought it was unfit for prod. Several points. So on the reviewer side, having a more structured review approach like as I mentioned earlier, actively check which people are using plus one, minus one in our current system and encourage people to get plus two to actually be able to merge. Cause I don't think that anybody's really checking if we have enough people who can actually merge or if that number is going up or down. Define a role to assign reviews that nobody selects. We already had that, especially when it comes to media wiki core. I think we simply need that. Also we need a review culture at least when it comes to WMF teams. That's something where I'd see maybe the team practices group but as much as there's daily standups, maybe there should also be like go through the unreviewed patches, especially those provided by volunteers. And last but not least improve the documentation both for reviewers, how to give good useful feedback to patch contributors and to contributors because there's pretty often a totally different assumption of what things are acceptable when it comes to not updating documentation or when tests break. And there's a huge gap of assumptions between potentially new contributors and what reviewers see as no goes. I just wanna come back to what Maxim said about your idea about people volunteering to look over code is kind of, it's great. And that works for Wikipedia. It doesn't work for media wiki because it's not happening. There's no system currently in place where like it's, there's people who- What would the system do? Like a system of you agree to do this and then they do that. Like right now there's some things, okay, there's a maintainers page that some people do but don't really follow through. There's no like social expectation that you actually- So what is the reward? If your people are going to do something for free, there's usually a reward. What's the reward? You get to call yourself a maintainer of this section. You get to call yourself a reviewer. Not many people like that. Question, have we ever tried anything like having sort of like office hours, something that's more of a live review session in IRC in which people who wrote the code are available at the same time people are reviewing things. So if there's a problem, they can correct it very quickly and actually have live discussions with each other. Has that ever been something that we've tried? Well there was code review day once but that's not quite the same thing. No. So basically no, we should do that. Cool. Let's see if it works, maybe it does, maybe it doesn't. Personally I've had a lot of success when I can actually in real time like synchronously have a conversation with somebody who's looking at the stuff that I wrote and then if I do have to patch it, we can do it very quickly and then it's done. So what Katie said does happen sometimes like at HUC but no it's not really standardized, it's not scheduled, it's not planned. But yeah, I have many times reviewed people just live on IRC. Just anecdotal evidence. We use Cloud9 once in a while and we do, we just go over that code review synchronously, get it in a good shape and then put it through Garrett and that works really nicely sometimes. So one thing we do sometimes and I can't really say in fundraising but one thing some people do is they merge patches if they're passable and if there's still issues they still comment on them and they just kind of rely on the author's goodwill to fix that up. So the visibility of those is a huge problem like Ori was saying I think. But I'm curious to hear from people who have experienced with the GitHub style feature branch development. Does that work for them? Because I would love to hear more about that because I really like the idea of just moving forward with a patch and then at the very end if you want you can squash it down and merge just that if you want it to look really elegant. Otherwise it's the real question is the final result what you want rather than is each intermediate step perfect? One thing I'd point out though is sometimes a lot of the problem is not coming to agreement on what the patch should be, although that's a problem. Oftentimes it's like the patch sits there, no one comments on it, no one says anything about it, no one touches it and like three months go by. So Brian, I'm in the reading team I guess. I don't know, does anybody know where I work? I think I work at the foundation. To Adam's question about GitHub style development, the composer merge plugin component that I originally developed and we're using in core. We have GitHub as the primary repo for it and my rationale for that was that I was trying to reach normal PHP developers outside the Wiki community to contribute and I can say that I have gotten a non-trivial number of enhancements, actually I have far more contributions from people outside the WMF and larger movement to that but there are a few key differences from Garrett that I find personally really crappy about GitHub. One, you can't do patch chains in GitHub so I can't propose two separate code reviews, one which depends on the other. You can propose them, you can have a feature branch that comes off another feature branch and put both of them up as pull requests but the second pull request will include all the changes in the first pull request and the second pull request so it's hard to compartmentalize a larger change that way, the way that you can in Garrett if you're stacking like four or five patches that build to a final point and then the other thing that I find truly horrible about GitHub's code review process is that often in Garrett if I'm reviewing something especially in MediaWiki Vagrant and there's a couple little nitpicky like, oh I'd like to see a tab here or a space there or this comment isn't quite right, I will amend the patch and merge it and move on or on a larger work that's not ready for merge I may even just, I may add a small revision to an existing patch that fixes something that I'm concerned about that there's been some discussion on it and I think it's ready for it but the developer hasn't had a chance to do and GitHub's pull request model does not allow that you can't take over somebody else's work or collaboratively work on a single change set that will eventually get merged together. So to speak a little bit to what Matt was asking about GitHub style pull requests, I think this is one place, without getting buried too much into the technologies which we don't wanna do, I think this is one place that fabricator kind of gets it right, it kind of straddles the difference between Garrett and GitHub in this regard, like it totally gives you the idea that a change is somebody's but at the same time it gives you better tools for creating dependencies between them, a lot Garrett, it's not quite as strict as the way as Garrett does it because Garrett does it strictly based off of the Shaw one in the branch history but you can indicate dependencies in a way on fabricator that you can't with GitHub. So I think that's one thing worth mentioning. The other thing is that I can't remember at this point so nevermind, I'll just leave it at that. Super small, super specific concrete proposal. So we may not be with Garrett forever, that's fine but I'd still like to see even if it's only for a short while, plus ones eliminated. And the reason is, so there's a bugzilla quip in which I think MZ asks what does a plus one mean anyway and Tim famously replied, reviewer has a working mouse. I think the plus ones do have a legitimate use case and that's for very controversial patches where you really want to establish consensus, it's a way of saying I'm for this but I wanna give other people a chance. In those cases, it would be I think less obvious from an interface perspective to just indicate in a comment like plus one but it would eliminate a huge use of plus ones that is not legitimate in my opinion which is, oh, can you review this patch for me? Sure, I did, plus one. Yeah, it's almost like a like button. Yeah. I've personally found that when I see a patch with a plus one, it doesn't necessarily mean it's any more likely to be good. If anything, it's slightly less likely to be an okay patch because usually what happens is it's, someone isn't getting the patch reviewed because it's very complicated so they ask their friend to plus one it. Seems to be like a trend I noticed sometime. I totally ignore them with an interface. And I think this is another area where a differential fabricator actually improves this is in the signaling. So we have to remember that we shy off from talking about technological solution sometimes but we have to remember that these technologies actually embody the way that we're communicating. So Garrett actually only gives us very like binary ways of communicating to each other that this has problems or it's ready to be merged. And like you said, a plus one is pretty much meaningless. Differential allows you, it signals these things differently. So when you comment on something, you don't give it and it's just purely a comment. You're not saying this should merge or it should not merge. It signals that is just information. So it's a very neutral way of telling the user, the contributor that there's information that they need to go look over and it's very neutral. So that forces the reviewer to actually give substantive feedback rather than just sort of this binary signal. The other thing about it is it puts the impetus for puts the responsibility of merging on the contributor and security implications aside and other implications aside. That actually makes it easier on the reviewer as well because they don't have to worry about knowing whether this patch is actually ready or to be reviewed and merged or if it's sort of a work in progress because those things aren't always marked either. So I have that problem a lot with media wiki vagrant. I'll go start reviewing something and somebody will comment really quickly like, wait, wait, I'm not done with this. So I think it improves those two sides of it, at least. Yeah, I just wanted to mention the main reason I wanted to shy away from technology thing is I don't want to be limited by current technology options. Like if we determine that the best way to do something is something we don't support, then the answer is to support it, not to limit to what we currently can do, I think. Sorry, I'm too short. I just wanted to point out, I don't know if it has to be by a plus one necessarily, but I do, this is like one reason why plus one might be actually useful. There are some cases and quite a lot actually where we have someone who is maybe more of an expert in one maybe type of thing where we would like to make sure that they give us their opinion, but they are not necessarily understand the code base enough to plus two. So for example, I'm sometimes asked to check things for right to left support and it might be extensions that I don't work on so I don't feel comfortable to plus two and I don't necessarily know if the library is being used correctly, if everything is okay, so I might leave comments about the code, but I don't feel comfortable plus twoing, but I do want to expressly say, yep, it works in right to left, got my stamp of approval or whatever. It doesn't necessarily have to have plus one might be in a comment, but sometimes comments get lost, especially if you continue discussing something. So it's something very useful to see that that person that understands whatever language or directionality or UI or UX gave it its plus one. And I certainly see that use case with C brand plus ones. Well, my code doesn't give very many design reviews apparently, but that's probably not a good thing. But yeah, I do see that use case sometimes, although often a comment would suffice, but in complex cases I could see why. So to answer Ori's question a little bit ago about numbers in terms of review, all of that is ultimately, I hate to use this because it's a cop out answer, but all of that's configurable. So if we were to come to consensus on a better set of metrics other than negative two to plus two, that's not something that we couldn't change. I generally agree when it comes to plus ones. I like Tim's comment about how it basically proves you have a functioning mouse. I've always likened it to Facebook likes. It matters about as much as a Facebook like in that it doesn't. I do see the use case for wanting to be able to chime in and say, yes, I've reviewed this for the parts in which I feel competent, but I'm not competent to fully merge it all the way through the cases like localization review or design review. Somebody comes along and says, this part of it is okay, but I don't wanna be responsible for merging it. And I think part of that is due to the workflow that we've built ourselves into in Garrett where plus two approval also implies merging. And that's not a good thing. I think we've kind of boxed ourselves into a corner there where nobody wants to come and review something because at the end of the day then it's going to go all the way through and end up on production. And then when it breaks, then they come back and say, well, why did you plus two this? Well, I thought it looked okay. As opposed to it being the responsibility of the person who was writing the code to begin with. Fabricator like Dan was saying gets this right as opposed to like instead of just putting us into little boxes where we have numbers that mean different things to different people depending on the context. Like Fabricator, when you respond to code review, like it's always asking for some sort of action as a result. It's either this is good, you can go ahead and merge the style or it's this is not good and here's the things you need to do to make it good. Like there's a whole series of things that you can possibly do and it's not just in nebulous like plus one minus one score that you then have to interpret as a author of the code. And so I think that's one thing we can do better both in the glorious Fabricator future as well as hopefully Garrett more immediately. Yeah, hopefully like making it less risky will hopefully mean more people do it too. Code review that is, at least I hope so. So picking up where Chad left off, in fact I use plus one and plus two in yet another fashion. Plus one when I give it means I've read it, I reviewed it thoroughly, I have not tested it. Plus two means I'd merge this right now and take the rap if there are problems, if you weren't taking responsibility yourself and in some cases I will in fact, someone else will say, hey, can you use it? I'm like, yeah, I'll even plus two and I'll merge it, do you care? And they're like, no. And so then I take ownership of it at that point. That's clearly different meanings that I'm hearing from other people but in that framework plus one makes perfect sense. I wanna preserve that, that I don't want to have had to test something before I give my review that says I like it, that I looked at it carefully. I was just coming up here to say the same thing. We'll persist across multiple catch sets, plus one and minus one will disappear if it's any, you just go along and say plus one and don't give it any comments or context whatsoever and then somebody comes along and revised the patch. You will never have that review again. Yeah, I have a few things to say. First, don't rely on votes. They're like, yes, there's plus one plus two but you also have to leave comments. If you're just swanning things without comments that's totally dumb and you should have your plus one rights taken away. But you have to leave comments. Like you have to explain why you're leaving a plus one. Yeah, the second thing is Mark and Max talked about that no one will be motivated to review stuff like volunteer and like Brian's proposal. And I'm thinking that like we need to give reviewers more recognition like right now no one notices the reviewer. So like we can put a list of maintainers on special version. Like we have the list of authors. We can also put the list of reviewers and maintainers and like, you know, the kernel does like signed off by in the get commit message but Garrett puts the reviewer in the get note. So no one sees it unless you use get blick which no one does. So like if we can make the reviewers more prominent and I think differential right now is putting the reviewers in the get commit message and that's another way to give them prominence. And then my last thing I wanted to say was that along with Brian's proposal I think it would be a good first starting step is like we already have a Garrett reviewer bot that automatically adds reviewers based on the paths changed. So if we can make sure that at least one person is assigned for every single like file or directory and core that would be a huge starting step to making sure that people have responsibility. And I think like we can go through the current list of the people who are added as reviewers by default and see which directories are missing people and which directories already have people. One thing I'd like to say about ensuring people get a recognition I think we should like I remember in bugzilla days we had like a report that got emailed to wikitec l regularly that included like the top five bug closures. And I think that inspired people to fix more bugs to try and get on the list. At least it inspired me at one point or another verification. Yeah so like a similar thing with I'm not sure if everyone's familiar with Nemo's code stats. It's a list of how many people have reviewed even plus two patches in the last month. Like something like that emailed regularly to wikitec l and maybe also something dividing the code into sections and saying how long the longest patch open in each section is. Assuming we somehow get rid of the trend where we just instead of rejecting patches we just let them simmer forever and ever. But I came up here mostly to say the same things that Chad said so I'll just plus one that. But essentially I think that the biggest impediment to myself doing code reviews has been the automatic merging. Like there's been a whole bunch of times at least at least 10 times that I have plus two something and then later on I've been like slapped on IRC by somebody saying why did you merge that or why didn't you deploy that? Because I didn't know that I was supposed to deploy it because certain repos have this implied you must deploy if you merge and it's not exactly explicitly shown in the UI anywhere or I'm not even sure where I would look to find that out. And so because of that it's demotivated me from reviewing things because plus two implies that I'm gonna get slapped at IRC for approving something that I shouldn't have and I don't know why. So that's the biggest, yeah demotivator just for me personally. Yeah well this discussion like about plus twos varies a lot depending on the repo we're talking about. Like I've mostly been talking about Meteoriki core generally but like in the Wikimedia Configure is a totally different kind of conversation in a way about reviews for that repo. Well there's also another reason why people do plus one things. They don't have plus two. So what are they supposed to do? And from what people are saying it's not just pointless for them to plus one things they can actually be counterproductive from the sound of it. It's more like it's very meaningless and if someone plus one's it who I've never seen before or heard of and their only action on Garrett is plus oneing that patch. It's often not a good sign honestly. I don't know but like usually I think the trend is sometimes people try and force other people to plus one their patches and hopes it helps things and then it's just like a side effect that sometimes when you see a plus one it sometimes is like it's the opposite. Another way in which it can have a signal value opposite than the one that's intended is okay so plus one tells me that someone liked the patch enough to endorse it but was shy urging it which means that it might be risky if someone was hesitant to actually have their fingerprints on the trigger. So one way I've used plus ones in the past is at the comment I reviewed this please merge because I didn't have the permission. Another one is but making a plus one useful always involves adding a comment saying what you actually reviewed. Yeah. Okay, anyone else have anything to say or should we? Yeah, that's kind of what we're doing. Yeah. Already, well I guess nobody has any other specific things they want to say. Now would be a good time to kind of look over maybe the list and see if anything, I don't know jumps out as invoking strong emotions I suppose. I guess starting from the beginning. So like talking from the summary thing so how do you find how does a review or find reviewable patches? That would be, I guess the obvious solution of the ones we talked about is breaking down core better into areas and a lot of that's very tied to technology also with like querying and Garrett which originally we weren't gonna. Can I interrupt a little bit and just talk about like the so part of the reason for doing so the thing that I, the reason why I've been going with the question format only is because like it's the clearest way I think to not stop conversation is to just basically put the questions in the summary and not the answers and say like that the answers are in the transcript, right? And then later on and so maybe one of the things that we can do as a wrap up on this is like are there questions on here for which the answer is obvious or are these all like tough questions that deserve more discussion? So I don't know how to do that sorting but that's that I think that's part of it. If there are questions on here that leap out as like the answer is obvious maybe we can we can actually put that into the obvious category and say what the answer is. Did you want to? Well to me the answer is obvious to almost all of these and it's fabricator differential seriously. One specific thing though is breaking up core into you know like ownership areas assigning people to own pieces of code. Fabricator has a tool that we can use right now to do that and it's called owners. So you should go on there and look at it. Would we actually use it as the thing? Well we could and we could right now. We can do right now we can. Right, yeah that's called action right now. Everybody go into fabricator find pieces of code that you're interested in or you're able to review, create an owner's package. If you need help doing it talk to me either here or an IRC and I'll help you do it. But like even before fabric we could always do that. We could have done that in SVN days if we wanted to. But there's tools to help us do it and to formalize it and why not use it. Tools don't necessarily mean we actually do it. Right, but we could actually do it right now. So basically you caused me to introduce another question to the list which basically means no we don't the answer's not obvious. There needs to be another further discussion on it. But the reason why I'm saying like that like asking is there anything where the answer is obvious? It's like is that if it's like something where somebody says yes of course. So for example let me just try to find one really quickly that I think was a really obvious choice here. So I mean these are all tough questions right? I think one of the ones that no one disagreed with was that we need code review guidelines for both contributors and reviewers. Office hours, okay so let's see here. So which line number is that? Okay so 42. Right so okay right. Anybody object to that? No, all right great. Any of these others that we should do that for? Well maybe there is an action. So are there action items like is there? Is there? Okay. Scheduling office hours. Yeah anybody want to volunteer for this? Cause it's like it doesn't make sense to put up an action item that nobody's going to take on. I wanted to talk about the like when McKinney was saying about the owner's thing. As I've said elsewhere the problem is not technology, it's a social problem. We have a list of maintainers like on mediawiki.org that people don't update. Summoner used to update it. So like we need someone to go through prune the people who are not actively maintaining those areas add the new areas of core that have showed up since that list was last updated and nag people to take responsibility for certain areas and then convert that list of like the areas based on whatever that list is to actual like code paths. So people will be in like in charge of like you're going to assign people specifically to a patch based on what code it touches. And I am willing to like do the effort on that. And of course I'm going to get. Can you scroll the right path now too? Oh yeah okay. So I'm going to quickly throw a wrench in Canals suggestion. Well I think you should. Not act on just in his spokes. But so part of the part that would make this actually use so right now we have all this right. We have you can edit that wiki page and Garrett will automatically add you and you know we have the maintainers page and we can update that but then what right. I think we need an actual dare I say guideline policy thing on you like what we were discussing before about you know there is responsibilities with adding your name to this list and how to get added to that list who you know all that kind of stuff. So maybe we can talk more about delay that to a different discussion I would assume and yeah. Yeah well I think it's kind of both are important in a way like it's important to have a list that someone can go to of areas where at least this person knows about this area of the code and is reasonably active in media working development still but that's kind of a separate thing from having a list of people where the buck stops here for getting code review done and social responsibility is what is needed there. So stop me if this is totally out of scope but it seems to me that we should calibrate kind of the effort that is being spent on this list. These are good questions. They're gonna be hard to answer. They're gonna take a long time and effort. I mean look at Linux right. It was reviewed over email by a jerk you know like. So and it works and it's scaled to like ridiculous proportions and we don't have these proportions. We have very few contributors from my you know objective point of view I think. Maybe we need to think about what other stuff like make the code more fun to work with and. I'm not really familiar with how Linux does code review but I'm given to understand that they have it kind of split up into components right. And like. I'm not talking about now like in the early days right. And I'm saying it's possible to be successful despite like really horrible experiences in code review and it's not necessarily obvious that making code review awesome and fun and perfect will improve like the stated goal of the session like the number of participants. Although I'd point out there's a difference between like indifference and kind of hate as a negative experience. Like in some ways indifference is much more sapping than someone who's mean. Cause indifference means no one even bothered to respond. And that's in some ways I feel like that's more demotivating than people being assholes. But I don't know maybe not. Are you all saying I believe that? He said there's a very lots of prestige with being a kernel contributor and not as much with meteorokin. Is that good? Yeah, so you're. Yeah, so people are more likely to suffer through it. And that's something we can use when we think about how we make our code base more fun to work with. Like maybe people just really don't feel like they would contribute to society if they add, if they make media wiki core better, maybe they wanna work on the front end or maybe they wanna work on something else. So we should figure that out and focus on those areas first. So the owner's list thing again, it was said it's social and not technical but it's kind of a combination of both. Because as it is right now we have the list on the wiki but it doesn't have a direct effect. If we were using the fabricated differential thing we would have the owner's list and it would directly, if you were the owner, you would automatically be added to review on any relevant patches. So there would be an actual immediate reason for people to keep that up to date. So that would be one of those cases where a. People use reviewer bot but that doesn't necessarily mean they review everything they get. Well, true. But if we're going for a comment of people won't follow the policy then nothing we're gonna do is going to fix anything. My point is more the policy currently is not just because you added yourself as a maintainer you even have to do anything to maintain it. Like there is no policy currently. Well, fair. So a long time ago we used to do code review here via email and it didn't scale for us. The reason being is we don't have proper breakdown into packages in the way that the Linux kernel does. The reason that that is scaled for Linus and everybody else there is because by the time by the time Linus reviews a particular patch to Linux like it, it's already gone through several layers of gatekeepers at this point. So he's basically at that point rubber stamping what other people have already reviewed. We didn't have that when we were doing it via email. So doing it via email didn't scale. And I think the problems we're having inherently come with lack of ownership over different parts of the code base. I don't think using a particular tool solves it by itself. And I think we really do have to have the sense of ownership in the way that we're talking about over different components of the code. And there has to be a clear process as well for removing ownership as well from somebody who if I claim ownership over a part of code and I keep getting added as a reviewer to stuff and I never review this stuff, like then we need to have an escalation process at that point so somebody else can step up and take over it just so we can't just say, well there's a reviewer but he never does anything so oh well, because otherwise we're kind of in the same situation we have now with doing it ad hoc via Wiki. The problem with that is you can show, you can be interested in code without taking responsibility for it. Like there's a lot of things that I add myself as a reviewer on but I'm never gonna review it. I'm just mildly interested in keeping an eye on where it goes. Like I automatically get added to every single configuration change. Like I'm not gonna merge and review every single one of them though. So I think it's a matter about, it's about getting people to really take the ownership of something once they've declared an interest in it and then having a way to say, I really don't care about this anymore and somebody else needs to step up and take over. Cindy? So what I'm trying to do to make this more concrete for me is apply it to my own particular situation right now where I have a patch that's languishing and would like to very much to see it in some form or another merged. So with the discussion to date, the ownership issue is a huge one here because it's not clear to me at all who the person is or should be who's responsible for the area in which I'm trying to apply a patch. And this patch started out as a new class. And so where it's clear when you're editing an existing class that there might have been some path of editors responsible for the bulk anyways of code in an existing class when you're trying to add a new class it becomes a little bit less clear who would be the appropriate person to have ownership of that type of feature. So the ownership issue is definitely a big one but it's more complicated than just who's responsible for this particular bit of code especially if you're trying to contribute something new. And then the other issue is an architectural difference between whether something should be implemented as a separate class or as new code added to an existing class and I have had two versions of my patch. I abandoned the first one as a separate class and then created another one where it was code added to an existing class. And there's no decision, I don't know who to go to for a decision on which because there are absolutely valid reasons for both approaches. So it's really more what's needed is some sort of architectural guidance. Are we going in a particular direction with the code base where features should be added as object access classes in which case existing code should perhaps be modified but also new things should be contributed in a particular style. So it becomes unclear when new patches are being asked to be added in a particular architectural style that's not yet common in the code base and who arbitrates those decisions? Who owns that? And so again, it may be more complicated because of a third party contributor and so I don't see you all every day in the halls and so I can't just chat at the coffee machine but I very much want to contribute and be a part of this wonderful code base that we're developing but it's unclear to me how. So is that 10 minute warning you mean? Yeah, okay. Yeah, I think it's very important to have some guidelines about like not just what is the best way to do things but also what is an acceptable way to do things and when we're transitioning to new, like when we're transitioning to new, like say dependency injection, I missed the session but it'd be like we'd have the guideline and then we'd have like modify the guideline too. Now it has new code has to use this or maybe new code preferred to use this but if someone submits something it's still okay as like just very specific guidelines about what's considered okay and what is not considered okay instead of just what this one person would like to see versus this other person and it ends up random depending on who happened to review your code. As for owners, I would also like, like if we do divide into owners I think we'd have to have like a catch all ownership for things that aren't currently in a component with hopefully with everything componentized that would be a very small amount to code because otherwise it'd be unworkable if it's like 90% of media work is ownerless. Greg? So my project manager self is freaking out still about the lack of action items or more action items on this. That's a good point. Yeah, I still feel like there's one about the ownership discussion, right? And we only have one now that's about updating the maintainers wiki page. That's as far as it goes though. So is that something that we want to make more movement on? Is that an RFC? Is that a what, right? So, I guess let me respond on that. I mean, I think there are things that we could have done prior to the summit to figure out how to make sure that we have better action items here. But we can't, we don't have access to time travel technology. So we can't do that. So that said, I think the thing that we can do is like some of the action items can be figuring out how to follow up on this list of questions. Like prioritizing the list of questions and figuring out which ones are the important ones to move on, which ones aren't. I mean, if this meeting wasn't useful, then that's fine. We don't have to make this meeting useful. That's not the most important thing. The most important thing for us to do is to improve our code review process, right? So, I'm not as worried about the lack of action items here as I am without, if we're not getting clarity on what the challenges that we have are. I think it's a little more clear in my mind what the challenges are. Yeah, as long as there's someone on point for doing that review, I guess, Brian. I guess that's me. If I was to add an action item, I think it would be for someone, possibly me to write up a more formalized proposal for owners and what precisely that means and like talk about it on Wikipedia L and start an RFC about it. Part of the challenge is that code review has to do with the rules and regulations of how we interact around software development and it's very difficult for any individual or any group of individuals, unless they are invested with some kind of special and formal power to actually change that because it's sort of meta. And in light of that, one recommendation that I would make is that we agree that code review is a sufficiently severe issue that we should appoint some small committee to study and analyze it in detail and to make binding recommendations about how to change it to the whole community. That's interesting. Actually first, before we go there, just by a show of hands, who here thinks there's a problem with code review as it currently is? Okay, just one double check, like. Okay. I don't think so. Yeah. I don't know. I think it's just fine. Anyone? Maybe it's just mildly annoying, but not that horrible. Yeah, very bias, selection bias. I think my point earlier was more that it, maybe it's bad, maybe it's really bad, but that there might be much more return on investment working on other parts of the problem that, where the problem is having lots of people start contributing on our code base and engage with us in a real open source way. Could you go in more details what you mean by the other problems? Yeah, so the way that, if as a front end developer, if I wanted to work with a front end and I try to understand it, it's a fragmented, non-standard, non-modularized, non-componentized mess. Code base sucks in other words? It's not so much that it sucks. It's a big code base that's mature and has. I'll give you a concrete example. I tried to help with visual editor and I think the code base is actually really good and the architectural documentation is really good. I read through a lot of that stuff. I understood a lot of it and then I went to Garrett and I tried to find a patch and I talked to James and I know James, which is a huge advantage and he's like, no, there's nothing to work on. So, and I doubt that's the case. That doesn't sound right. I mean, it's a big project and I'm sure there's something to do. So I think there's some step missing from a total newcomer's perspective to actually participating. There's like a gap there that needs to be filled. When he said there was nothing to work on, did he mean there was nothing to review? No, I just generally wanted to help like write something new or whatever. And that could be a reflection of James' opinion of me and he didn't want me to. Yeah, cause I don't believe that there wouldn't have been things in visual editor to do because we have roadmaps for stretching into the 20s. Yeah, this is, okay. But we keep on top of code reviews so it is actually plausible there wouldn't have been anything to review because enough of the team reviewed cross reviews that that can happen. Okay, so we're running low on time here, I think, like four minutes. So does anyone have any specific things they wanna get off the chest before it ends? I also think in whatever planning we do to come up with good solutions, we should talk with teams who do a good job of code review because there are some teams that are actually really on top of code review and actually have volunteer contributors that actively contribute to their code bases. Maxsem wants to know examples. Like mobile team that used to exist. I think what that comes down to though is ownership. Like a lot of stuff in MWCore isn't owned by people and probably a reason why we didn't always have a huge code review backlog in visual editor was because it was clear that that was the thing that we owned and that if you were on a team you were gonna review something that was what you were gonna do. Yeah, so I guess what you're saying is teams that where there's a very specific boundary where they're specifically responsible for the entirety of an extension do well. I'm not saying that they necessarily do well but I think that is an element that makes them do better than MWCore, which as a thing is not owned by anybody. One thing I'd like to say, like I really wanna make sure when we do owner thing it's not necessarily like there's a WMF team that owns every kind of, like I want it to be whoever owns it. Maybe people work for WMF team, maybe they're not. Maybe it's something they do on the side that they care about. I don't know. And I guess it goes beyond ownership it's a maintainership because the visual editor team doesn't just own visual editor as a thing they grudgingly take responsibility for. Everybody on a team works on a software every day and so they are naturally predisposed to reviewing stuff there because it is easy for them to review and comprehend the code because they work on it every day. So it's more maintainership than ownership maybe. Yeah, okay. I have a follow up comment to that. I think one of the things that we can look into is other organizations taking on ownership of pieces which could involve us pulling in code from other organizations rather than inventing the wheel ourselves. I don't really consider them an external organization. Yeah. Already, I think, okay. Already, so last final chance to comment on anything? Anyone? As I think that's, yeah. Hopefully this was useful at least got people thinking about it and yeah, I guess that's then. I guess that's then. And hopefully I probably will follow this up with some post to Wikitech L about things that were talked about here and the more popular solutions and hopefully we will implement them eventually.