39 ms·

Improving code review time

216 points by pzrsa 4y ago

deleted 4y ago
[deleted]
posharma 4y ago
Disclaimer: these are anecdotal reports. I've heard from a lot of my friends how abysmal the quality of code is at Meta. Obviously, this may not be true in all teams/products, but that's the general sentiment. Why make it faster when you're already dealing with mess! This is abundantly evident from the constant fire fighting, duct tapes and a metric driven culture that incentivizes the number of diffs landed.
- rat9988 4y ago
  Because making it slower won't improve anything.
- cheriot 4y ago
  Are there companies with a reputation for code quality?
  - no-dr-onboard 4y ago
    GitLab and Netflix come to mind.
  - sgeisenh 4y ago
    Google places a pretty high emphasis on code quality and readability. It's not universally great, but it's a big part of the culture. You can catch a glimpse in their [style guides][1], [abseil totws][2] and [aips][3]. Almost every change is required to be reviewed for "readability" in addition to functionality. This can feel like a lot, but it leads to pretty consistent style across the codebase which makes it a lot easier to switch projects or read and understand an unfamiliar section of the code. [1]: https://google.github.io/styleguide/ https://google.github.io/styleguide/ [2]: https://abseil.io/tips/ https://abseil.io/tips/ [3]: https://google.aip.dev/ https://google.aip.dev/
    - CharlieDigital 4y ago
      I find Google's take on code reviews to be particularly good: https://cloud.google.com/architecture/devops/devops-tech-trunk-based-development#common_pitfalls https://cloud.google.com/architecture/devops/devops-tech-tru...
      - ok_dad 4y ago
        I enjoy how they state like 3 times that code reviews should be synchronous, yet "industry-standard" (aka: what people really do) is to toss it over the fence in a PR and go back and forth for several days with stylistic bullshit.
        Jensson 4y ago
        Code reviews are usually synchronous at Google though, commenting and fixing things is like chatting with the reviewer so are usually done quickly. Not sure why this wouldn't be industry standard, is there any reason to make code reviews more painful than that?
        ok_dad 4y ago
        I was kinda joking that places like Google might have good processes but most places just cargo cult it and do it incorrectly. My current job does reviews that are so useless I don’t even participate anymore and no one cares. I really want to work at a place where they care about code quality and an effective process.
    - aftbit 4y ago
      Oh, Abseil is new to me. Thanks! Do you know where the missing tips are? For example: https://abseil.io/tips/110 https://abseil.io/tips/110 The root page (parent's [2]) mentions this one as famous by name: >Often they are cited by number, and some have become known simply as “totw/110” or “totw/77”. Is totw/110 some Google secret sauce that we are forbidden from knowing or did they skip some weeks?
  - jiggawatts 4y ago
    Yes. VMware's ESXi kernel was unbelievably rock-solid compared to other similar systems. Fantastic resilience against even hardware faults. I've heard that there was a culture of "doing things the right way" there. Meanwhile in the same org, the group doing their GUI kept adding band-aids to a broken mess for years.
    - jeffbee 4y ago
      ESXi is a gem that few appreciate. It's been quietly chugging along as my home server hypervisor since 4.1 (2010). And it's free.
- ummonk 4y ago
  I'd say local code quality is generally good but overall it's a big hodge podge of small features duct-taped together, so the whole app becomes a tangled mess. The metric driven culture tends to emphasize impact, not diffs landed.
  - Dwolb 4y ago
    How would you measure impact?
    - zhengyi13 4y ago
      At first guess, landing/launching a new feature, and then possibly feature adoption.
    - haney 4y ago
      A change you made moved some metric a measurable amount (usually aligned with a team or product goal). You put these little nuggets of impact on your performance review.
  - bombolo 4y ago
    > I'd say local code quality is generally good Have you ever used facebook.com? At least the frontend is incredibly slow (on a thousands of € machine), so that's not synonym with quality in my experience.
    - KaoruAoiShiho 4y ago
      From reading the comment thread I would say the front end would be macro, local would be like a specific function on the front end, like the friends list, that by itself could have "good code".
      - bombolo 4y ago
        Like when I have a red number indicating I have a notification and then I click and it just loops forever on some grey animation because it doesn't manage to load the list of notification on my fiber connection?
- allenu 4y ago
  If what you’re saying is true, I wonder how much of the quality issues is just a company getting really big over time. Harder to standardize or keep tabs on such a large number of developers.
- deleted 4y ago
  [deleted]
comfypotato 4y ago
Am I reading this right? If the total median time in review is “a few hours”, that means that people are dropping what they’re doing to review the code. That can’t really be a good code review? A context switch alone would be half of that time for me.
- ummonk 4y ago
  Working there I tended to conduct code reviews each afternoon at the end of work. Sometimes after coming back from lunch as well. There isn't any "drop what they're doing" involved.
  - pavon 4y ago
    Sure, I do reviews first thing every morning, and sometimes right after lunch (mostly just rereviews), but that would give a mean/median response time of 4 hours assuming work completion time is uniformly distributed. And if changes are requested, that would add another 2-4 hours, which brings the total review-in-wait time to a full day, which the post was saying was unacceptable. To get the numbers they claim they must be reviewing more frequently than that.
    - disgruntledphd2 4y ago
      You can use timezones to your advantage here. When California is finishing, Singapore is starting and when Singapore is finishing London is starting.
      - gtirloni 4y ago
        Why would you do that unless it's a hotfix or something really critical? Are the costs negligible?
        disgruntledphd2 4y ago
        So, it depends. When I was at FB, we had one person in Europe (me), one in SF and one in Asia. Organising the diffs in this way made it much, much easier to collaborate. Given how much FB has grown since then, I suspect this would be even easier, and this is how the numbers noted above are what they are.
bsaul 4y ago
there’s so many low hanging fruits for improving the quality of diff viewing. The worst code reviews are often the ones where code get refactored, leading to piles of delete / create lines that are just code being moved or slightly renamed. One very simple approach would be better git integration with the IDE, helping build commit that make sense, where a set of changes could easily be commented by the author as they’re performing the edits, then keep improving from there.
- azinman2 4y ago
  This is a concept I almost pursed for my PhD over a decade ago. I’m still surprised no one has done this. The bigger picture: context is often missing for anything complicated, be it software or a new law. Yet many hands touch and retouch the underlying material over time. If you could capture _how_ something was built, and had enough insight into the larger process to sample some of the _why_, then you could both know what changed together and what potentially impacted the final decisions. This would result in (hypothetically) tremendous gains for anyone working on or joining a project that’s bigger than can fit in the mind of one person.
  - lamontcg 4y ago
    The PR history can answer the why, and if you can anticipate someone will ask why because you know it is edge-condition spaghetti then you can document it right in a comment.
    - azinman2 4y ago
      This is a pretty myopic stance that says everything is fine. It’s not. PR histories can be gigantic, interwoven, doesn’t tell you how and only sometimes tells you the why—-usually at a very fine grain of detail. Saying that you can “document it right” is like saying “code it well in the first place.” Much of the time there aren’t great comments or PRs, and what there is ends up assuming a huge amount of knowledge about the why/how. If you’re an outsider coming in, we should be able to do way better to help.
