14 ms·

The Theatre of Pull Requests and Code Review

• kaapipo 1y ago
  I mean, stacked PRs are a thing for a reason
  • keriati1 1y ago
    I can also recommend rather to use the stacked PR approach. We have it since years, PR review "issues" are not a thing for us. I still encourage do to a lot of small commits with good commit messages, but don't submit more then 2-3 or 4 commits in a single PR...
  • epage 1y ago
    I see stacked PRs as independent of this. PRs are a good unit of cohesion of changes, particularly changes that only make sense if later changes are also merged.
  • kiitos 1y ago
    stacked PRs are wildly reviewer-hostile, please do not do them
• ValleZ 1y ago
  If you split all the changes for a feature this way not only you hide the way all changes interact with each other but also make the development at least 10x longer because an average approval time is often more than a day.
  • fidotron 1y ago
    This will be accompanied by the sort of dev manager that thinks a KPI for "number of PRs merged" won't in any way be gamed or backfire. I don't know what they're doing where you can do code reviews in 5-10 minutes, but in my decades doing this that only works for absolutely trivial changes.
    • StilesCrisis 1y ago
      The goal here is to make almost all CLs trivial enough that they can be reviewed quickly. You can compose almost any feature out of many small simple changes.
      • fidotron 1y ago
        > You can compose almost any feature out of many small simple changes. You can?
        • StilesCrisis 1y ago
          Follow these guidelines and you'll be surprised how far you can go. https://google.github.io/eng-practices/review/developer/small-cls.html#what-is-small https://google.github.io/eng-practices/review/developer/smal...
          • fidotron 1y ago
            So you want code added in 100 line sections behind one feature flag which we then suddenly turn on? Isn't that exactly how Google's latest big cloud outage happened? EDIT: referring to https://news.ycombinator.com/item?id=44274563 https://news.ycombinator.com/item?id=44274563 I should add that the idea complexity relates to LoC is also nonsense. Everyone that's been doing this for a while knows that what kills people are those one line errors which make assumptions about the values they are handling due to the surrounding code.
        • SAI_Peregrinus 1y ago
          Yes, and you can waste a ton of a coworker's time doing it. Say you're upgrading to a new library, it has a breaking change to an API. First, add `#if`s or similar around each existing change to the API that check for the existing library vs the new version, and error if the new version is found. No behavior change, one line of code, trivial PR. One PR per change. Bug your coworker for each. Next, add calls to the new API, but don't actually change to use them (the `#if`s won't hit that condition). Another stack of trivial PRs. Your coworker now hates you. Finally, swap the version over. Hopefully you tested this. Then make a final PR to do the actual upgrade. For something less trivial than a single breaking upgrade, you can do the same shit. Conditionally compile so that your code doesn't actually get used in the version you do the PR in, you can split out to one PR per character changed! It'll be horrible, everyone will hate you, but you can split changes down to ridiculously small sizes.
  • viraptor 1y ago
    > only you hide the way all changes interact with each Splitting the change does not prevent you from looking at diffs of any combination of those commits. (Including whole pr) You're not losing anything. > at least 10x longer because an average approval time is often more than a day. Why do you think it would take longer to review? Got any evidence?
    • eigencoder 1y ago
      I think approval time would take much longer. The issue is that while actual time spent in review may be shorter, there's a lot of context-switching time costs that increase with the number of PRs submitted. No one on the team is just sitting there refreshing the list of PRs, ready to pick one up immediately. There's a delay between when the PR is marked as ready and when someone can actually get to it. Everyone is trying to get work done and have some time daily in flow state. Imagine you have a change; you could do it as one PR that takes 1 hour to review, or 3 small PRs that each take 15 mins to review. The time spent in review may even be shorter for the small PRs, but if each PR has a delay of 1 hour before a reviewer can get to it, then the 3 PRs will take almost 4 hours before they're done, as opposed to just 2 hours for one big PR.
      • viraptor 1y ago
        I don't think that's a realistic view of the timeline. I've done features as multiple PRs and there are really two cases: 1. I can't submit pieces until I have the final version. PRs go up at the same time and can be reviewed one after another immediately. 2. There's a very specific split that makes that feature two features in reality. Like adding a plugin system and the first plugin. Then the first part gets submitted while I still work on the second part and there's no delay on my side, because I'm still developing anyway. Basically, I've never seen the "but if each PR has a delay of 1 hour before a reviewer can get to it," getting serialised in practice. It's still either one time or happening in the background.
• jvanderbot 1y ago
  PR review is probably at least a little performative. But I trust my colleagues to do good reviews when I ask them to, and to ignore my PRs when I don't. That's kind of the way we all want it. I regularly ask for a review of specific changes by tagging them in a comment on the lines in question, with a description of the implications and a direct question that they can answer. This, "throw the code at the wall for interpretation" style PR just seems like it's doomed to become lower priority for busy folks. You can add a fancy narrative to the comments if you like, but the root problem is that presenting a change block as-is and asking for a boolean blanket approval of the whole thing is an expensive request. There's no substitute for shared understanding of the context and intent, and that should come out-of-band of the code in question.
  • pards 1y ago
    > and to ignore my PRs when I don't PRs should be optional, IMHO. Not all changes require peer review, and if we trust our colleagues then we should allow them to merge their branch without wasting time with performative PRs.
    • fidotron 1y ago
      There is a difference between a code review and approval to merge/release. Part of the difference is the idea you can catch all problems with piecemeal code review is nonsense, so you should have at least some sweeping QA somewhere.
    • flir 1y ago
      I always appreciate an extra pair of eyeballs, even on a one-liner. Everyone's an idiot sometimes.
      • eCa 1y ago
        I’m firmly in this boat too. If it’s a small change I can likely get it reviewed within minutes, if it isn’t small it should have a review regardless.
    • comprev 1y ago
• dakiol 1y ago
  There’s a fine difference between a) splitting a big feature in parts that interact with each other via well defined interfaces, and b) splitting a big feature in parts that are suitable for PRs You can split a big feature in N MRs and that doesn’t necessarily mean the N MRs are easier to understand (and so to review) than a single big MR. Taking into account the skills of the average software engineer, I prefer to review a single big MR than N different and not very well connected MRs (the classic example is that MR number N looks good and innocent, but then MR number N+1 looks awful… but since MR number N was already approved and merged the incentives are not there to redo the work)
• nenenejej 1y ago
  300 LOC in 10 minutes. Or 2 sec per loc. Or for average 30 char line, 600wpm reading speed. OK. There is little you can review properly in 10 minutes unless you were already pairing on it. You might have time to look for really bad production-breaking red flags maybe. Remember the underlying reasons for PR. Balance between get shit done and operational, quality and tech debt concerns. Depending on what your team needs you can choose anything from no review at all to 3x time reviewing than coding. What is right depends on your situation. PR is a tool.
  • rustybolt 1y ago
    Agreed, 300 lines will take me a lot more than 10 minutes to review properly! Depends on the specific changes of course, but generally speaking.
    • whstl 1y ago
      Also depends on the codebase. 300 lines is nothing in some boilerplate-heavy codebases I've worked at. After seeing the same patterns for the hundredth time, it's easy to detect deviations. Not to mention linters and typing helps a lot too. Not a fan of those but oh well.
  • enlyth 1y ago
    I like to actually checkout the branch I'm reviewing and run the code myself to observe if it does what is claimed, that usually takes up at least 10 minutes in itself, sometimes more. From my experience most of the issues I find are actually from this type of observation rather than actually reading the code and imagining what it does in my head.
    • qiine 1y ago
      the mind remains a poor compiler ;p
  • franktankbank 1y ago
    Your linter/tests are for catching real errors. Review is to understand the shape of it mostly IMO. I could probably fairly easily review 300 loc if its not a particularly confused shape.
• epage 1y ago
  Agree with the overall sentiment but disagree with > A good rule of thumb is 300 lines of code changes - once you get above 500 lines, you're entering unreviewable territory. I've found LoC doesn't matter when you split up commits like they suggest. What does matter is how controversial a change is. A PR should ideally have one part at most that generates a lot of discussion. The PR that does this should ideally also have the minimal number of commits (just what dossn't make sense standalone). Granted this take experience generally and experience with your reviewers which is where metrics like LoC counts can come in handy.
  • padjo 1y ago
    It’s often an inverse correlation in well functioning environments. Controversial changes and bug fixes are small and targeted and are deeply reviewed for unintended side effects, while large change sets are often new work that’s been discussed upfront and follows well established patterns and gets waved through.
  • SOLAR_FIELDS 1y ago
    Good luck getting 90% of devs to commit (har har) to this level of history surgery. Of the ones that actually know how to do it (a small fraction of your typical engineer) an even smaller fraction of that is going to have the patience to do it correctly. You’ll tell devs to do this kind of thing and you’ll either have their eyes glaze over from lack of understanding, annoyance at the extra work, or nodding then apathetical disregard. The one top engineer will do it then be frustrated that the rest of the org doesn’t do proper commit hygiene. It’s not really something you can easily enforce with automation, so basically unachievable unless you are like Netflix and only hiring top performers. And you aren’t like Netflix.
    • StilesCrisis 1y ago
      You also need tools that support the workflow. I love small self-explanatory commits. In git, it's easy to do. Recently I switched to a Perforce organization and it's a disaster. P4 doesn't support stacked CLs and it dramatically hurts engineering quality. Everyone lands mega-CLs because it's the only supported workflow.
