9 ms·

I know this is not point of the article but: > The PR was bigger than what I felt I could sensibly review and, in honesty, my desire to go through the hours of

• MentallyRetired 3y ago
  It's certainly not hours of work to review it... or maybe it is since it's financial? Either way, "it was in the script" as my wife and I say about corny movie moments. It made for a good article.
  • robertlagrant 3y ago
    Critical Drinker has a phrase for nonsensical things in movies: "X occurred... so the movie can happen".
• golergka 3y ago
  That's literally 300 something lines. I'm buffled, 5k PRs aren't that rare at work.
  • plugin-baby 3y ago
    > at work Level of trust with colleagues will hopefully be higher! And individual ownership of and responsibility for the code probably lower.
    • g8oz 3y ago
      Which is the philosophy behind Gerrit as opposed to Github.
  • nateberkopec 3y ago
    What language/stack?
    • golergka 3y ago
      Typescript, React, Monaco, Treesitter, etc.
  • zuprau 3y ago
    I’d hate to work there. I’d rather review small chunks and merge often than review 5k lines that could be built on a shaky foundation/idea
    • golergka 3y ago
      Best team I've ever worked with. 5k was on the larger side, sure, but technically difficult tasks often just can't be split into a series of smaller pull requests.
      • tsimionescu 3y ago
        I am always amazed that many people with significant experience are so resistant to this idea. For what it's worth, my experience matches yours entirely: many significant changes can't be meaningfully committed in small chunks, they only make sense as an all in one. And even more so, I've often seen the opposite problem: people committing small chunks of a big feature that individually look good, but end up being a huge mess when the whole feature is available. I hate seeing PRs that add a field or method here and there (backwards compatible!) without actually using them for now, only to later find out that they've dispersed state for what should have been one operation over 5 different objects or something.
        • charcircuit 3y ago
          >many significant changes can't be meaningfully committed in small chunks They almost always can. The exceptions are stuff like autogenerated code or updating a dependency.
        • sh4rks 3y ago
          I think if a review is very large, the owner of the code should do a code walkthrough for the whole team.
        • ZephyrBlu 3y ago
          What changes can't be committed in <5k LOC? That's a shit ton of code. If you can't break that down into smaller shippable chunks there's probably something wrong, or you're building something extraordinarily complex. It's definitely overall quicker to ship like this, but there are tradeoffs. You are effectively working independently from the rest of your team, there is no context sharing and everything is delivered at once after a longer period of time.
  • TeMPOraL 3y ago
    What is being changed matters. To use an analogy: if you wanted to reupholster my car seats, or spice up the radio panel, sure knock yourself out - I'll come check when it's done. But if you were to as much as think about tightening or loosening a single screw anywhere near the engine block, believe me I will be paying very close attention to what you're changing.
    • robertlagrant 3y ago
      That would be a reason to never accept the PR; not to auto-accept it.
  • bjornasm 3y ago
    Well, I feel like "at work" is a keyword here.
  • globular-toast 3y ago
    As if you are reviewing 5k lines, though. It's either 90% whitespace changes that you completely skim over (probably missing the one bit that actually changed) or you just skim over it looking for a few patterns you don't like such as using a loop instead of Array.map or something.
    • smabie 3y ago
      This is so true.
    • sh4rks 3y ago
      When they say 5k lines, I'm assuming they mean 5k lines of meaningful code. 5k lines with whitespace changes, linting etc. would be impossible to meaningfully review
  • ZephyrBlu 3y ago
    You genuinely review 5k LOC PRs? If I was doing a proper review of a PR that big and making sure you understand how everything works that would easily take multiple days and generate 10s-100s of comments. In reality I would flat out refuse to review it. Even 1k is very large without being broken down. The only time I see PRs that big at work are cleanups (E.g. deleting whole directories), automated linting changes across the whole codebase and large structural refactors (E.g. changing directory structure).
    • opportune 3y ago
      Almost every time I’ve reviewed a 1k+ LOC PR, even if it’s from a really experienced and good engineer, it has introduced a bug. Obviously I’m not gonna say it at work, but changes that big are too hard to properly review and consider all side effects and gotchas
      • tstrimple 3y ago
        It's also more difficult to bisect to find the actual bug. Small commits have a lot of advantages.