- 4y ago
Scubabear68 4y ago
I came to the conclusion long ago that mandatory code reviews are a waste of time. For critical stuff, absolutely. But PRs and review cycles over burden dev teams and don’t seem to move the quality needle higher one bit. A better way is to ensure multiple hands touch a given area of the code, so that multiple eyes ultimately are seeing and manipulating those bits. If they are given a task to do in that area they will be motivated to understand it (and potentially improve it). By contrast, with code reviews often the reviewer does not have time to really deep dive into the code and will only have a superficial understanding of it. Oh, also use code quality scanners to keep an eye on tactical code debt.
- rbongers 4y ago
  I have been on teams that never review code and teams that always review code, and I can confidently say I never want to work on the former again. That was with a junior team, but your team is going to have new people, if not junior, at some point. People who are familiar with each other's code and have agreed on standards can review code pretty quick. I would rather have it and not need it than need it and not have it.
- kentonv 4y ago
  The most important reason for code review is not to fix the code but rather to transfer knowledge between developers. I've learned a lot of techniques for writing better code from suggestions from my code reviewers, and from reviewing other people's code. Without code review, when will a junior developer ever learn anything from a senior developer?
  - partdavid 4y ago
    It works both ways, too--it makes more developers aware of the others' work, what changes are being made and so forth. So it's not just expertise that gets transferred, but also the current state of the codebase.
  - aftbit 4y ago
    In that case, it doesn't matter if you do the code review before or after they merge and deploy. You could read through the diffs in the commit log whenever it is convenient for you, and leave review comments for the other developer on their PR after the fact.
toast0 4y ago
When I was there, you could always just put Reviewed-By: self in the commit, and not wait. Much faster ;)
- system2 4y ago
  It might be the reason why you aren't there anymore. (kidding)
  - toast0 4y ago
    Lol, I let myself out, but appreciate the thought ;)
- philipwhiuk 4y ago
  In the future "Simon Elf" will be asked why he approved so much broken code ;)
andreygrehov 4y ago
Meta: > At Meta we call an individual set of changes made to the codebase a “diff.” GitHub: > Pull request Amazon: > Change Request GitLab: > Merge Request Google: > Changelist Nitpicking, but jesus christ, why can't we stick to a single term?
- arcturus17 4y ago
  Is diff the best of them all though, because of the implication?
  - jeffbee 4y ago
    Diff seems the best because the others describe internal details of the source code management implementation. Pull request, merge request, and changelist especially.
  - relueeuler 4y ago
    They are all diffs. Simple is better here.
    - anamexis 4y ago
      All pull/change/merge requests are diffs, but not all diffs are pull/change/merge requests.
      - relueeuler 4y ago
        Right, but in the context of making code changes, you can use diff. Which aligns with the Unix tool diff, and application of said diff with patch. So diff and patch are the operations that happen as you mutate a code base.
  - rubyist5eva 4y ago
    Mac: What implication? Dennis: The implication that things might go wrong for her if she doesn't review my code in a timely manner. Now, not that things are gonna go wrong for her, but she’s thinking that they will.
lizardactivist 4y ago
Situation: code review takes too much time. Solution: announce unprecedented layoffs of 10000 programmers. Resolution: no work to be done. code review team on schedule.
- loeg 4y ago
  11,000 employees, about half technical.
- deleted 4y ago
  [deleted]
bagels 4y ago
Here's another factor at Meta that can reduce code review time: Your performance review is based in part (maybe not a large part, but in part) on how many reviews you perform, and how many words you put in to those reviews. edit: In short, people are incentivized to review
- jiveturkey 4y ago
  I quite agree with the proposition you are making here. It goes to the very heart of the matter, does it not. The better you are at giving detailed, explicit and concrete feedback about each and every particular aspect of a diff (NOTE: both good and bad), the better and more competent you clearly are, and the more of a true champion for the cause you are proving to be. Time and time again, I wish my reviewers would just lay it all out on the table. As opposed to the limited and perfunctory LGTM
  - imiric 4y ago
    Meh. I've often seen reviewers delivering essays to back up their arguments, which essentially boil down to "because I prefer it this way". Focusing on pure word count doesn't mean that the feedback is valid, or even explains the reasoning well. If anything, it encourages nitpicky and long winded comments based on personal preference. Often, less is more. If you can get a point across by a small code suggestion, do that instead. But definitely don't fall into the trap of suggesting huge chunks of code, or rewriting parts of it. Sometimes even asking a question to improve understanding is better than arguing a point. And then other times, especially for trivial changes, "LGTM" or just a blank approval is perfectly fine as well. No need to waste time discussing trivial things if everyone is on the same boat.
    - itsdrewmiller 4y ago
      I think you're replying to a joke comment about inflating word count via loquacious reviews. But I'm not sure either, so kudos to the author if it is.
- nsenifty 4y ago
proc0 4y ago
If you're going to add machines to the process why not add it with the purpose of eliminating the human from the process all together? Reviews are necessary because compilers and linters can't catch everything. Runtime bugs that are not caught by the pipeline tend to be edge cases that don't happen until there is enough data to test (in the general sense) the feature. ML could be used for smart testing and if it passes the code diff merges automatically. It always surprises me how much software companies want to rely on human verification. The whole point of programming is to automate and let the machine take care of it. Every few years the industry does add new tools to automate process like CI/CD pipelines, but at the ground level most companies seem to favor adding more humans whenever the technology is not good enough.
- twblalock 4y ago
  Catching bugs is only one of the reasons code review is important. It is also important to transfer knowledge between developers and review design, architecture, scalability, and performance concerns.
  - proc0 4y ago
    I don't know about knowledge transfer. I feel that can be done separately and more effectively. However in the context of this article, if you're going to build an AI tool to aid the process, then why not make an AI that can be trained on a new feature and then tests code changes for bugs. The dev process has been increasingly more and more automated over the years, and I think that won't stop. The current low hanging fruits are things like rolling back the code when something breaks, and testing the code before publishing.. things that companies usually have a lengthy manual process for.