• rjmunro 1y ago
  Another tip: Use `git log --first-parent` and `git log --merges` to hide the intermediate commits. `--first-parent` also works with `blame` in modern git. These mean you don't have to look at all the small commits when browsing history, only when you want to dive in deeper.
• btreecat 1y ago
  My guide to good PRs: - Keep PR messages short and to the point. - use as many commits as you need, it's all the same branch. Squash if you want, I think it hides valuable meta. - put the ticket in the branch name. Non negotiable. - Update your ticket with progres. Put as much details as you can, as if you were writing to someone who's picking up the task after you. - add links to your ticket. Docs, readme, o11y graphs, etc. - Link ticket in PR for easy access to additional info - Admit if you don't understand what your looking at. Better to pair and keep moving forward. - if you request changes, stay available for conversation and approval for the next few hours. - punt the review if you don't feel like you can legitimately engage with the content right now. Make sure you communicate it though. - Avoid nits. This causes a loss in momentum and v rarely is worth changing.
• ekidd 1y ago
  A lot depends on your goals for your code reviews. And your goals might even be different for different parts of the code base. - Are you trying to make sure that more than one human has seen the code? Then simply reading through a PR in 10 minutes and replying with either a LGTM or a polite version of WTF can be fine. This works if you have a team with good taste and a lot of cleanly isolated modules implementing clear APIs. The worst damage is that one module might occasionally be a bit marginal, but that can be an acceptable tradeoff in large projects. - Does every single change need to be thoroughly discussed? Then you may want up-front design discussions, pairing, illustrated design docs, and extremely close reviews (not just of the diffs, but also re-reviewing the entire module with the changes in context). You may even want the PR author to present their code and walk throuh it with one or more people. This can be appropriate for the key system "core" that shapes everything else in the system. - Is half your code written by an AI that doesn't understand the big picture, that doesn't really understand large-scale maintainability, and that cuts corners and _knowingly_ violates your written policy and best practices? Then honestly you're probably headed for tech debt hell on the express train unless your team is willing to watch the AI like hawks. Even one clueless person allowing the AI to spew subtlety broken code could create a mess that no number of reviewers could easily undo. In which case, uh, maybe keep everything under 5,000 lines and burn it all down regularly, or something?
  • pm215 1y ago
    I think the other thing that often muddies the waters in discussions of code review is that open source projects and internal codebases are generally in rather different situations. An internal codebase is usually worked on by a fairly small group of experienced people, who are both creating and also reviewing PRs for it. So: - the baseline "can I assume this person knows what they're doing?" level is higher - making the "create PR" process take longer in order to make the review process faster is only a tradeoff of the time within the team - if something is wrong with the committed code, the person who wrote it is going to be around to fix it But in open source projects, there are much more often contributions by people outside the "core" long-term development team, where: - you can't make the same assumptions that the contributor is familiar with the codebase, so you need to give things extra scrutiny - there are often many fewer people doing the code review than there are submitting changes, so a process that requires more effort from the submitter in order to make the reviewer's job easier makes sense - if there's a problem with the code, there's no guarantee that the submitter will be available or interested in fixing it once it's got upstream, so it's more important to catch subtle problems up front and these tend to mean that the code-review process is tilted more towards "make it easy for reviewers, even if that requires more work from the submitter".
• surgical_fire 1y ago
  Can't relate. I take code reviews as possibly the most important part of my job as a developer. Suggesting extra tests, thinking about unintended side effects, and yes, aiming for consistency and readability, without being picky on style choices. I trust my colleagues to do the same (and they often do). I can't imagine working in an environment where this is a theater.
  • gjgtcbkj 1y ago
    It sounds like a good job where the most important part is finding other people’s mistakes. Though I do appreciate the shoutout to adding tests in CR. But returning a PR solely because it doesn’t have tests, is effective, but a little performative too. It kind of like publicly executing someone, theirs gotta be some performance for it to be a deterrent. If something doesn’t have tests my review is going to be a very short performance where I pretend read the rest of the code. Then immediately send it back.
    • surgical_fire 1y ago
      > It sounds like a good job where the most important part is finding other people’s mistakes. And reviews are not that. Systems are complex, and having a mental model of complex systems is difficult. Everyone has blind spots. A fresh pair of eyes can often spot what who was coding would not. > But returning a PR solely because it doesn’t have tests, is effective, but a little performative too. And this is not what I said. I spoke of suggesting extra tests. A scenario that wasn't covered, for example.
    • roguecoder 1y ago
      It's interesting: the original definition of "performative" is "a speech-act that changes something about the world". And that basically describes all of programming: we are building metaphors that will run electricity at a higher or lower voltage, and be translated again into something meaningful to a different human. In many ways, all we are doing is performing. And that is some of what makes this job challenging: the practices that build software well are all just ways of checking how humans will interact with the ones and zeros we've encoded. Returning a PR because it doesn't have tests means that code will have automated validation, which is a real change. It also means the code will be written in a testable way: too often we don't realize we wrote code in a way that is hard to test unless we try to write the tests. And on a larger level, it means that this team of engineers will learn and use the practices and tools that lead to testable code and effective tests and more easily-changeable code. It makes total sense to not keep reading if there aren't tests, because adding the tests can be expected to change the code. But just because that is a performance doesn't mean it doesn't profoundly change the world.
• 2sk21 1y ago
  Reviewing someone else's large pull request is like having a second task in parallel with what you are working on yourself!
  • chrisweekly 1y ago
    It's not "like" another task, it IS another task!
    • osigurdson 1y ago
      Yeah but it is just a quick look, "yep", "yep", "oh what about this"?, "wow we dodged a bullet there". Its like self managed error correction that the collective does on its own. Fast, simple and produces good results. The less software you write the more this resonates.
  • eterm 1y ago
    So don't do it in parallel. Completely park other tasks, spend time on the review and record that time appropriately. There's nothing wrong with saying you spent the previous day doing a large review. It's a part of the job, it is "what you're working on".
    • sexyman48 1y ago
      You might as well go into HR. Everyone knows reviewing other people's PRs is like nurturing their kids at the expense of your own.
      • eterm 1y ago
        I don't understand what you're trying to say.
      • mystifyingpoi 1y ago
        Well, then just don't play the game. Make a decision in the team, that everyone accepts everyone's PR immediately without any review. At least you won't have to wait.
