Correlation is not causation. What if changes that are inherently more complex — which take longer to review, are more likely to have bugs, etc — typically require 250 lines or more?
Claim: the ideal PR is 50 lines long
91–100 of 127 posts
Re: Claim: the ideal PR is 50 lines long
#92I'm probably in the minority here, but personally I'd much rather review a 300 line PR instead of 6 50-line ones if the change is a single context. I briefly worked with a hard line-count-limit for PRs and I thought it made everything much worse. It is fine for changes that are actually small, but when you need to go back and re-open 4, 5 merged PRs in different tabs to get the full context again, the time to review…
Line count (or count of any units in the PR) is completely meaningless. I feel that in the last decade somehow there has been a loss of context of what is the purpose of version control. Why do we bother with version control? Sure, there are many reasons. But a primary reason is that if we discover behavior X turns out to cause regressions (or is otherwise a bad idea), we can easily revert it by reverting its commit.…
Sure, and at the same time a PR can contain multiple commits. You don't have to squash and merge.
Re: Claim: the ideal PR is 50 lines long
#93I'm seeing some pretty surprising claims in this thread about, "I prefer reviewing larger PRs," and, "Teams that make larger PRs go more quickly." That all seems like rubbish to me. I've seen data indicating smaller PRs having lower error rates and such and then we have this article with data recommending a 50 line PR size. I've *never* seen data show large PRs are better except for some developers saying, "I prefer…
Yeah. Last team I joined I started doing reviews and immediately I was dealing with, like, 500 line PRs. They’d do things like add three completely new API endpoints (one of which is broken), or set up an entirely new service with all sorts of associated infrastructure.
At first I faced a lot of pushback from team members when I wanted them to break up the commits. So I caved. I just tried to give the feedback that I could give, and approved the PRs when they were “good enough, I guess”. They kept breaking in production and it would take a week to fix them. It was hard to tell what, in the commits, was broken. They were just so large.
And it’s not like these PRs were getting written quickly. People would spend two weeks or four weeks working on one PR, because they wanted to make one massive change that did everything and closed out a ticket.
At this point, I’ve made inroads and the team is sending out much smaller PRs, much more frequently. The PRs are getting reviewed, merged, and tested within an hour, and one developer can submit three or four PRs if they’re being productive that day.
I think the problem is that the cognitive load of making large changes is superlinear. A 2x larger change is not 2x as hard, but maybe 2.5x or 3x as hard. With large PRs, you spend way too much time just trying to understand what you are doing, as an author, or what you are looking at, as a reviewer.
Re: Claim: the ideal PR is 50 lines long
#94The correct PR is the one that solves the problem the best. The correct PR's size has nothing to do with the # of lines it is. Tracking time to merge is not that meaningful because merging code does not in itself say anything about the value of that code in production. That is the type of metric that may become a target for a scrum master or project manager despite it being fairly divorced from the reality of moving the right code into the production environment.
Re: Claim: the ideal PR is 50 lines long
#95It's times like these when i question the quality of HN or how can such shit get so many upvotes?
> We flew down weekly to meet with IBM, but they thought the way to measure software was the amount of code we wrote, when really the better the software, the fewer lines of code.” — Bill Gates
Re: Claim: the ideal PR is 50 lines long
#96Earlier quoted context omitted.
This must be a typo - 50 or 500 lines maybe? ...or satire?
I wish it had been satire. This was fundamentally one of the worst parts of my coding career, and it was disappointing because the company had been amazing to work for prior to this individual's arrival. Post-arrival, I was ordered to rewrite a working, stable PHP web app into Go ("Google doesn't use PHP, because it doesn't scale"). We weren't allowed to write RESTful APIs any more, everything had to use Protobuf. An…
I should probably also point out that the "improved" architecture turned out to be corrupting customer data for more than 6 months... which led to termination of that individual.
I'd already left by that point, after a heated conversation in which I was told "you're not half as good a software engineer as you think you are". Well, duh.
Amusingly, the PHP app is still running, still working.
Re: Claim: the ideal PR is 50 lines long
#97Earlier quoted context omitted.
But why does it take longer for 10x60 line PR's? If it's because those PR's are getting reviewed more thoroughly, that's likely a great use of time. If it's because the small PR's are getting bikeshedded, it's not. If it's because of tooling, then the OP at graphite.dev has something to sell you. I'd be buying but we're a GitLab shop. > Indeed we have 2-3x more test code than actual code. So should we be writing less…
It's because every time I switch from coding to reviewing, there's a context switch, and every time I switch back from reviewing to coding, there's another context switch. I can easily spend 30 minutes getting back into the flow of coding after doing something else. One PR has 1 context switch. 10 PRs have 10. To say nothing of the cascading effects of CR on early PRs. Oh, you want me to rename a variable that I used…
Yeah—on the other hand, I’ve gotten PRs that are just too damn big and that means that I can’t review them without sitting down and getting into the flow just for that review. What I end up doing is, after some false starts, getting myself a block of time to review the massive PR.
Let’s say I just finished up some change I’m working on, and I take a look at my coworker’s PR. It’s 11:45, and I’m heading to lunch in 15 minutes. If it’s a 10-150 line PR, I can probably review it within 15 minutes, and maybe I can review six or eight reviews like that in a day, when I stand up to get coffee or something like that.
If I see some 700-line PR coming through, I have to allocate some focus time to that, just like I were programming. It’s a burden.
Re: Claim: the ideal PR is 50 lines long
#98I'm probably in the minority here, but personally I'd much rather review a 300 line PR instead of 6 50-line ones if the change is a single context. I briefly worked with a hard line-count-limit for PRs and I thought it made everything much worse. It is fine for changes that are actually small, but when you need to go back and re-open 4, 5 merged PRs in different tabs to get the full context again, the time to review…
> but when you need to go back and re-open 4, 5 merged PRs in different tabs to get the full context again, the time to review increases tenfold with tiny PRs You can't really say much about the first 4 PRs because if you don't know how that code is going to be used, it's kind of hard to giving meaningful feedback. By PR 5, it's too late to give feedback on PRs 1-4. I also have worked with people that prefer that but…
A skill that you see in some of the more experienced devs is the ability to front-load the most important stuff in a series of changes, or make changes that can at least be understood in isolation.
It’s not always reasonable to do that, but you can often deliver incremental changes where each incremental change either brings a whole feature visibly closer to completion, or provides some self-contained utility, or makes some refactoring changes with the promise of “this will make it possible to do X, later”.
Re: Claim: the ideal PR is 50 lines long
#99I'm probably in the minority here, but personally I'd much rather review a 300 line PR instead of 6 50-line ones if the change is a single context. I briefly worked with a hard line-count-limit for PRs and I thought it made everything much worse. It is fine for changes that are actually small, but when you need to go back and re-open 4, 5 merged PRs in different tabs to get the full context again, the time to review…
Line count (or count of any units in the PR) is completely meaningless. I feel that in the last decade somehow there has been a loss of context of what is the purpose of version control. Why do we bother with version control? Sure, there are many reasons. But a primary reason is that if we discover behavior X turns out to cause regressions (or is otherwise a bad idea), we can easily revert it by reverting its commit.…
100% agree, if you have multiple commits to complete the change during development, just squash them into one for the PR.
That being said many companies (including some FAANG) use commit counts as a performance metric, thereby directly disincentivizing clean, readable commits.
Re: Claim: the ideal PR is 50 lines long
#100Earlier quoted context omitted.
Line count (or count of any units in the PR) is completely meaningless. I feel that in the last decade somehow there has been a loss of context of what is the purpose of version control. Why do we bother with version control? Sure, there are many reasons. But a primary reason is that if we discover behavior X turns out to cause regressions (or is otherwise a bad idea), we can easily revert it by reverting its commit.…
Sure, and at the same time a PR can contain multiple commits. You don't have to squash and merge.
I've rarely seen cases where multiple commits per PR are a good thing. When that happens, it's (usually) that either the commits can be squashed into one logical/consistent commit, or there are too many changes for one PR.
The exception being that many ci/cd setups will squash your commits for you when merging a PR.