- peteradio 4y ago
  How would the AI be aware of the business logic communicated via some word doc?
  - proc0 4y ago
    I'm guessing it would be trained via manual input (+visual), i.e. recording actions on the app. The AI would repeat the process and decide if it's a pass or fail. Different AIs could also be trained on other data, like network calls, and test those as well. I'm sure something like this must exist already, but I'm not seeing efforts to integrate it with current industry practices.
underdeserver 4y ago
I'm a strong believer in fast reviews. I get into deep working mode for 3 hours a day total, on a good day. The rest is meetings, daily sync, coffee, lunch, "hey can you look at something", hallway conversations, emails, my own inability to concentrate when I'm not feeling it. I've been in this industry for coming up on ten years. None of this is going to change, unless I become an academic or a hermit. I don't get to do deep work, at least I'm going to unblock other people as fast as possible. That means out of an 8-hour work day, you're going to get a review from me within half an hour in most cases. I've been on the team for years and I have write access. I WILL merge your change if you pass my review. I find that this has immense benefits: 1) People just do things. They don't schedule design meetings, get approvals, get consensus. You know why? Because if someone has a good reason why the commit wasn't a good idea, we roll back. No harm, no foul. And guess what? It happens once in 100 commits. (If it's something truly complex, you do get a design doc approved first. But then the review is about making sure your code is correct, matches your design and our style/testing requirements, not whether it's the right thing to do.) 2) People write good commit messages. If your commit message isn't in the following format: Push foos in bar order instead of baz order. Following discussion with johnsmith, benchmark (http://<shorturl>) shows 12% improvement in the hot frobnication flow. Ticket: http://tickets/<ticket_number> I'm sending it back to you. Since I merge most of my team's code most commits look like that. 3) People write small commits. Got a bigger change? I'll ask you to split it up (without breaking the build if we ship a version between commits). People don't push back on that, because they know it's not going to add a lot of overhead. 4) In the same spirit, people don't push back on changes I request - unless it's for a good reason. Discussions are on-point. When changes are made you get back approval half an hour later. No background psychological pressure of "I wanted to get this in today and I don't want to have to restore context tomorrow morning". The velocity you reach is amazing. True serendipity. Unless you're consistently able to get full days of deep work in, I suggest you try it. (edited for formatting)
- mike_d 4y ago
  > 2) People write good commit messages. If your commit message isn't in the following format: [snip] I'm sending it back to you. Try reviewing commits without reading any associated messaging, or having your team submit complex changes with no message. You have to engage your brain to understand what the code is doing and what is being changed, you'll have a better understanding of if the comments are useful or insufficient, and most importantly you don't already have a bias that the code does what it says. You may find that when you really look at it foos are getting pushed in qux order, or that the benchmark was calling a different function.
jensvdh 4y ago
Stacked diffs are awful compared to PR's. Not every commit should be clean
- TheTomBombadil 4y ago
  Why? I found them way easier to review than regular PR during my time at fb. Since every commit is cleaner, you get fewer, higher quality commits than in a PR.
kerblang 4y ago
In the spirit of tangentialism I randomly suggest: Architecture Review! - Prevents juniors from being blown out of the ocean into startalloverland by seniors at tail end - Focus on the most dangerous aspects of the change that can't be fixed later - Sets the stage for more informed programming reviews later on (lower priority to me though)
- pvg 4y ago
  Architecture Review! Looks a lot more fun than code review to boot: https://www.youtube.com/watch?v=QfArEGCm7yM&t=57s https://www.youtube.com/watch?v=QfArEGCm7yM&t=57s
- deleted 4y ago
  [deleted]
- jeffbee 4y ago
  I've seen mixed things from architecture reviews. I've seen it used by people with titles that exceeded their actual abilities, to stop people with junior titles from doing things the senior person simply didn't understand. And I've seen architecture reviews used just to satisfy the whims of senior people, to gratify that urge to nitpick or dictate what language they wanted to use. Those are the bad ways. The good way I've seen architecture review used is nobody was going to tell you not to write or even deploy whatever the hell it was that you thought you wanted to write, but if you wanted to integrate with Grown Up Systems, there were ACLs that your system would not be added to unless and until your system had passed the review of the Grown Ups. I think this way is strictly better for two reasons: it can't strangle good ideas at birth, and it minimizes the amount of architecture reviewing that everyone needs to do, because half of the junk that gets brought to pre-implementation arch reviews never gets built anyway.
yrgulation 4y ago
Too many think code reviews are an opportunity for endless debates over personal preference. A code review should be fast and cover blatant good practice violations and architectural mistakes. Everything else should be taken care of by linters and tools. If a reviewer wants code done in a different way they can write the code themselves.
- 98codes 4y ago
  One thing I've enjoyed where I am now is that PR comments come in two flavors. The first, actual feedback. The second, borderline pedantic issues that are prefaced with "nit: " in the comment. Nit comments are safely ignored but are there so that if the author wants to put in that change while changing some other issue, then OK.
  - gknoy 4y ago
    I especially like Github's new option to make a _suggested diff_ of what you want changed. Typo fixes, comments re-worded, etc. It really reduces friction, both as the person making the suggestion, and as the person who authored the PR.
    - jeffbee 4y ago
      Yes, hopefully someday GitHub will have all the major features that Gerrit has had for years. By the time I'm ready to retire, GitHub PR review UI should be up to approximately 2010 standards.
    - agentwiggles 4y ago
      I completely agree, suggesting the actual diff is the ultimate "put up or shut up". One of the most annoying review situations is when you get some vague comment that doesn't spell out what change is being asked for. Although the best response to that sort of thing is just a quick DM - "hey, I'm not sure how to interpret this comment, wanna hop on a call for a minute and explain?" Again - put up or shut up. Want a change? Cool, let's pair on it. I want the code to be good too. But I'm not going to just read your comment and based on the vibe I'm feeling shoot off some change, only to find out that it wasn't what you meant.