• Supermancho 1y ago
  > Story-Telling Commit Messages No thank you. Talking to future ME, I don't need to know how I got to what I want me to look at. A squashed ticket-by-ticket set of merges is enough for me.
  • smashedtoatoms 1y ago
    I'm editing this to be nicer. I'm really trying to be nicer. Consider the possibility you're not the only one in the codebase and that the git history might provide the why to the code's what.
    • leoedin 1y ago
      I do think commit messages should give some reference to what they're changing. However, in more than a decade of software development, I don't think I've ever got much use out of commit messages. The only reason I'd even look at them is if git blame showed a specific commit introduced a bug, or made some confusing code. And normally the commit message has no relevant information - even if it's informative, it doesn't discuss the one line that I care about. Perhaps the only exception would be one line changes - perhaps a change which changes a single configuration value alongside a comment saying "Change X to n for y reason". Comments can be a bit better - but they have the nasty habit of becoming stale.
      • g-b-r 1y ago
        > However, in more than a decade of software development, I don't think I've ever got much use out of commit messages. > normally the commit message has no relevant information Maybe that's why you've never got much use of them? If your colleagues or yourself wrote better commits, they could have been of use. An easily readable history is most important in open source projects, when ideally random people should be able to verify what changed easily. But it can also be very useful in proprietary projects, for example to quickly find the reason of some code, or to facilitate future external security reviews (in the very rare case where they're performed).
    • ziml77 1y ago
• vdupras 1y ago
  A while ago at a past job, I was working on OpenERP (now called Odoo I think). This "community" had instated this kind of "mandatory community review" policy so that each change had to be reviewed by X developers from other organizations. I kind of virtuous web of review. But the thing is: this code is terrible and huge chunks of it are a unholy mix and match of code written for very specific purpose for this or that client, with this very weird "falsely generalized" code. I don't know how to call that: you have some very specific code, but you insert useless and probably buggy indirections everywhere so that it can be considered "general". The worst kind of code. Anyways, I was asked by my boss to do such a review. I look at it and I realize that building a database setup to be able to properly run that code is going to take me weeks because I'm going to have to familiarize myself with tons and tons of modules I don't know about. So I write down my estimate in our tracker: 1 month. He comes back to me, alarmed. A whole month? Well yeah, otherwise I can't even run the code. All you have to do is look at the code! What? No way, that ain't a review. Well, I ask you to do it as such. I'm not writing LGTM there. So I was never asked to do reviews there again (in fact, I stopped working on OpenERP at all), but I could see "LGTM" popping up from my colleagues. By the way, on OpenERP tracker, all you ever saw in review logs was "LGTM" and minor style suggestions. Nothing else. What a farce. So yeah, as the article says, there are some "LGTM-inducing" type of PRs, but the core of the problem is developers accepting to do this "LGTM-stamping" in the first place. Without them, there would only be reviewable PRs.
• conartist6 1y ago
  If your goal is to lower the velocity of your organization, e.g. because in practice code churn or poor quality are major problems, then by all means do this. If you still need to move fast, then don't. This is the "don't run in the hallways" version of software culture, but I would contend that you should choose your pace based on your situation. It's just like gradient descent really. The be efficient sometimes you need to make big hops, sometimes small ones.
  • gorgoiler 1y ago
    Your comment includes capital letters and punctuation! These are skills we all learn to ensure our writing is as legible as possible to the reader. Quotes, periods, commas, clauses, paragraphs etc. are all stylistic decisions that support the underlying text and message. I contend that learning the art of story telling through a stack of patches is just as important and, once learned, comes just as naturally as utilizing vocabulary, grammar, syntax and style with the written word.
    • deleted 1y ago
      [deleted]
  • roguecoder 1y ago
    Changing cowboy code is much, much slower than changing good code. If you need to move fast for the next two weeks, sure. If you need to move fast for the next year, you are better off collaborating.
    • OsrsNeedsf2P 1y ago
      You're making the faulty assumption the whole project won't get scrapped
    • conartist6 1y ago
      Yeah, cowboy coding as a strategy makes most sense if it's very likely that the next person to look at the code will be you. I'm not advocating either way as superior, but cowboy coding shouldn't mean that you don't pay your tech debt. It just means that it's much more common to roll a bug fix or small factoring improvement in with a feature, probably because you were already touching that code and testing it. If prod bugs are so critical that there will be a rollback and a forensic retrospective on each one, then yeah you should bite the bullet and use all the most defensive PR tactics. If prod bugs have small costs and you can quickly "roll forwards" (ship fixes) then it's better to get some free QA from your users, who probably won't mind the occasional rough edge if they're confident that overall quality is OK and bugs they do find won't stay unfixed for years.
• octo888 1y ago
  What I've often found is that people only really accept feedback from the Tech Lead, and peers are dismissed (not outright and not obviously - kind of sealioning etc). Peer-to-peer code reviews are another instance implementing a thing that pretends hierarchy does not exist. You can only get basic tweaks accepted. The sunk-cost fallacy is a huge force. Maybe I've only worked at crappy places
  • gjgtcbkj 1y ago
    Allen Iverson got criticized by the media for “letting down his teammates” for skipping a few practices. He famously said “We’re talking about practice, practice! not the game! How is MY going to practice gonna make THEM better?!” He got flack for those comments, but what he said was accurate. If we’re talking about outcomes you’re beholden to the person who is seen as the difference maker, all the teamwork in the world isn’t going to improve an individual’s the lack of ability.
    • sexyman48 1y ago
      Every company depends on 3-5 guys for their competitive advantage. While the other thousand guys are interchangeable, they're just as necessary (and needful) to the company's survival.
    • roguecoder 1y ago
      For an alternative perspective, I recommend the business book The Captain Class. If you aren't making your teammates better, and they aren't making you better, you will never be able to be as good as a team that is greater than the sum of its parts. Individual genius is consistently beat by professional collaboration.
      • sexyman48 1y ago
        Consistently? You don't know that. There are just as many successful one-man shows -- with the important caveat that they lack redundancy.
  • roguecoder 1y ago
• gjgtcbkj 1y ago
  Every developer I know who applies this sort of “highly documented development” approach where they “work through their thought process openly.” Is only doing it because their thought processes are already so funky and counterintuitive that reviewers actively reject their work unless have written evidence that the developer didn’t just entirely change the scope of their assignment to justify the bizarre decisions.
  • g-b-r 1y ago
    You only know poor developers then, I guess
  • watwut 1y ago
    That is 100% not my experience. People who do that tend to be perfectionists trying to cross all the ts. Confused developers are just unable to create such reasoning.
• osigurdson 1y ago
  Unless you have a broader context, reviewing 300 line PRs in 5 minutes is going to be surface level at best. Plus that time comes with an expensive context switch so the actual cost is likely more like 20 - 30 minutes. At this point, I think a reasonable question is why not just use AI for shallow reviews like this? This would free up bandwidth in situations where you really want code reviewed by someone else.
  • roguecoder 1y ago
    One of the advantages of small, frequent code reviews is that a team shares the broader context. Which is far more valuable than ten minutes of extra typing.
    • osigurdson 1y ago
      There are better ways to get broader context. Such as getting people to work in other areas of the code. A 5 minute review isn't going to do much in this regard.
• davedx 1y ago
  It's a very common refrain but I don't really agree with it: "How do you create a PR that can be reviewed in 5-10 minutes? By reducing the scope. A full feature should often be multiple PRs. A good rule of thumb is 300 lines of code changes - once you get above 500 lines, you're entering unreviewable territory." The problem with doing this is if you're building something a lot bigger and more complex than 500 lines of code, splitting that up across multiple PR's will result in: - A big queue of PR's for reviewers to review - The of the feature is split across multiple change sets, increasing cognitive load (coherence is lost) - You end up doing work on branches of branches, and end up either having to become a rebase ninja or having tons of conflicts as each PR gets merged underneath you The right answer for the size of a PR is NOT in lines of code. Exercise judgement as to what is logically easier to review. Sometimes bigger is actually better, it depends. Learn from experience, communicate with each other, try to be kind when reviewing and don't block things up unnecessarily.
  • CuriouslyC 1y ago
    100%. I think the right answer is to break features into atomic commits, but keep PRs at the feature level. This reduces the PR friction, while letting reviewers easily view change sets for specific features, and if a specific feature needs a patch you don't need to do any rebase gymnastics, just add a patch commit. AI agents like frequent checkpoints because the git diff is like a form of working memory for a task, and it makes it easy to revert bad approaches. Agents can do this automatically so there isn't much of an excuse not to do it, but it does require some organization of work before prompting.
  • yunohn 1y ago
    > You end up doing work on branches of branches, and end up either having to become a rebase ninja or having tons of conflicts as each PR gets merged underneath you +100 to this. My job should be thoughtfully building the solution, not playing around with git rebase for hours.
    • tcoff91 1y ago
      Just use jj instead of git and cut your rebasing time by 95%. Suddenly rebasing a stack of branches becomes 1 command.
• osigurdson 1y ago
  The PR type approach comes from the Linux kernel where there is essentially a hierarchy of gatekeepers with increasing trust and responsibility. It is a very individualistic type approach (i.e. if you merged it into your branch, it's on you and speed is a non-goal for the most part). This is often different from many software projects as it is like a collective where often, no one really has individual responsibility for anything (it is more like collective responsibility), speed is everything and there are certainly no gate keepers - maybe the review actually doesn't matter that much in this context.
  • g-b-r 1y ago
    If security or bugs don't matter much for your project, sure
    • osigurdson 1y ago
      I don't think "LGTM" helps with either of those. Making someone personally responsible is the real unlock here.
      • g-b-r 1y ago
        Personally responsible for the review? For sure if you can say LGTM without even looking at anything it doesn't make much sense
• mtlynch 1y ago
  I love code reviews and blog posts about them, but I vehemently disagree with all of this advice. >His example PR[0] adds just 152 lines of code, removes 2 lines, but uses 13 thoughtful commits. >While some developers might understand those 152 lines from the final diff alone, I couldn't confidently approve it without the commit story. This is ridiculous! You absolutely can and should review a PR without demanding its "commit story." Go read the PR under discussion here.[0] There's nothing about it that's hard to understand or that demands you go read the 13 intermediate steps the developer took to get there. The unit of change in a code review is the PR itself, not the intermediate commits. Intermediate commits are for the author's benefit, not the reviewer's. If the author rewrote the code in FORTRAN to help them understand the problem, then converted it back to the codebase's language, that's 100% okay and is not something the reviewer needs to care about. The PR should squash the individual PRs at merge time. The linked PR[0] is a perfect example, as the relevant change in the permanent commit history should be "Measure average scheduler utilization" and not "Collect samples" or "Mock sampling." When you need to communicate extra context outside of the code, that should go in the PR description.[1] Your reviewer shouldn't have to go scour dozens of separate commit messages to understand your change. >How do you create a PR that can be reviewed in 5-10 minutes? By reducing the scope. A full feature should often be multiple PRs. A good rule of thumb is 300 lines of code changes - once you get above 500 lines, you're entering unreviewable territory. 5-10 minute reviews are so low that it's basically thoughtless rubber-stamping. If someone spent 5-10 hours making a change, the reviewer should definitely think about it for more than 5 minutes. If all the reviewer is doing is spot checking for obvious bugs, it's a waste of a code review. The reviewer should be looking for opportunities to make the code simpler, clearer, or more maintainable. 5-10 minutes is barely enough time to even understand the change. It's not enough time to think deeply about ways to improve it. [0] https://github.com/sasa1977/hamlet/pull/3 https://github.com/sasa1977/hamlet/pull/3 [1] https://refactoringenglish.com/chapters/commit-messages/ https://refactoringenglish.com/chapters/commit-messages/
  • tantalor 1y ago
    Hard agree. Commits are not important. As an author, you should not waste your time on this. As a reviewer, just ignore them.
  • 1y ago
• stack_framer 1y ago
  My manager recently told our team that "AI usage" would be added to our engineering competencies, and we would all be expected to "use AI more." When I said my top preference for AI usage, by far, would be to eliminate human code reviews, the response was basically, "Oh, not like that."
  • CuriouslyC 1y ago
    Don't worry, if you actually increase AI usage your team will be forced to automate code review, either explicitly or implicitly.
  • BowBun 1y ago
    That's a bummer! At my company we've started investing in what I'm calling 'semantic linting', which is basically running GPT over a PR with a set of rules that we iterate on. Already I'm finding huge value for style/pattern comments that linters can't easily catch, dropping warnings for common DB migration footguns, or notifying people of changing patterns/new ways of doing things. Been great so far!
    • reitanuki 1y ago
      Do you have any write-up about this or more info? It sounds like a useful use case but I haven't yet got it right
      • BowBun 1y ago
        I do not, but it took me and a coworker all of an hour to setup. Create a CI workflow, vibe code it to load up the PR contents + any rules you want (we maintain a directory of individual rules that people can tweak), and ship it off to GPT for a response. Add some input/output schemas, and it's pluggable back into the CI hooks to notify of failed build steps (Github in my case).
  • Entelligence25 1y ago
    Rn 70% of ai is being used only for code review documentation purpose, i think we really need entire engg workflows to be ai automated for better team insights, better team performace , sprint assesment etc and track entire engg lifecycle. Vision should not be AI code, but it should be AI beyond code.
• karel-3d 1y ago
  If you want to let the review in, you need to make some obvious issues - typo or space/tab thing - so you give the reviewer some bone so they feel like they did something and accept your request otherwise.
  • patrickmay 1y ago
    This is the drawback of teaching some software developers social skills. /s
  • bonzini 1y ago
    Kinda like "the queen's duck" (https://bwiggs.com/notebook/queens-duck/ https://bwiggs.com/notebook/queens-duck/).
• iamleppert 1y ago
  I've tried his advice several times and it's been a complete failure. Splitting up a PR into a bunch of little PR's causes more problems than it solves, and it makes it 10x harder for the reviewer, no matter how much they complain about long PR's. Now they need to suss out some kind of ordering of the PR's, and navigate between multiple change sets for changes that depend on one another. It doesn't matter how well you can isolate the "concerns" a frontend depends on backend API, etc. You end up creating more work for the reviewer, and most people just simply won't do the work of a proper review. You also don't have the advantage of any CI or tests running across the entire set of changes, so you also have separate CI reports to review. All this adds up for more places for bugs to hide or happen. All the same risks are still there, and you've also added a few more points of failure in the splitting process. And for what? To end up, most likely merging in one PR after the next, for a feature that should just be all logically grouped together, or just squashing and merging the PR's together anyway.
  • jghn 1y ago
    This. I have colleagues who helpfully break things into small PRs, but more often than not I wish they hadn’t. I usually want to review things in context of the big picture and that gets lost. Usually what I do is check out their last PR, figure out what I want to say, and then identify the appropriate place to leave a comment in their stack of PRs. Which is a lot more work for me. And this assumes that they’ve even finished all their PRs instead of expecting them to merge in one at a time
    • tcoff91 1y ago
      What I find helps with stacked PRs is it helps with getting code review incrementally as the various changes for a larger effort come together.
• williamcotton 1y ago
  Pro tip: Get your potential code reviewers involved before you even start coding. Keep them abreast of the implementation. When it comes time for a review they should be acquainted with your work.
  • ed_blackburn 1y ago
    This. Absolutely this. The PR should be the final step in the process. Never the first. Ambushing people with PRs and then demanding their attention is a massive time and mood sink. It is ineffective and counter-productive. You may as well just commit to main, and honestly, in so many situations I think that's perfectly rational thing to do, so much about PR culture is theatre.
    • Jcowell 1y ago
      This seems like a bit of an over exaggeration. A pull request isn’t just a chunk of code thrown at people. it’s an entire process with a Title , description, pipeline checks that all come together to say I want this in another branch. As the PR author it should be your job to: * Self Review the PR * Ensure you adhere/fulfill to all the expectations and requirements a PR should have before it’s pulled out of draft * That all pipeline test pass * That for a given request X there is test that validate X, Not X , and edge cases of X and are ran in the pipeline. * Has a clear description of what you’re changing/adding/removing, why, how, and the rollout plan , roll back plan , & the risk level. The peer review process should make the reviewer engage in a rubber duck process to review their code , loop the team in for changes that can change their mental model of how a system they own works, and to catch things that we might not catch ourselves. Not to the mention the security implications
  • keeda 1y ago
    I feel like this is a good use of standups rather than just status updates. "I have started working on X" should ideally be accompanied by "I am planning on doing this using Y and Z" if there is any chance that it could be contentious. There should ideally be no contentions, but the moment any questions come up, they should be tabled for a post-standup huddle amongst the relevant stakeholders. If you don't do standups, just do the same thing ad-hoc on the team chat channel. Larger tasks and discussions are probably the purview of RFCs.
• deleted 1y ago
  [deleted]
• markus_zhang 1y ago
  Story from friend but I can relate: Some teams and code are pretty much unreviewable and the best thing is to add CI for simple tests and lgtm if there is no glaring mistakes. My team only has 3 including the manager, so, eh, each one holds a lot of knowledge that only he or she knows. Documentation? Yeah that’s a good idea, but I don’t have time to read them because “We want to ship as fast as possible”. So I just put up a precommit for testing and plug in the same tests for the CI and call it a day. If you pass the CI I’ll take a cursory look and LGTM. Some code is unreviewable. I work as a DE and it’s all business logic entangled. Why is there a specific id excluded? What is the purpose of this complex join? I mean, the reviewer is not supposed to know everything, right? So the best thing, again, is to only look at the technical things (is the join done properly), let the CI figure out the weird stuffs and LGTM.
  • g-b-r 1y ago
    I mean, the reviewer is not supposed to know everything, right? Uhm he kind of is, if you want a proper review...
  • roguecoder 1y ago
    "Why is the specific id excluded?" is exactly the kind of thing we can capture in code, and that a good code review will flag. It takes two seconds to write `EVIL_IDS_THAT_STOLE_OUR_LUNCH_MONEY=[1] [...] NOT IN EVIL_IDS_THAT_STOLE_OUR_LUNCH_MONEY` instead of `NOT IN [1]` and then not hate your past self six months from now when you have to figure out why something has been excluded. If some code can't be understood today, it's not going to be able to be understood when someone comes to modify it. Maybe in your domain all the code is write-once-read-never, but for those of us maintaining enterprise software that isn't an option. We absolutely expect our reviewers to fully understand the code. If you want a quick shortcut to make that cultural change, have any bugs in code go to the code reviewer instead of the original author. You will find people start taking code reviews a whole lot more seriously.
    • markus_zhang 1y ago
      > We absolutely expect our reviewers to fully understand the code. I suspect you don’t work as a DE or your company has top notch culture. I have never worked in any companies (including some big ones) that encourage a full understanding of any code submitted to PR review.
• danielfalbo 1y ago
  Jane Street implements an awesome code review system: https://janestreet.com/tech-talks/janestreet-code-review https://janestreet.com/tech-talks/janestreet-code-review > [...] Telling a Story with Commits [...] > [...] it should take the average reviewer 5-10 minutes [...] Jane Street code review system kinda solves this problem by - making each commit a branch, - stacking branches on top of each other (gracefully handling rebases and everything that comes with it), and - reviewing one iteration at a time So one reviews single commits independently (takes probably around 5mins), and "forces" the reviewer to re-live the story that led to the bigger diff. I do not work at Jane Street but I frequently find myself pondering on how broken the common code review system/culture is. I've heard of tools like graphite.dev that build on top of git to provide a code review system similar to the Jane Street one, but I'm not an active user yet (I just manually stack PRs, keep them small and ask for review to one at a time to my colleagues, and handle the rebasing etc. manually myself for now).
  • huflungdung 1y ago
    [dead]
  • maleldil 1y ago
    > handle the rebasing etc. manually myself for now jj[1] is helpful for this. If you have a chain of commits like A -> B -> C, and you make a change to A, the rest of the chain is automatically rebased. [1] https://jj-vcs.github.io/ https://jj-vcs.github.io/
  • deleted 1y ago
    [deleted]
  • mtoner23 1y ago
    This is also the system we used with gerrit. Googles PR system. It uses cherry picks instead of branches but it's fantastic and gracefully splits the PR work between reviewer and committer
• jmull 1y ago
  I think a more fundamental and important aspect to this is developing a shared understanding of the design the change is ultimately intended to accomplish and the roadmap to achieving that. Shared between the implementer and the reviewers, that is, which means design brainstorming, design formalization of some kind (writing the significant aspects down, or recording them in some other way), and a review process. I should also say: this process doesn't have to be any larger or more heavy-weight than the change itself. And changes that don't have a design aspect can skip it entirely. But this article is talking about building a story with commits, and at that level you're almost surely talking about a significant design aspect.
• bob1029 1y ago
  I see the primary value of a pull request being the simple awareness of what is being worked on. I aggressively sync my local changes to PRs that are marked draft in GitHub. Other developers I work with do the same. Throughout the day we asynchronously check in on the scope of the others' work. If there is something that looks like it might conflict, we call a meeting. The actual code review phase for me is more about making sure the checkin is clean and that what I am intending to work on wont get caught up in a conflicting mess. The code review is NOT a recurring opportunity to purity test my teammates. Presumably, the reason they are working with us in the first place is because they already succeeded at this. "Trust but verify" is a fun trope if you are working somewhere the consequences of a mistake are one-way and measured in millions of dollars. However, a bad commit can be reverted in 10 seconds. Builds of software can be easily recreated. Deploying to production is still sensitive, but why get all weird about rapidly iterating through dev or QA environments?
  • roguecoder 1y ago
    Why do you see feedback as "purity test"ing? Newspaper reporters are professionals and they still have editors. We've all seen the disaster that results when an author gets too famous to edit. And we don't have to go in and work with their prose later. Code reviews are where "my" code becomes "our" code: I want my coworkers to feel comfortable with and fully understand and be happy to support the changes I am proposing to our software.
    • bob1029 1y ago
      > Why do you see feedback as "purity test"ing? I didn't say this. There are many forms of extremely valuable feedback that do not involve subjecting your teammates to a ritual of "clean code" every time a PR is submitted.
• saghm 1y ago
  I feel like quite a lot of the pain is best solved for both sides by inverting the expectations a little towards the author of the PR like this mentions, but around communication, not the substance of the changes itself. Over the years I've built up a bit of a reputation on some teams I've worked on for crafting PRs that are disproportionately easy to review relative to the scope, and it pretty much is entirely from just spending a bit of extra time explaining things in the review comments itself. In addition to the top-level description (which in practice I've doing found often is something people who are understandably busy will just glance through quickly if not skip it entirely to jump right into the diff), I always go through my own code in the review tool without publishing and tend to add a fairly high number of comments explaining things that might stick out as odd in the context of the diff specifically (with comments in the code making more sense for things that will stick out regardless of context). My experience is that for a lot of potential review comments, it's not particularly hard to anticipate just from looking at the diff from the perspective of the reviewer, and it takes far less time as the author to look through and add comments explaining those cases than it does as a reviewer to go through and write up comments on all of those places (especially given that as a reviewer, I do think it makes sense to be thoughtful about how exactly to phrase comments on a PR given how easily tone can be misunderstood over text). My perception is that even going a bit overkill with the self-review doesn't hurt too much; often I'll notice that certain comments get a "thumbs-up" reaction compared to others that don't, which is a nice quick way for reviewers to signify that they understand what I've said and find it reasonable (compared to the comments with no reaction that I assume didn't end up being necessary to address a potential concern). I picked up this habit from an early teammate (and manager, who eventually went back to just being a teammate because he didn't love being a manager) who recommended it, and in places I've worked where they've had struggles with their review culture, I've had colleagues express to me how much they love that they do this and mention to me that they've sometimes started asking other teammates to do it for certain changes (e.g. "some of this code looks like it might have gotten moved around without changing, but it's not obvious from the diff, do you think you could go through and note wherever that happened?"). At the end of the day, teams will function best when there's mutual good faith and respect for each other's time. (Obviously some teams are lacking this to various degrees, but at that point I don't don't think code review is really the larger problem, but just symptom of the larger underlying dynamic that either needs to somehow be addressed or the team will never work well). Recognizing where you can save your team time overall by spending some of your own is a pretty useful with that in mind, and code review ends up having quite a lot of low-hanging fruit in this regard both because the context that the PR author has tends to make the amount of effort needed to preemptively help the reviewers understand things is quite low compared to the reviewer needing to ask, and because the return on time spent by the author scaling with the number of reviewers.
• manoDev 1y ago
  Code review is a good way to ensure 1) the team has context 2) double-check the approach taken 3) keep conceptual integrity high. For that, you should make atomic PRs (not focus on minimal LoC, but rather a coherent patch set for a feature/bugfix), agree with your team that reviewing code is work and set appropriate time aside for it. Also, code review should be ego free: 1) criticize the code, not the author 2) don’t be too attached to code written, the objective is the product and not number of LoC contributed 3) it’s okay to start from scratch after learning about a better approach, or even make more than one and compare approaches. Where most teams fail is treating it as a gatekeeping process rather than context sharing, make PRs too small to be meaningful or only waste time arguing about code style and other minutiae.