• femiagbabiaka 3y ago
  For this PR in particular, seems like a lot of it is formatting changes, so you’re right, may not have been a big deal in practice. But I wouldn’t take the statement so literally. For an open source project, every PR can contain unbounded toil for no pay.
• rtpg 3y ago
  So with libraries that are consumed by third parties, it's rarely about the number of lines of code. Since you don't really have a definitive list of all usages of your code, things like the changes to `_money_from_dict` making things nullable mean you have to consider all ways in which that might blow up. And like "the public API is the public API, the private API everything goes" is easy to say, but it's so easy to just break other projects with these kinds of changes. This makes it very hard to move forward with certain kinds of "bug fixes" in projects. That being said, it's not that that PR is "hard", but it's hard to say "merging this is A-OK" instantly. Hours? I dunno, but I would definitely add some tests and try to figure out how to write code that breaks with those changes. If I were managing that project at the time, and had motivation, I'd definitely do a lot of cherrypicking, get all the "obviously won't break anything" changes merged in, to leave the problematic bits in for a focused review. Again, this all might be under an hour, but sometimes you look at a thing and are like "I don't really want to deal with this, I have my own life to deal with." At a higher level, the biggest problem with these kinds of libraries is having the single person who can merge things in, who can hit the "release" button. There's a lot of projects that interact with Django that have heavy usage, and survive mainly thanks to random people making forks and adding patches that properly implement support for newer Django. At $OLD_JOB we used a fork of django-money (including a lot of patches to fix stuff like "USD could be ordered with JPY", pure bug generators). It was very easy to add patches because, well, we had our usage and our test suite and no external users. It's great, but it's also important to try and get patches upstreamed when possible (and we did for a lot of projects).
  • trevyn 3y ago
    >So with libraries that are consumed by third parties, it's rarely about the number of lines of code. Since you don't really have a definitive list of all usages of your code, things like the changes to `_money_from_dict` making things nullable mean you have to consider all ways in which that might blow up. And like "the public API is the public API, the private API everything goes" is easy to say, but it's so easy to just break other projects with these kinds of changes. Strongly typed languages, a well-designed API, and senantic versioning can make this problem largely disappear.
• bruce511 3y ago
  Depends on the bits they don't do. I get contributions from time to time. Yet can look small , but I need to add tests, documentation, and so on. It can easily take an hour for just one method added, or whatever. If the code is just fire and forget, then fine. If it's part of a bigger system with rigorous standards then 300 lines can take a day or more to "merge" in.
• eyelidlessness 3y ago
  After a brief scan I’d call the full change reviewable enough I could do it in a sitting. Most of it looks reviewable on my phone. But seeing >30 commits, I’d pause. Partly because I’ve become a lot more sensitive to the impact of commit history itself, partly because the quick scan of such a small change set doesn’t seem to line up with so many commits, but mostly because it implies much more context exists than the attention I’d pay if it came pre-squashed. That kind of implication stops me in my tracks to learn more. I’ve spent literal days tracking down the meaning of single line code changes through multiple dozens of commits, sometimes across repo boundaries (ahem the original author’s suggestion of deprecating in favor of a fork comes to mind). The size of this particular PR only becomes a factor when any one of those numerous commits can become that rabbit hole. How many humans’ days are going to be spent tracing history through this particular merge? For how many different reasons? I didn’t even look at the changes midway, but how many nuances are buried in there and lost unless this weird bundle of changes is preserved?
  • hoten 3y ago
    I bet you're other thinking in this case. In general, the expectation on GitHub is that PR commit history doesn't matter, and owners should simply squash on acceptance. I think most contributors don't event consider that all their commits are visible or would be of interest and only think about the final product. It's certainly simpler for the contributor to do the squashing, but when GitHub makes it so simple in practice it doesn't matter.
    • akx 3y ago
      I disagree pretty hard with this – for instance I've recently needed to dig into the code for the Gradio library, and when PRs are like https://github.com/gradio-app/gradio/pull/3300 https://github.com/gradio-app/gradio/pull/3300 (and the merge commit's message is what it is) it's hard to understand why some decisions have been made when doing `git annotate` later on.
      • hardware2win 3y ago
        I disagree, commits are mehh Pull requests, patch notes, documentation and comments should be source of truth Git is not project management tool, it just manages my letters history.