osculum 4y ago
Is it just me, or in the last couple of weeks (since announcement of layoffs) there's been an increase in FB tooling/infra threads? Could be Baader–Meinhof effect, of course.
davidmurdoch 4y ago
All these comments about how code review is a waste of time, or suggest code review is only for bugs, really shine light on why so much software is incredibly slow today.
- itsdrewmiller 4y ago
  None of them engage with the content of the article either - having a "play next" button for code review is awesome assuming it works reasonably well. I'm curious about the quality of nudgebot reviews. In my experience the PRs that sit around forever are the 3000 line epic find/replace refactors all done in one commit that are impossible to really review. I skimmed the paper and didn't see any accounting for "diff time to diff length", so I'm not sure the result there is anything meaningful. Maybe people are getting faster feedback to not submit such shitty PRs.
  - riwsky 4y ago
    what stops you from reviewing the sed one-liner that created the diff, instead?
    - Leherenn 4y ago
      Reviewing the sed isn't going to help you spot the instance that shouldn't have been replaced or the missing substitution.
- logicchains 4y ago
  Maybe people'd have more time to optimise the performance of their software if they weren't spinning their wheels and context switching waiting hours for minor changes to be merged.
anikom15 4y ago
If code is important enough, it will get reviewed and tested one way or another. Everything else is a waste of time.
milin 4y ago
Somewhere in the post, it's mentioned fb uses code ownership logic in the next review engine. If folks are interested, there's project called https://github.com/milin/gitown https://github.com/milin/gitown which does something similar in github leveraging code owners.
- _boffin_ 4y ago
  Thanks
ep103 4y ago
> Next reviewable diff Holy Fuck No. 90% of my PR review time goes into "Okay, how will this change impact parts of the system that this mid-level Dev doesn't understand yet?" and "Does this PR actually do what the ticket it is claiming to implement actually intended?" Reviewing diffs in isolation completely removes one's ability to do that. If you remove a person's ability to do that, what you've left them with is the easiest part of the PR, just checking that the logic seems logical. And honestly, most of that work can be automated by linting, style cops, and unit tests. The fact that they got rid of the part of the PR review process that matters, and only saw a 1.5% improvement speaks to all sorts of problems in the process overall, not an improvement by this tool
- Arainach 4y ago
  You don't remove their ability to do that. If you need more context, then there are UI elements to show the other lines. At Google, there are also links to the file in question in code search if you want to look at history or any other related context. Most changes don't need this. A prerequisite of fast code reviews is small changes. Rather than 3000-line features, make a series of changes with 10-100 lines of code plus tests. Reviews can quickly understand the change in logic and confirm that test cases for the new codepaths are being added. Wham, bam, done in two minutes. Sure, some reviews take more time, but of them 10-30 code reviews I do a week, it's perhaps 10% of them.
- solatic 4y ago
  > "Does this PR actually do what the ticket it is claiming to implement actually intended?" Let me ask about your unspoken assumption: is this the PR reviewer's job? Maybe the PR seems to implement what the ticket asked for, then after merging it becomes clear that it didn't fully implement it, or the business stakeholders are unsatisfied, etc.?
  - ohgodplsno 4y ago
    It's not the PR reviewer's job to go actively test it out (you can assume that your colleagues are somewhat competent at what they're doing), but if you review with the spec or the issue open on the side that says to add a blue button and you see it's red, it's your job to ensure it's not a mistake and point it out to the author.
s3000 4y ago
Have I missed the feedback from the users? There should be some quotes from team members who liked the change. Their mentioning that they start being data-driven for internal tools suggests that they start treating developers like cattle and not pets. >Driving down Time In Review would not only make people more satisfied with their code review process, it would also increase the productivity of every engineer at Meta. This hasn't been tested. "The average Time In Review for all diffs dropped 7 percent" - they have verified that they changed the left side of the equation, the review time, but they haven't checked the outcome, the productivity. Overall it doesn't seem like they have checked if their changes have negative side effects. Likewise >The choice of reviewers that an author selects for a diff is very important. Diff authors want reviewers who are going to review their code well, quickly, and who are experts for the code their diff touches. doesn't match >A 1.5 percent increase in diffs reviewed within 24 hours and an increase in top three recommendation accuracy (how often the actual reviewer is one of the top three suggested) from below 60 percent to nearly 75 percent. They have shown that the people they nudge are more likely to do a code review. But are they the experts who do the review well? The 1.5 percent in reviewed diffs could also be jitter. *edit: Meta could extend the review process. There doesn't seem to be a review process for the review team. If they don't like to review their changes, or if they cannot find suitable reviewers, how are they qualified to role out their changes to the software development team?
- charcircuit 4y ago
  >They have shown that the people they nudge are more likely to do a code review. But are they the experts who do the review well? I think there's assumption that people won't just rubberstamp significant diffs to code they don't own. When submitting a change to another team's project if the reviewers that are suggested aren't actually the right person they are more likely to know the right person who should review it and they can manually add that person. >The 1.5 percent in reviewed diffs could also be jitter. Facebook / Meta has tools for measuring the effects of changes and seeing if they are statistically significant. Yes, it could still be jitter but without them giving more data about the experiment we can't tell what the chance of it being due to chance is. >There doesn't seem to be a review process for the review team There isn't a review team. Anyone can review a change.
PAREL 4y ago
PAREL 4y ago
PAREL 4y ago
penguin_booze 4y ago
> Next reviewable diff As the commit author, it's in my (and everyone's) interest to size changes up so that it's easier for review, and also present in them in the logical order of thinking. Personally, I prefer the bottom-up approach. I bring the non-functional and impertinent changes (like refactoring and tangential changes) ahead in the line-up so that the actual changes are kept separate and are concentrated at the tail end. I make commit messages of the pattern: Present situation, the problem with that, what this patch does, and what the effect it has/how it solves the problem or sets up a path forward. The initial PR might be sliced too thinly, and so will have more commits than ideal. But, as the review progresses, and once both the reviewer(s)' and the author's mental models are in sync, commits can be collapsed at their logical boundaries. Regardless of the tooling and presentation, it's imperative that that the reviewers are intuitively aware of the ramifications of the change. Without that, the review ends up being nit picking, spell checking, and whatever that's obvious on the immediate vicinity, and the process degrades into a box-ticking exercise. No AI needed. Be human.
stephenjane 4y ago
kissgyorgy 4y ago
On a much smaller scale (with a team of 8), but I also noticed this problem and wrote a “nudge bot” for Slack and Gerrit. It takes the team-relevant changes and post it to a Slack channel in a formatted message with the patch state (not reviewed, pending, needs change, etc) I made a talk about it, unfortunately in Hungarian, but you can see screenshots how it worked: https://youtu.be/7WiICWyP1sQ https://youtu.be/7WiICWyP1sQ Here is the code: https://github.com/kissgyorgy/slack-review-bot https://github.com/kissgyorgy/slack-review-bot
deleted 4y ago
[deleted]