• Dan42 1y ago
  I see posts like this one pop up from time to time. I love it. Based on my 30y of exp that's also the workflow I converged on. It seems to me like every experienced and skilled developer is converging on this. jujutsu is entirely built to accommodate this workflow. There are no silver bullets or magical solutions, but this is as close to one as I've ever seen. A true "best practice" distilled from the accumulated experience of our field, not from someone with something to sell.
• devmor 1y ago
  I actually didn't ever realize that some people dreaded code reviews. To me, PRs are one of the most exciting parts of the process. That's where you have the most potential to learn from your colleagues and/or teach them something you know.
  • roguecoder 1y ago
    I am very glad that has been your experience! It depends very much on your coworkers, unfortunately. When a team is all pulling in the same direction, and is kind and constructive and rigorous, code reviews can be awesome. In some companies, especially ones with stack ranking where one person doing better means someone else does worse, it is easy for them to go horribly awry and become an ordeal of a bug hunt. The obvious solution is to not work at horrible companies that pit engineers against each other, but when that describes some of the biggest employers in the industry it's easier said than done.
• parpfish 1y ago
  i don't know how people can build something and leave behind a coherent series of commits that tell a nice story of progressively building a thing. my commits include lots of false-starts that get abandoned and "i need to commit this interim state because i deprioritized this and will come back later". the sequence of events that i used to build the thing isn't necessarily the best sequence of events to tell the story of what the final thing is. sometimes you can get a good story by stacking PRs, but if the stack gets too deep you can end up with some rebase nightmares.
  • Master_Odin 1y ago
    I'm a strong believer that PRs should be merged via a "squash and merge" strategy, with the singular commit being descriptive of the overall change and having a link back to the PR for deeper story analysis as needed. I'm also a staunch believer at this point that PRs should really focus on one thing as well. If when working on a bug you discover another semi-related bug? Open two PRs. Let main be the story of how code got from point A to B, and PRs be the story of how each incremental step was made.
    • g-b-r 1y ago
      PRs only live on GitHub, what happens if it gets shut down or it accidentally loses some data?
      • kiitos 1y ago
        the entire repo lives only on github as well, there is no meaningful difference between git commits and PR comments in a github-hosted repo
        • g-b-r 1y ago
          Uhm you might want to look up git clone