• edem 3y ago
  This is not at all a huge PR. Sometimes I make thousand line changes (or way over that), although in most cases it is just me working on a project. Changing a couple files (like in this PR) should be OK.
• theden 3y ago
  It's not too bad, but in the context of reviewing for a project in your spare time, it can be draining. I totally understand why he didn't feel like doing it.
• bjornasm 3y ago
  >This doesn't look like a particularly big PR to me, judging solely by the amount of code changed and the nature of the changes at first glance. Well the author specified that this was their subjective take on it.
• theshrike79 3y ago
  I've committed bigger just with a "minor changes" -message :D But seriously: that code seems to be touching bits that really should have automated tests attached. If the tests pass, then I would feel more comfortable accepting the PR.
• akx 3y ago
  The PR isn't very large, diff-wise, but IMO it isn't well made, which makes it hard to review. Half of the commits are merges from other fork branches into the contributor's master, and the PR name and description doesn't mirror that in the least. Then (eyeballing) 90% of the diff is whitespace changes, which would be fine in its own PR ("Formatting changes") because it's easy to eyeball that it's just that, but when you mix it with other changes, it's hard again.
  • seedie 3y ago
    FWIW, whitespace changes can easily be filtered out in Github's PR view.
    • mindB 3y ago
      I don't think this was the case in 2016 though.
      • wes-k 3y ago
        They added it to the UI in 2018 but you could do it via URL since 2011. > A diff view with reduced white space has been available since 2011 by adding ?w=1 to the URL. https://github.blog/2018-05-01-ignore-white-space-in-code-review/ https://github.blog/2018-05-01-ignore-white-space-in-code-re...
        • seba_dos1 3y ago
          Also, Git had it since forever, and since GitHub's diff view isn't particularly convenient for browsing multi-commit PRs you usually review the changes using Git anyway. That said, I'd ask the contributor to tidy up the branch first. It's kinda disrespecting to ask others to review branches in such state.
    • akx 3y ago
      Sure. The link above is in "hide whitespace" mode (`?w=1`), and there are still visible whitespace changes (new or removed whitespace lines).
• quickthrower2 3y ago
  Life is gonna life
• deleted 3y ago
  [deleted]
• jakewins 3y ago
  When I wrote this, almost all my engineering experience was in OSS database development. That environment has several forces pushing towards very detailed reviews and clean commit histories, like others have hinted at in the thread here. The PR review is in public and heavily scrutinized by paying customers and passionate community members. APIs cannot be broken, and even with automated tooling it's very easy to accidentally introduce a change that breaks tens of thousands of deployments. And the code itself is really sensitive. If a bug gets in and released, it can be several days of grind to get a patch out, and after that many months of new tickets for the bug from customers that won't move to the latest patches. Now I work somewhere where the code I write runs in-house. If a bug sneaks in, it's usually a 5-minute redeploy to resolve and the cost is borne primarily by my own team. So I think the answer to your question is: it really depends on the environment you're writing code in. In some setups the cost of introducing mistakes is very high, so it makes sense to pay a lot at the review stage; in others the correct balance is less strict review and fast fixes/rollbacks instead.
  • bbbobbb 3y ago
    That makes sense, thanks for the clarification.
• kunley 3y ago
  Do others share the sentiment that your reply is quite, sad to say that, arrogant? What are you exactly trying to achieve by comparing the guy's "I'm busy, this is long" with yours or anybody elses? Moreover, what on earth has his job to do with the topic? Bad day...?
• xeromal 3y ago
  It isn't that big at all, but when you maintain something you really don't want to maintain, you're over anything but the tiniest changes such as updating the copy. Ask me how I know. lol
• hluska 3y ago
  According to the article, the author no longer used this project. It was no longer front of mind. There is a massive difference between reviewing a PR when you are actively involved in a project and reviewing one when the project is in your past. In that case, it’s perfectly reasonable to spend a couple of hours getting back into code you wrote a long time ago. If anything, taking that time is a big win for overall project stability.
• paulddraper 3y ago
  One issue that makes it more inconvenient is there's no CI set up for this. Some typo could break everything.