216 points by pzrsa 4y ago

deleted 4y ago

[deleted]

Disclaimer: these are anecdotal reports. I've heard from a lot of my friends how abysmal the quality of code is at Meta. Obviously, this may not be true in all teams/products, but that's the general sentiment. Why make it faster when you're already dealing with mess! This is abundantly evident from the constant fire fighting, duct tapes and a metric driven culture that incentivizes the number of diffs landed.

rat9988 4y ago
Because making it slower won't improve anything.
cheriot 4y ago
Are there companies with a reputation for code quality?
- no-dr-onboard 4y ago
  GitLab and Netflix come to mind.
- sgeisenh 4y ago
  Google places a pretty high emphasis on code quality and readability. It's not universally great, but it's a big part of the culture. You can catch a glimpse in their [style guides][1], [abseil totws][2] and [aips][3]. Almost every change is required to be reviewed for "readability" in addition to functionality. This can feel like a lot, but it leads to pretty consistent style across the codebase which makes it a lot easier to switch projects or read and understand an unfamiliar section of the code. [1]: https://google.github.io/styleguide/ https://google.github.io/styleguide/ [2]: https://abseil.io/tips/ https://abseil.io/tips/ [3]: https://google.aip.dev/ https://google.aip.dev/
  - CharlieDigital 4y ago
    I find Google's take on code reviews to be particularly good: https://cloud.google.com/architecture/devops/devops-tech-trunk-based-development#common_pitfalls https://cloud.google.com/architecture/devops/devops-tech-tru...
    - ok_dad 4y ago
      I enjoy how they state like 3 times that code reviews should be synchronous, yet "industry-standard" (aka: what people really do) is to toss it over the fence in a PR and go back and forth for several days with stylistic bullshit.
      - Jensson 4y ago
        Code reviews are usually synchronous at Google though, commenting and fixing things is like chatting with the reviewer so are usually done quickly. Not sure why this wouldn't be industry standard, is there any reason to make code reviews more painful than that?
        ok_dad 4y ago
        I was kinda joking that places like Google might have good processes but most places just cargo cult it and do it incorrectly. My current job does reviews that are so useless I don’t even participate anymore and no one cares. I really want to work at a place where they care about code quality and an effective process.
  - aftbit 4y ago
    Oh, Abseil is new to me. Thanks! Do you know where the missing tips are? For example: https://abseil.io/tips/110 https://abseil.io/tips/110 The root page (parent's [2]) mentions this one as famous by name: >Often they are cited by number, and some have become known simply as “totw/110” or “totw/77”. Is totw/110 some Google secret sauce that we are forbidden from knowing or did they skip some weeks?
- jiggawatts 4y ago
  Yes. VMware's ESXi kernel was unbelievably rock-solid compared to other similar systems. Fantastic resilience against even hardware faults. I've heard that there was a culture of "doing things the right way" there. Meanwhile in the same org, the group doing their GUI kept adding band-aids to a broken mess for years.
  - jeffbee 4y ago
    ESXi is a gem that few appreciate. It's been quietly chugging along as my home server hypervisor since 4.1 (2010). And it's free.
ummonk 4y ago
I'd say local code quality is generally good but overall it's a big hodge podge of small features duct-taped together, so the whole app becomes a tangled mess. The metric driven culture tends to emphasize impact, not diffs landed.
- Dwolb 4y ago
  How would you measure impact?
  - zhengyi13 4y ago
    At first guess, landing/launching a new feature, and then possibly feature adoption.
  - haney 4y ago
    A change you made moved some metric a measurable amount (usually aligned with a team or product goal). You put these little nuggets of impact on your performance review.
- bombolo 4y ago
  > I'd say local code quality is generally good Have you ever used facebook.com? At least the frontend is incredibly slow (on a thousands of € machine), so that's not synonym with quality in my experience.
  - KaoruAoiShiho 4y ago
    From reading the comment thread I would say the front end would be macro, local would be like a specific function on the front end, like the friends list, that by itself could have "good code".
    - bombolo 4y ago
      Like when I have a red number indicating I have a notification and then I click and it just loops forever on some grey animation because it doesn't manage to load the list of notification on my fiber connection?
allenu 4y ago
If what you’re saying is true, I wonder how much of the quality issues is just a company getting really big over time. Harder to standardize or keep tabs on such a large number of developers.
deleted 4y ago
[deleted]

comfypotato 4y ago

Am I reading this right? If the total median time in review is “a few hours”, that means that people are dropping what they’re doing to review the code. That can’t really be a good code review? A context switch alone would be half of that time for me.

ummonk 4y ago
Working there I tended to conduct code reviews each afternoon at the end of work. Sometimes after coming back from lunch as well. There isn't any "drop what they're doing" involved.
- pavon 4y ago
  Sure, I do reviews first thing every morning, and sometimes right after lunch (mostly just rereviews), but that would give a mean/median response time of 4 hours assuming work completion time is uniformly distributed. And if changes are requested, that would add another 2-4 hours, which brings the total review-in-wait time to a full day, which the post was saying was unacceptable. To get the numbers they claim they must be reviewing more frequently than that.
  - disgruntledphd2 4y ago
    You can use timezones to your advantage here. When California is finishing, Singapore is starting and when Singapore is finishing London is starting.
    - gtirloni 4y ago
      Why would you do that unless it's a hotfix or something really critical? Are the costs negligible?
      - disgruntledphd2 4y ago
        So, it depends. When I was at FB, we had one person in Europe (me), one in SF and one in Asia. Organising the diffs in this way made it much, much easier to collaborate. Given how much FB has grown since then, I suspect this would be even easier, and this is how the numbers noted above are what they are.

bsaul 4y ago

there’s so many low hanging fruits for improving the quality of diff viewing. The worst code reviews are often the ones where code get refactored, leading to piles of delete / create lines that are just code being moved or slightly renamed. One very simple approach would be better git integration with the IDE, helping build commit that make sense, where a set of changes could easily be commented by the author as they’re performing the edits, then keep improving from there.