• niko_dex 1y ago
  This post came up at a perfect time for me. I'd been feeling like my commits were too small, littering the activity pane on GitLab. I guess I was worried that nobody would want to read the commits, but I really like the thought that what I've been providing is a simple narrative thread to help guide a reader through my train of thought.
• necrotic_comp 1y ago
  My preferred way of doing PRs/Code Review echoes some of the statements below, but also requires the engineer to do the following: 1) Each PR (even if it's part of a larger whole) can be committed and released independently. 2) Each PR has a description of what it's doing, why it's doing what it's doing, and if it's part of a larger whole, how it fits into the broader narrative. 3) PRs are focused - meaning that QOL or minor bugfixes should not be part of a change that is tackling something else. 4) PRs are as small as possible to cover the issue at hand. 5) All PRs are tested and testing evidence is presented. 6) When a PR is committed to master, the final narrative in step 1) is the Git commit, along with the testing evidence, includes the JIRA ticket number in the headline, and the reviewer in the body of the git commit. This way we have a clean, auditable, searchable history with meaningful commit history that can help reconstruct the narrative of a change and be used as a guide when looking at a change in, say, a year.
• huflungdung 1y ago
  [dead]
• gagabity 1y ago
  I never read the commit messages always straight to changed files. Also find doing it like this either incredibly hard or have to do a ton of git magic after I'm done to get commits into this state which is very frustrating. I think it might be the codebase I work on but who knows.
• WorldMaker 1y ago
  I wish GitHub's PR UI was better at walking through a PR one commit at a time. As someone who does try to make good commits, and as someone who does try to read PRs sometimes a commit at a time, GitHub's UI gets in the way and keeps trying to drive you back to "whole PR" reviews. It is very telling in the article itself there is a screenshot of the commits tab in the PR workflow that many don't realize even exists and/or never think to use. In the Files tab the commit picker has gotten better in recent years, but it is still overly focused on selecting ranges of commits over individual ones, and there's no shortcuts to easily jump Next or Previous commit, you have to remember your place and interact with the full pulldown every time. Also, it's hard to read the full descriptions of commits in the Files view and I find I often have to interrupt my flow to open the commit in another browser tab or flip back and forth between the Commits tab and the Files tab in the PR. The Commits tab also defaults to hiding most of the commit descriptions, so it's still not a particularly great reading experience. It feels like a bit of a bad feedback loop that GitHub's UI doesn't make commit-by-commit reviewing clean/easy because GitHub themselves don't expect most developers to write good commits, but a lot of developers don't write good commits today simply because GitHub's PR interface is bad at reviewing individual commits and developers don't see as much of a point in it if they aren't going to be reviewed in that way.
  • candiddevmike 1y ago
    Can you describe a little more about why you walk through individual commits instead of just reviewing the latest commit only?
    • XorNot 1y ago
      Because the idea is that each commit that you submit is a coherent unit of work. That's why we have commit messages.
    • WorldMaker 1y ago
      Good commits can tell a story. (The article here discusses this, too, and suggests that good commits should tell a story.) When a commit author takes the time to fill out the commit message, it will include things like the "how" and "why" of each step, what they were thinking about as they worked on that part of the whole PR. Often I find that will save you from asking questions like "Why did you take this approach?" which I see all the time in PRs (and make all the time in PRs with "bad" or just "save point" commits). The PR should be the smallest unit of integration (this works and builds and is ready to merge to the next steps), but the commit is the smallest unit of any progress at all. Ideas don't come fully formed and ready to compile. Progress sometimes includes back tracks and experiments. Good commits say things "hey, I learned from this thing that wasn't working and that's what pushed me into this next direction". They document the journey, why a specific path was taken, what obstacles were in the way, what other paths were explored and dismissed. Some PR authors can capture a lot of that in a PR description as well, but commits tie that to specific context of the code at a point in time in ways that a PR description often can't, without linking to commits (in which case the commits again speak for themselves). But yes, not everyone can or will write good commits. I see that partly as a tooling failure because our tools themselves like GitHub PRs don't encourage it, often fail to reward it. I've seen plenty PRs full of commits named only "WIP" and "Fix" and "oops", but the best PRs tell a story in the commits, have meaningful descriptions on each commit. I would love for our tools to encourage more of those because I think they are better PRs; often easier to review PRs and enough good PRs like that form a string of documentation that you can search through (through git blame and git log if nothing else, but that's still a lot of useful research data). (If you keep that data, I know a lot of people like squash merges because they distrust the git DAG and how many tools show ugly or hard to read "subway diagrams" for it instead of simpler collapsible views. But that's another long conversation.)
• KronisLV 1y ago
  > This makes logical sense, but it's challenging to implement because it can feel like admitting we're not smart enough to understand the code. However, saying "I don't understand this enough to approve it" is far more valuable than pretending with an empty "LGTM". Sounds nice but I’m sure that there are projects out there that are like constantly being in the trenches, testing in prod and the original developers being long gone, where the devs manage to barely keep alive this WH40kesque monstrosity. Where all of the code has a lot of incidental complexity that will just never be resolved. Alternatively, there’s probably countries and companies out there where that attitude would get you flagged as someone who is slowing down the velocity and get you fired - because there’s an oversaturation of devs in the local market and you’re largely an expendable cog. Surely there’s other methods, formal or LLM driven that could be used to summarize changes and help explore everything bit by bit (especially when there’s just one big commit that says “fix” as a part of the PR). Sometimes I get too caught up with coming up with contrived scenarios where “best effort” just will never happen but I bet someone out there is living that reality right now.
  • roguecoder 1y ago
    If that's what the code is like and you aren't allowed to make it better, that is not a safe workplace. Especially if the software matters at all. I purposefully find jobs in fields like healthcare and physics and finance where it actually matters that the software works. Right now, if there is a bug in my code people could die. And in that case, there are worse things than being fired. If people do find themselves in that situation, the best answer is "unionize". The second best answer is to work with your coworkers to adopt better practices (it is very unlikely that they are going to fire you all all at once). And the third best answer is to do the job well, regardless of what is going on around you, and if you get fired you get fired.
    • KronisLV 1y ago
      > If people do find themselves in that situation, the best answer is "unionize". The second best answer is to work with your coworkers to adopt better practices (it is very unlikely that they are going to fire you all all at once). And the third best answer is to do the job well, regardless of what is going on around you, and if you get fired you get fired. That's a pretty nice way to view things! I've been lucky enough to mostly be in environments where I can work towards that (even if there is the occasional enterprise legacy project that can feel as a maze). But at the same time, even in better circumstances, people can get lazy, I certainly do, and are sometimes also set in their ways. In those cases, I'll take any automated tools that will make it easier to do things in more organized and better ways - e.g. automatic code formatting with a linter for consistency, or various scripts to run on the CI server, to check codebase against a specific set of rules (e.g. "If you add a reusable component, you should also add it somewhere in the showcase page"). I wonder what automation and tooling could be used to manage the complexity of some pull requests, even when people have deadlines or sub-optimal understanding of the whole codebase, or any number of other reasons that make things more difficult. Things like better code navigation tools (even in the web UI), or more useful dependency or call graphs, a bit like what SourceTrail did back in the day, before going bust: https://github.com/CoatiSoftware/Sourcetrail https://github.com/CoatiSoftware/Sourcetrail
• dvcoolarun 1y ago
  It’s a challenge: you put the craftsmanship into a PR, but the reviewer might not share that dedication—they just want to give an LGTM bypass to clear their queue. That's why working at a company that truly values these practices is essential.
• deleted 1y ago
  [deleted]
• javier_e06 1y ago
  I see Pull Requests and Code Reviews the same way as making a sandwich for someone else. My team wants for me prepare a sandwich ASAP. They, the reviewers, have to eat it, not me. Some reviewers want wonder bread bread with a slice of spam. Some want hand-made spreads with home-grown vegetables. Is easier to fix a sandwich than a wedding cake. The take-away Don't do wedding cakes.
• roblh 1y ago
  Maybe I'm just a scrub, but something that I find makes it harder to do smaller commits is that I frequently rely on being able to see which lines I've changed directly inline in my editor. When you commit, vscode now stops highlighting all of those lines, and that makes it much more difficult for me to orient myself relative to what I've already done. The individual lines, and the git pane that shows which files have been changed, act as waypoints for me while I'm working on stuff. It's particularly important on more complicated features that span more files, and I'll often intentionally commit stuff I feel like I'm not likely to touch again to reduce some visual noise.
  • hypeatei 1y ago
    git add --patch ...is your friend if you want to leave all your changes unstaged for awhile then break it out into multiple commits later.
    • t-writescode 1y ago
      To add, when I’m breaking my changes down into multiple parts for review, I tend to: * squash everything I’ve done into one commit * create a new branch off main/master that will be the “first commit” * cherry-pick changes (easy from some git guis) that represent a modular change. * push and make an MR from the new branch * rebase “the big commit” on top of the partial change. * wash, rinse and repeat for each change, building each MR off its requisite branch. The squashing part is vital because otherwise you enter merge conflict hell with the rebase.
      • m000 1y ago
        How about: * squash into one commit * git reset HEAD~1 * git add -p * git commit -m commit1 * repeat until no changes are left * add any file deletions/additions I use this because you can have several commits marked e.g. "commit1". Then you make a final interactive rebase to squash them together.
  • t-writescode 1y ago
    This sounds like a real and valid incompatibility with a high frequency commit cadence. If you’re interested in trying these strategies anyway, does your editor of choice have an inline “git blame”? In IntelliJ, I can see who and when committed the most recent change in the line around the one I’m working on. It doesn’t resolve the “which files have I worked on” issue; but it might help the others? Not as nice as a different colored line like uncommitted code would otherwise be highlighted, but it could be enough of a step in that direction?
• herval 1y ago
  I find the sort of opinions on this post quite common on a subset of engineers - namely mid levels with some time in the career, who start to consider themselves senior engineers and want everyone to follow the same set of strict rules they decided make sense. It’s the same mindset that makes people pedantically apply DRY to every situation or forcing others to TDD basic apps. In practice: - smaller PRs aren’t necessarily easier to review (and this arbitrary obsession almost always leads to PR overload in chunks that don’t make any sense, reducing code quality as a result) - nobody reads intermediate commit messages one by one on a PR, period. I worked on a team where the lead was adamant about this and started to write messages in the vein of “if you’re reading this message, I’ll give u $5”. I never paid anyone a dollar. Don’t waste your time writing stuff for no one. - “every commit must compile” - again, unnecessary overzealousness. Every commit on the MAIN branch definitely should compile. Wasting your time with this in a branch, as you work towards a solution, is focusing on the wrong thing You want PRs because they help others absorb what you’re doing (they’ll have to read that same code sooner or later). You don’t want to create a performance theater.
  • leetrout 1y ago
    My simple suggestion to my teams: PRs are emails to your team and to your future self. Framed in that context it's easier to carry the correct tone and think about scoping / what's important. --- > pedantically apply DRY to every situation I swear DRY has done more damage to the software industry from the developer side than it has done good because it has manifested into this big stick with which to bludgeon people without taking context into account.
    • BobbyJo 1y ago
      A great way to frame DRY that I heard from hackernews: "DRY things that are supposed to have the same behavior, not things that happen to have the same behavior"
      • collingreen 1y ago
        This is a really good way to put this. The "just because the do they same thing right now doesn't mean they _do the same thing_" concept is hard to convey!
• Illniyar 1y ago
  I disagree with practically everything suggested. Reducing scope and splitting a single task into multiple PRs each small but part of a bigger picture makes it very hard to see the bigger picture. You should try to make PRs small, but if a PR is big, then you just have to spend more time to review it. Formatting commits as a story is a huge hurdle for the one making the changes. And unless every PR is meticulously prepared - going over the commits by the reviewer is a waste of time. I agree you should return PRs you don't understand though. Or don't feel comfortable reviewing for whatever reason.
• CraigJPerry 1y ago
  Pair programming > PR reviews. I've heard people scoff at the $$ cost of "mob programming". I think that view is totally myopic, for appropriate problems there's just no faster nor higher bandwidth way to transfer code knowledge in a group. Plenty of people dislike pair programming, i don't dislike it but i do find it mentally intense, tiring. I really really enjoy that it's an accelerator for getting to done - not just i wrote the code but the code is correct - sooner. Long way to say don't rely on pull requests when you could be doing pairing for the important stuff.
  • EasyMark 1y ago
    The only time I like pair programming is when I (or someone else) is stuck on an issue and we need to work together and bounce ideas off each other; otherwise I prefer to work on a problem myself rather than trying to be in constant contact with me paired person.
• arnorhs 1y ago
  This idea of every PR being a small chunk that you can review in 5-10 minutes is completely ridiculous. It is reasonable for bug fixes or small improvements, but the review time you should expect for a PR should reflect the size and impact of a feature, not some arbitrary number. Yes, everybody would love it if every PR was small enough. In reality that is not a good way to build substantial features. Often, fully building out a substantial feature, causes you to change your mind and completely changing your approach the further along you are. You don't want to be muddying up the PR pipeline with a bunch of half-assed changes. Doing that just makes reviewers less inclined to give good feedback on a PR, because they "know it's going to change so much anyways". If you are building a substantial feature, it is reasonable that the PR is large and reviewers will have to dedicate substantial time to reviewing it. Reviewing it is work on its own and hopefully your engineers have dedicated time to review substantial features. Of course, you should make sure your substantial feature is as minimal as possible, for whatever is needed to ship the feaure - but not any less than that.
• crazygringo 1y ago
  I can't agree with a lot of this. PR's should generally be the size of a feature, or a meaningful subfeature for large features. When you arbitrarily split up PR's into something "300 lines" or "5-10 minutes" you can miss the forest for the trees. The little thing looks fine in isolation but doesn't make any sense as part of a larger approach. Different people are reviewing it piecemeal but nobody is reviewing the approach as a whole or making sure the parts fit together right. And then the idea of "telling a story with commits" feels like a waste of time to me. I have no interest in the order in which you wrote the code, or what you wrote and then rewrote. The code itself needs to be legible. Your final code with its comments should speak for itself. Code is the what and comments are the why. Now, what I will say is that the more junior the developer, the smaller their commits should be. But that's because they should be assigned smaller features, and have more handholding along the way. And when people are making larger architectural changes, they should be getting signoff on their overall approach from the start -- if you're frequently rejecting the whole approach to a problem in code review, something's going wrong with your communications processes.
  • Too 1y ago
    Yeah. While this narrative style tries to explain what things are done, it instead often leaves the question: Why are we doing this at all? Commit #1 adds a helper function for whatever, looks innocent enough, implementation is correct. Believe it or not, it even has tests, lgtm. Then only by commit #8 do you realize this helper function is not needed at all and the entire approach is wrong. Happens every time. I started reviewing these chains backwards and refuse starting a review until the whole chain is available. That’s however not always easy either, when commit #2-#5 has incrementally refactored everything into something unrecognizable, so that both the left and right side of the diff are wrong! No, I’m not interested in ”this will be fixed 2 commits down the chain”. I just want to review the final state that goes into production, nothing else matters. Yes, commits should be made small whenever possible and not include unrelated fixes or refactors. Just please, keep them meaningful on their own.
  • jordwest 1y ago
    Thank you for saying this. This drove me up the wall everywhere I worked that had those arbitrary ~300 line rules as all that happened was code review devolved into "I don't like this code pattern, use another code pattern". While the larger architectural ideas that actually became problematic were rarely reviewed since they weren't obvious when split across 5 PRs often with different reviewers. Honestly I think it would be far more effective to just review a paragraph and maybe a diagram that explains "here's how I think I'm going to tackle this problem" and forget about line-by-line code review entirely. Other than for training juniors, I don't think there's much long term value in "I think you should use an anonymous function here instead of a named function". The kinds of things that are usually brought up in code review are not what contributes to real long term technical debt, because a function name or code formatting can be changed in an instant, while larger architectural decisions cannot. The other thing I noticed is that even when an architectural issue is obvious, there's a tendency to not want to change it because so much of the work has already been done in preparing the PR for review. If you point out a flaw in an architectural decision, it's not unusual for the person to reasonably say "I've just put together a chain of 5 PRs and now you're asking me to rewrite everything?"
• lo_zamoyski 1y ago
  It's a nice thought, but PRs, when more than performative, are often more about basic sanity checks than anything else. There are two things that can be involved when a change is submitted: a design change and an implementation change. Design changes that are significant enough to warrant review should be expressed and discussed in text, not code, in design documents of some kind. These tend to be quite general, but they establish agreement about general structure, which tends to be the most difficult to change later on, as it circumscribes and guides the implementation that follows. Writing is also a way of working out ideas. The very act of having to explain something clearly to someone else forces you to confront your own ignorance and the consequences of your proposal. Besides, it's the job of the person proposing the design to work out those consequences so that others can verify whether they're true. (1) Reviewing implementation changes with an understanding of design allows the reviewer to understand how changes relate to the whole as well as the aim. This is an insider's perspective. (2) Reviewing implementation changes without a good understanding of the context will be limited to general technical remarks, but can descend into excess attention given to style or inconsequential matters of taste. This is the outsider's view. (3) The question, I think, that looms in the background is whether familiarizing yourself with the context sufficiently well so that you can judge the PR submission is reasonable for a PR. In many cases, it isn't. It's too time consuming and the context is too big. If we had infinite time, this would be great: having to explain to an outsider what you've done forces you to give a much more thorough account, if your goal is to achieve thorough understanding. It also exposes your thinking to someone who doesn't necessarily share your assumptions. But this can be a Herculean task for anything sufficiently complex. So the criticality of the change must be weighed against the effort needed to explain or learn. Are you making a change to something with high tolerance for error, or a small margin of error? Two pieces of advice... Since it is unrealistic to expect an exhaustive verification all the time, focusing more on tests will be more fruitful. You still need context to judge whether they're exhaustive or test the right things, but it's the one place where correctness criteria are expressed in code, apart from type signatures, in a clear enough manner that expectations can be judged. If they aren't clear, you should ask. It's a good locus for discussion. The second: code doesn't include the rationale or "why" for what it is. It just is. Context goes a long way to help infer reason for the change. This means we should use comments, either in the code or the PR submission itself, to explain changes. If something isn't sufficiently clear, ask. But the key is prudential judgement. You have to determine how to limit your verification efforts and when to begin accepting on trust for practical reasons. And do away with your pride. It's only difficult to ask questions if you suffer from pride, and pride is a sure sign of mediocrity. You're also lying through omission.
• chickenzzzzu 1y ago
  Clean coders, PR reviewers, and architecture astronauts deliver astonishingly useless code at incredibly slow rates. Features are everything. Don't give me that bullshit about refactoring and generifying and DRY and TDD.
• anal_reactor 1y ago
  My biggest problem with reviews is that I just don't care. I work for a company that does nothing interesting with a coworker that doesn't like me under a manager that is incompetent. I have zero reasons to do a good job, and all the reasons to do bare minimum not to get fired.
• citizenpaul 1y ago
  I've seen numerous comments over the years on HN where people admit that their process for review is: Yea, this person knows x so blindly approve. or Hey jane/john did you remember to check x before this? Yes! Ok, blindly approve.
• flatline 1y ago
  To pile on to the litany of complaints, I think PRs are a rather poor abstraction. It’s not a good way to review code. If I’m doing an actual review of anything complex, I’ll check out the branch, step through the code, jump around in the IDE: this is how we work with code, not through a fairly unreadable git diff. PRs have no scope and no way to designate scope. What do I want from each reviewer? At some point, the PR becomes superfluous to adequate code review and communication around said review. Ultimately they can be used to micromanage and gatekeep merges to main. This is the PR at its worst. Overall I’m not a big fan, it feels like a necessary evil to meet the demands of Big Agile.
• mercurialsolo 1y ago
  This seems a lot like fragmenting context across the edit and the commit itself. With coding agents in the loop now 1. Why do you want to fragment context? Why not add this to the main file with inline docs?
• tibbar 1y ago
  I would argue that code review of large changes is hard because we don't write good PR descriptions. If you just get 200 files to review in alphabetical order, sure, that's basically an impossible task. Instead, the PR description should have the narrative of what the PR is about, what is the high-level strategy and key decisions, what are the entrypoint(s) to reading the code. What does correctness look like, and what have we done to ensure that? How does this PR fit into the important problems we're working on and what follow-ups or maintenance are implied by it? If you have a good description, then you understand (a) what parts are important to think about and (b) do you agree with the approach and (c) is there anything in the code that doesn't line up with the approach. This strategy scales up pretty well to even large PRs.
• scottgg 1y ago
  As a jujutsu fan, I like to prepare a nice clean commit history for my reviewers to probably ignore. But I kind of assume - there’s _some tool_ we could use - perhaps on top of GitHub - to make stacked reviews not suck, either actually stacked PRs or just incrementally reviewing a nicely divided branch PR. I thought it might be graphite but it seems that needs some whole other tooling _stuff_ on top? Any recommendations ?
  • kiitos 1y ago
    yeah, for sure -- avoid stacking changes in the first place! the purpose of PRs and code review is to establish a single unified shared context between multiple stakeholders. it's the author's responsibility to propose changes that are independent and coherent and easy to understand.
• lijok 1y ago
  The only advice worth following here is: send shit PRs back. If you don’t understand the change, it’s too large, it contains multiple independent changes that should’ve been separate PRs, anything that doesn’t smell right - send it back. My expectation when you review my PR is that your ass is on the line just as much as mine if something goes wrong. PRs aren’t a checkmark exercise that validates you’re not trying to backdoor an exploit into the system. A reviewer that accepts a change is committing themselves to maintain said change going forward. If you let your kids get a dog and the kids don’t take care of it, you will. There’s no two ways about it.
• riyanapatel 1y ago
  This post hits on something crucial - the difference between performative code review and substantive collaboration. The "theatre" metaphor is spot-on. The core issue Saša identifies - PRs that are "unreviewable" getting rubber-stamped with "LGTM". Teams go through the motions without the actual substance. The storytelling approach through commits is brilliant, but it only works if you solve the human factors too. Even perfectly crafted PRs with great commit narratives get surface-level reviews. The friction kills engagement. A few complementary approaches I've seen work: pair reviewing for complex changes that are hard to break down, AI pre-screening for basic issues so humans can focus on architecture/business logic, and synchronous review sessions when async back-and-forth is just burning time. The key insight: good PR structure needs to be paired with removing tooling friction. When review is painful, people default to "LGTM" regardless of how well the story is told.
• dkarl 1y ago
  There's a chicken-and-egg problem with "story-telling commits." Codebases don't contain commits that tell stories, so engineers don't look for them, so engineers don't understand the value of them, so engineers don't learn the skills of rewriting commits, so engineers just squash everything, so codebases don't contain commits that tell stories.
• constantcrying 1y ago
  Great advice, especially I, as a new developer, had to learn. At first it was a mystery why some other dev reviewing my PR wanted me to split a commit into separate parts. It's also not something you learn in university and definitely requires some git knowledge to do properly. But after doing it once or twice i definitely understood, that it was also helping *me and forced me into better version control practices. Definitely a good lesson to learn, but more valuable to me was that the job taught me, that I absolutely despise doing software development.
• jongjong 1y ago
  I really enjoy doing code reviews. I don't mind reading other people's code. If you do it enough, it becomes easier. Also it's good for your own coding style if you read other people's code as it helps you to normalize your style an become more minimalist. I enjoy LLM vibe coding now because I know approximately what code to expect for any given prompt. Basically if the agent doesn't give me code I expect, I usually reject it. It's rare when it comes up with a solution I didn't expect though. I think this is because reading other people's code trains you to be minimalist because it's easier to spot unnecessary complexity when it's from someone else. I think the skill of being able to read code quickly and applying your intuition is going to be increasingly valuable. Nowadays, I don't even need to read the whole code to sense when there are issues. I usually have a 'gut feeling' when the code has problems by glancing over it; though of course, I need to invest some effort to list out specific issues because I can't say "this doesn't feel right" in my code review. But even with this, you can become better. Developers who write a certain way tend to make the same kinds of mistakes.
• haglin 1y ago
  Splitting up a PR is a lot of work, especially if you want each commit to compile, but it would be great if you could categorize changes using 5–10 colors and then have checkboxes that you can toggle to hide code corresponding to a color. What each color means would depend on the PR, but, for instance, yellow = refactoring, brown = test code, blue = drive-by fix, orange = more efficient data structure etc. The colors and their meanings could be set by either the author or the reviewers. It would be similar to the file checkboxes that exist today, but in this case, it would be per concept, not per file.
  • m000 1y ago
    How it started: "It would be great if you could categorize changes using 5–10 colors and then have checkboxes that you can toggle to hide code corresponding to a color." How it's going: https://gitmoji.dev/ https://gitmoji.dev/ IMHO, the novelty wears out fast. Especially when your git history starts looking like a Messages thread.
    • haglin 1y ago
      It would be in the "Files changed" section on GitHub, not in the git history. If you don't want to see the context, you can turn it off.
• bvrmn 1y ago
  If you could make a good "story by commits" I assure you: a combined single commit would be as much readable as multiple. Good story is a result of composable changes highly visible without artificial splitting. Funny thing I never seen a good PR with meaningful multiple commits. At least you need to be quite proactive in history rewrite to make it presentable. Usually it's mere fix/fix/fix looking amazingly bad without squash. If you want to comment on this matter, you are welcome to share your github examples.
• BrenBarn 1y ago
  The idea of insisting on a "clean" commit history always rubs me the wrong way. In my mind a commit is like hitting Ctrl-S. It's just saving a checkpoint in your work. And the commit history is supposed to show what actually happened, not a cleaned-up story about what happened. I can see that it's good to be able to see that cleaned-up story, but I don't think the commit history should be that. There should be something else, a meta-statement about a bunch of commits, that summarizes it. But the commit history is still there if people later need to dig into how something was done. I saw some other comments talking about PRs as this unit, which maybe works, but I've always thought it would make more sense if the VCS had a notion of "commit sets" to which metadata could be attached. Then you could either look at such a set in collapsed form, with a single summary description of the whole set, or you could expand it out to look at the individual commits. In theory these could even be nested, although that might get messy and I think even just two levels would be very useful.
• rdiddly 1y ago
  Jeez, are there really people out there so afraid of looking unsmart that they won't stand up against the demon Complexity? Complexity is an evil whose presence you tolerate, not something you go chasing after like a trophy. Everybody is always trying to introduce more of it, especially noobs, inadvertently. It must be fought. And when you fight something, you don't blame yourself ("I'm too dumb to get this"). You blame the thing, you blame the complexity. "This shit is too complicated." Because guess what, chances are pretty high that if it's so sprawling, so bifurcated, so confusing, that you can't wrap your mind around it, it's because it's a piece of shit. E = mc² is what a finished product looks like. It has 2 variables and describes the whole universe. Whatever, you can argue all you want with me here, I'm being provocative, but the bottom line, regardless, is take yourself and your ego out of it. It's not you. You're not even important. Edit to add: By fighting complexity, you're demanding more and you're right back to looking smart again. So actually your precious ego can still save face.
• selinkocalar 1y ago
  We've definitely fallen into the trap of performative code reviews where everyone feels obligated to find something to comment on Quickly learned the best code reviews are focused on logic and architecture, not formatting. But it's easy to slip into nitpicking because those comments are easier to write