azinman2 4y ago
This is a concept I almost pursed for my PhD over a decade ago. I’m still surprised no one has done this. The bigger picture: context is often missing for anything complicated, be it software or a new law. Yet many hands touch and retouch the underlying material over time. If you could capture _how_ something was built, and had enough insight into the larger process to sample some of the _why_, then you could both know what changed together and what potentially impacted the final decisions. This would result in (hypothetically) tremendous gains for anyone working on or joining a project that’s bigger than can fit in the mind of one person.
- lamontcg 4y ago
  The PR history can answer the why, and if you can anticipate someone will ask why because you know it is edge-condition spaghetti then you can document it right in a comment.
  - azinman2 4y ago
    This is a pretty myopic stance that says everything is fine. It’s not. PR histories can be gigantic, interwoven, doesn’t tell you how and only sometimes tells you the why—-usually at a very fine grain of detail. Saying that you can “document it right” is like saying “code it well in the first place.” Much of the time there aren’t great comments or PRs, and what there is ends up assuming a huge amount of knowledge about the why/how. If you’re an outsider coming in, we should be able to do way better to help.
4y ago

Scubabear68 4y ago

I came to the conclusion long ago that mandatory code reviews are a waste of time. For critical stuff, absolutely. But PRs and review cycles over burden dev teams and don’t seem to move the quality needle higher one bit. A better way is to ensure multiple hands touch a given area of the code, so that multiple eyes ultimately are seeing and manipulating those bits. If they are given a task to do in that area they will be motivated to understand it (and potentially improve it). By contrast, with code reviews often the reviewer does not have time to really deep dive into the code and will only have a superficial understanding of it. Oh, also use code quality scanners to keep an eye on tactical code debt.

rbongers 4y ago
I have been on teams that never review code and teams that always review code, and I can confidently say I never want to work on the former again. That was with a junior team, but your team is going to have new people, if not junior, at some point. People who are familiar with each other's code and have agreed on standards can review code pretty quick. I would rather have it and not need it than need it and not have it.
kentonv 4y ago
The most important reason for code review is not to fix the code but rather to transfer knowledge between developers. I've learned a lot of techniques for writing better code from suggestions from my code reviewers, and from reviewing other people's code. Without code review, when will a junior developer ever learn anything from a senior developer?
- partdavid 4y ago
  It works both ways, too--it makes more developers aware of the others' work, what changes are being made and so forth. So it's not just expertise that gets transferred, but also the current state of the codebase.
- aftbit 4y ago
  In that case, it doesn't matter if you do the code review before or after they merge and deploy. You could read through the diffs in the commit log whenever it is convenient for you, and leave review comments for the other developer on their PR after the fact.

toast0 4y ago

When I was there, you could always just put Reviewed-By: self in the commit, and not wait. Much faster ;)

system2 4y ago
It might be the reason why you aren't there anymore. (kidding)
- toast0 4y ago
  Lol, I let myself out, but appreciate the thought ;)
philipwhiuk 4y ago
In the future "Simon Elf" will be asked why he approved so much broken code ;)

andreygrehov 4y ago

Meta: > At Meta we call an individual set of changes made to the codebase a “diff.” GitHub: > Pull request Amazon: > Change Request GitLab: > Merge Request Google: > Changelist Nitpicking, but jesus christ, why can't we stick to a single term?

arcturus17 4y ago
Is diff the best of them all though, because of the implication?
- jeffbee 4y ago
  Diff seems the best because the others describe internal details of the source code management implementation. Pull request, merge request, and changelist especially.
- relueeuler 4y ago
  They are all diffs. Simple is better here.
  - anamexis 4y ago
    All pull/change/merge requests are diffs, but not all diffs are pull/change/merge requests.
    - relueeuler 4y ago
      Right, but in the context of making code changes, you can use diff. Which aligns with the Unix tool diff, and application of said diff with patch. So diff and patch are the operations that happen as you mutate a code base.
- rubyist5eva 4y ago
  Mac: What implication? Dennis: The implication that things might go wrong for her if she doesn't review my code in a timely manner. Now, not that things are gonna go wrong for her, but she’s thinking that they will.

lizardactivist 4y ago

Situation: code review takes too much time. Solution: announce unprecedented layoffs of 10000 programmers. Resolution: no work to be done. code review team on schedule.

loeg 4y ago
11,000 employees, about half technical.
deleted 4y ago
[deleted]

bagels 4y ago

Here's another factor at Meta that can reduce code review time: Your performance review is based in part (maybe not a large part, but in part) on how many reviews you perform, and how many words you put in to those reviews. edit: In short, people are incentivized to review

jiveturkey 4y ago
I quite agree with the proposition you are making here. It goes to the very heart of the matter, does it not. The better you are at giving detailed, explicit and concrete feedback about each and every particular aspect of a diff (NOTE: both good and bad), the better and more competent you clearly are, and the more of a true champion for the cause you are proving to be. Time and time again, I wish my reviewers would just lay it all out on the table. As opposed to the limited and perfunctory LGTM
- imiric 4y ago
  Meh. I've often seen reviewers delivering essays to back up their arguments, which essentially boil down to "because I prefer it this way". Focusing on pure word count doesn't mean that the feedback is valid, or even explains the reasoning well. If anything, it encourages nitpicky and long winded comments based on personal preference. Often, less is more. If you can get a point across by a small code suggestion, do that instead. But definitely don't fall into the trap of suggesting huge chunks of code, or rewriting parts of it. Sometimes even asking a question to improve understanding is better than arguing a point. And then other times, especially for trivial changes, "LGTM" or just a blank approval is perfectly fine as well. No need to waste time discussing trivial things if everyone is on the same boat.
  - itsdrewmiller 4y ago
    I think you're replying to a joke comment about inflating word count via loquacious reviews. But I'm not sure either, so kudos to the author if it is.
nsenifty 4y ago

proc0 4y ago

If you're going to add machines to the process why not add it with the purpose of eliminating the human from the process all together? Reviews are necessary because compilers and linters can't catch everything. Runtime bugs that are not caught by the pipeline tend to be edge cases that don't happen until there is enough data to test (in the general sense) the feature. ML could be used for smart testing and if it passes the code diff merges automatically. It always surprises me how much software companies want to rely on human verification. The whole point of programming is to automate and let the machine take care of it. Every few years the industry does add new tools to automate process like CI/CD pipelines, but at the ground level most companies seem to favor adding more humans whenever the technology is not good enough.

twblalock 4y ago
Catching bugs is only one of the reasons code review is important. It is also important to transfer knowledge between developers and review design, architecture, scalability, and performance concerns.
- proc0 4y ago
  I don't know about knowledge transfer. I feel that can be done separately and more effectively. However in the context of this article, if you're going to build an AI tool to aid the process, then why not make an AI that can be trained on a new feature and then tests code changes for bugs. The dev process has been increasingly more and more automated over the years, and I think that won't stop. The current low hanging fruits are things like rolling back the code when something breaks, and testing the code before publishing.. things that companies usually have a lengthy manual process for.
peteradio 4y ago
How would the AI be aware of the business logic communicated via some word doc?
- proc0 4y ago
  I'm guessing it would be trained via manual input (+visual), i.e. recording actions on the app. The AI would repeat the process and decide if it's a pass or fail. Different AIs could also be trained on other data, like network calls, and test those as well. I'm sure something like this must exist already, but I'm not seeing efforts to integrate it with current industry practices.

underdeserver 4y ago

I'm a strong believer in fast reviews. I get into deep working mode for 3 hours a day total, on a good day. The rest is meetings, daily sync, coffee, lunch, "hey can you look at something", hallway conversations, emails, my own inability to concentrate when I'm not feeling it. I've been in this industry for coming up on ten years. None of this is going to change, unless I become an academic or a hermit. I don't get to do deep work, at least I'm going to unblock other people as fast as possible. That means out of an 8-hour work day, you're going to get a review from me within half an hour in most cases. I've been on the team for years and I have write access. I WILL merge your change if you pass my review. I find that this has immense benefits: 1) People just do things. They don't schedule design meetings, get approvals, get consensus. You know why? Because if someone has a good reason why the commit wasn't a good idea, we roll back. No harm, no foul. And guess what? It happens once in 100 commits. (If it's something truly complex, you do get a design doc approved first. But then the review is about making sure your code is correct, matches your design and our style/testing requirements, not whether it's the right thing to do.) 2) People write good commit messages. If your commit message isn't in the following format: Push foos in bar order instead of baz order. Following discussion with johnsmith, benchmark (http://<shorturl>) shows 12% improvement in the hot frobnication flow. Ticket: http://tickets/<ticket_number> I'm sending it back to you. Since I merge most of my team's code most commits look like that. 3) People write small commits. Got a bigger change? I'll ask you to split it up (without breaking the build if we ship a version between commits). People don't push back on that, because they know it's not going to add a lot of overhead. 4) In the same spirit, people don't push back on changes I request - unless it's for a good reason. Discussions are on-point. When changes are made you get back approval half an hour later. No background psychological pressure of "I wanted to get this in today and I don't want to have to restore context tomorrow morning". The velocity you reach is amazing. True serendipity. Unless you're consistently able to get full days of deep work in, I suggest you try it. (edited for formatting)

mike_d 4y ago
> 2) People write good commit messages. If your commit message isn't in the following format: [snip] I'm sending it back to you. Try reviewing commits without reading any associated messaging, or having your team submit complex changes with no message. You have to engage your brain to understand what the code is doing and what is being changed, you'll have a better understanding of if the comments are useful or insufficient, and most importantly you don't already have a bias that the code does what it says. You may find that when you really look at it foos are getting pushed in qux order, or that the benchmark was calling a different function.

jensvdh 4y ago

Stacked diffs are awful compared to PR's. Not every commit should be clean

TheTomBombadil 4y ago
Why? I found them way easier to review than regular PR during my time at fb. Since every commit is cleaner, you get fewer, higher quality commits than in a PR.

kerblang 4y ago

In the spirit of tangentialism I randomly suggest: Architecture Review! - Prevents juniors from being blown out of the ocean into startalloverland by seniors at tail end - Focus on the most dangerous aspects of the change that can't be fixed later - Sets the stage for more informed programming reviews later on (lower priority to me though)

pvg 4y ago
Architecture Review! Looks a lot more fun than code review to boot: https://www.youtube.com/watch?v=QfArEGCm7yM&t=57s https://www.youtube.com/watch?v=QfArEGCm7yM&t=57s
deleted 4y ago
[deleted]
jeffbee 4y ago
I've seen mixed things from architecture reviews. I've seen it used by people with titles that exceeded their actual abilities, to stop people with junior titles from doing things the senior person simply didn't understand. And I've seen architecture reviews used just to satisfy the whims of senior people, to gratify that urge to nitpick or dictate what language they wanted to use. Those are the bad ways. The good way I've seen architecture review used is nobody was going to tell you not to write or even deploy whatever the hell it was that you thought you wanted to write, but if you wanted to integrate with Grown Up Systems, there were ACLs that your system would not be added to unless and until your system had passed the review of the Grown Ups. I think this way is strictly better for two reasons: it can't strangle good ideas at birth, and it minimizes the amount of architecture reviewing that everyone needs to do, because half of the junk that gets brought to pre-implementation arch reviews never gets built anyway.

yrgulation 4y ago

Too many think code reviews are an opportunity for endless debates over personal preference. A code review should be fast and cover blatant good practice violations and architectural mistakes. Everything else should be taken care of by linters and tools. If a reviewer wants code done in a different way they can write the code themselves.

98codes 4y ago
One thing I've enjoyed where I am now is that PR comments come in two flavors. The first, actual feedback. The second, borderline pedantic issues that are prefaced with "nit: " in the comment. Nit comments are safely ignored but are there so that if the author wants to put in that change while changing some other issue, then OK.
- gknoy 4y ago
  I especially like Github's new option to make a _suggested diff_ of what you want changed. Typo fixes, comments re-worded, etc. It really reduces friction, both as the person making the suggestion, and as the person who authored the PR.
  - jeffbee 4y ago
    Yes, hopefully someday GitHub will have all the major features that Gerrit has had for years. By the time I'm ready to retire, GitHub PR review UI should be up to approximately 2010 standards.
  - agentwiggles 4y ago
    I completely agree, suggesting the actual diff is the ultimate "put up or shut up". One of the most annoying review situations is when you get some vague comment that doesn't spell out what change is being asked for. Although the best response to that sort of thing is just a quick DM - "hey, I'm not sure how to interpret this comment, wanna hop on a call for a minute and explain?" Again - put up or shut up. Want a change? Cool, let's pair on it. I want the code to be good too. But I'm not going to just read your comment and based on the vibe I'm feeling shoot off some change, only to find out that it wasn't what you meant.

osculum 4y ago

Is it just me, or in the last couple of weeks (since announcement of layoffs) there's been an increase in FB tooling/infra threads? Could be Baader–Meinhof effect, of course.

davidmurdoch 4y ago

All these comments about how code review is a waste of time, or suggest code review is only for bugs, really shine light on why so much software is incredibly slow today.

itsdrewmiller 4y ago
None of them engage with the content of the article either - having a "play next" button for code review is awesome assuming it works reasonably well. I'm curious about the quality of nudgebot reviews. In my experience the PRs that sit around forever are the 3000 line epic find/replace refactors all done in one commit that are impossible to really review. I skimmed the paper and didn't see any accounting for "diff time to diff length", so I'm not sure the result there is anything meaningful. Maybe people are getting faster feedback to not submit such shitty PRs.
- riwsky 4y ago
  what stops you from reviewing the sed one-liner that created the diff, instead?
  - Leherenn 4y ago
    Reviewing the sed isn't going to help you spot the instance that shouldn't have been replaced or the missing substitution.
logicchains 4y ago
Maybe people'd have more time to optimise the performance of their software if they weren't spinning their wheels and context switching waiting hours for minor changes to be merged.

anikom15 4y ago

If code is important enough, it will get reviewed and tested one way or another. Everything else is a waste of time.

milin 4y ago

Somewhere in the post, it's mentioned fb uses code ownership logic in the next review engine. If folks are interested, there's project called https://github.com/milin/gitown https://github.com/milin/gitown which does something similar in github leveraging code owners.

_boffin_ 4y ago
Thanks

ep103 4y ago

> Next reviewable diff Holy Fuck No. 90% of my PR review time goes into "Okay, how will this change impact parts of the system that this mid-level Dev doesn't understand yet?" and "Does this PR actually do what the ticket it is claiming to implement actually intended?" Reviewing diffs in isolation completely removes one's ability to do that. If you remove a person's ability to do that, what you've left them with is the easiest part of the PR, just checking that the logic seems logical. And honestly, most of that work can be automated by linting, style cops, and unit tests. The fact that they got rid of the part of the PR review process that matters, and only saw a 1.5% improvement speaks to all sorts of problems in the process overall, not an improvement by this tool

Arainach 4y ago
You don't remove their ability to do that. If you need more context, then there are UI elements to show the other lines. At Google, there are also links to the file in question in code search if you want to look at history or any other related context. Most changes don't need this. A prerequisite of fast code reviews is small changes. Rather than 3000-line features, make a series of changes with 10-100 lines of code plus tests. Reviews can quickly understand the change in logic and confirm that test cases for the new codepaths are being added. Wham, bam, done in two minutes. Sure, some reviews take more time, but of them 10-30 code reviews I do a week, it's perhaps 10% of them.
solatic 4y ago
> "Does this PR actually do what the ticket it is claiming to implement actually intended?" Let me ask about your unspoken assumption: is this the PR reviewer's job? Maybe the PR seems to implement what the ticket asked for, then after merging it becomes clear that it didn't fully implement it, or the business stakeholders are unsatisfied, etc.?
- ohgodplsno 4y ago
  It's not the PR reviewer's job to go actively test it out (you can assume that your colleagues are somewhat competent at what they're doing), but if you review with the spec or the issue open on the side that says to add a blue button and you see it's red, it's your job to ensure it's not a mistake and point it out to the author.

s3000 4y ago

Have I missed the feedback from the users? There should be some quotes from team members who liked the change. Their mentioning that they start being data-driven for internal tools suggests that they start treating developers like cattle and not pets. >Driving down Time In Review would not only make people more satisfied with their code review process, it would also increase the productivity of every engineer at Meta. This hasn't been tested. "The average Time In Review for all diffs dropped 7 percent" - they have verified that they changed the left side of the equation, the review time, but they haven't checked the outcome, the productivity. Overall it doesn't seem like they have checked if their changes have negative side effects. Likewise >The choice of reviewers that an author selects for a diff is very important. Diff authors want reviewers who are going to review their code well, quickly, and who are experts for the code their diff touches. doesn't match >A 1.5 percent increase in diffs reviewed within 24 hours and an increase in top three recommendation accuracy (how often the actual reviewer is one of the top three suggested) from below 60 percent to nearly 75 percent. They have shown that the people they nudge are more likely to do a code review. But are they the experts who do the review well? The 1.5 percent in reviewed diffs could also be jitter. *edit: Meta could extend the review process. There doesn't seem to be a review process for the review team. If they don't like to review their changes, or if they cannot find suitable reviewers, how are they qualified to role out their changes to the software development team?

charcircuit 4y ago
>They have shown that the people they nudge are more likely to do a code review. But are they the experts who do the review well? I think there's assumption that people won't just rubberstamp significant diffs to code they don't own. When submitting a change to another team's project if the reviewers that are suggested aren't actually the right person they are more likely to know the right person who should review it and they can manually add that person. >The 1.5 percent in reviewed diffs could also be jitter. Facebook / Meta has tools for measuring the effects of changes and seeing if they are statistically significant. Yes, it could still be jitter but without them giving more data about the experiment we can't tell what the chance of it being due to chance is. >There doesn't seem to be a review process for the review team There isn't a review team. Anyone can review a change.

PAREL 4y ago

penguin_booze 4y ago

> Next reviewable diff As the commit author, it's in my (and everyone's) interest to size changes up so that it's easier for review, and also present in them in the logical order of thinking. Personally, I prefer the bottom-up approach. I bring the non-functional and impertinent changes (like refactoring and tangential changes) ahead in the line-up so that the actual changes are kept separate and are concentrated at the tail end. I make commit messages of the pattern: Present situation, the problem with that, what this patch does, and what the effect it has/how it solves the problem or sets up a path forward. The initial PR might be sliced too thinly, and so will have more commits than ideal. But, as the review progresses, and once both the reviewer(s)' and the author's mental models are in sync, commits can be collapsed at their logical boundaries. Regardless of the tooling and presentation, it's imperative that that the reviewers are intuitively aware of the ramifications of the change. Without that, the review ends up being nit picking, spell checking, and whatever that's obvious on the immediate vicinity, and the process degrades into a box-ticking exercise. No AI needed. Be human.

stephenjane 4y ago

kissgyorgy 4y ago

On a much smaller scale (with a team of 8), but I also noticed this problem and wrote a “nudge bot” for Slack and Gerrit. It takes the team-relevant changes and post it to a Slack channel in a formatted message with the patch state (not reviewed, pending, needs change, etc) I made a talk about it, unfortunately in Hungarian, but you can see screenshots how it worked: https://youtu.be/7WiICWyP1sQ https://youtu.be/7WiICWyP1sQ Here is the code: https://github.com/kissgyorgy/slack-review-bot https://github.com/kissgyorgy/slack-review-bot

deleted 4y ago

[deleted]