11–20 of 127 posts
I wish. My small PRs die by a thousand bikesheds. My large refactors are followed by LGTMYOLO
Same rat brain muscle that made me buy an extra 200,000 dollars of stuff I didnt really need on my home, but I spent 6 days at Home Depot fighting over which color of mustard to paint the guest bathroom
> 50-line code changes are reviewed and merged ~40% faster than 250-line changes. Reviewing a 50-line code change in 60% of the time it takes to review a 250-line code change means the shorter code review takes _four times_ as long to review per line.
Unless the metric is actually 40% faster per LOC but that's not specified.
1. The "unit of work" is small enough to decompose into a 50 line PR. This is possible on larger codebases and less verbose languages. It can enforce over-DRY code which often leads to a lower universal ability to understand what is going on.
2. The team responsible for reviewing is responsive. I have never worked at a company where PR reviews happened quickly. Everyone is busy, everyone is overworked, and often times PRs are a chore that can be put off until later. Of course you can make some labyrinthine chain of small PRs all merging into one big PR but the problem still exists. I'd personally rather review a 300 line complete PR written well than 6 PRs that I have to constantly recall context on.
This advice follows the same logic as the asinine cyclomatic complexity rules. Yes, in theory they are sound. In practice, it's more of a flexible benchmark rather than a hard rule. But since someone inevitably writes YASL (yet another stupid linter) that enforces these types of things, an enterprising work-focused engineer will end up spending either more time fixing code to please the linter or more time exploiting loopholes in the rules.
Just write Good Code (TM) - whatever that looks like.
> 50-line code changes are reviewed and merged ~40% faster than 250-line changes. Reviewing a 50-line code change in 60% of the time it takes to review a 250-line code change means the shorter code review takes _four times_ as long to review per line.
> 50-line code changes are reviewed and merged ~40% faster than 250-line changes. Reviewing a 50-line code change in 60% of the time it takes to review a 250-line code change means the shorter code review takes _four times_ as long to review per line.
You could argue that shorter PRs are easier to review and therefore people are doing more review, and that should result in better changes (indeed the 15% fewer reverts supports this).
Or you could argue that shorter PRs are harder to review as they have less context, and therefore more time is taken but that a better output is not necessarily being reached.
I think both of these are entirely plausible, and the truth probably depends on other process factors.
Those small PRs might not be reverted, but there are probably a fair number of 50 line PR follow ups that end up modifying the original implementation, and using up more total time and effort than if it was just a larger PR implementing the feature in its entirety.
I wonder how many of the large PRs they say take longer to merge and get less comments are actually feature branches into which the smaller PRs are merged, and which we expect to hang open for long periods of time with little interaction.
This analysis seems to take a set of data without much investigation into what the data actually represents and makes an immense effort to find some conclusions about the context-less data.
If CI takes 15 minutes and I create PRs one by one, I’ll need to wait a total of 10 hours just for CI, which is impossible, I’ll need to use stacked PRs. If I’m using stacked PRs, that means I do all the work up front, and then try to painstakingly split up my final code into little 50 line chunks that build incrementally. I’m sure someone will say “oh just write incremental small commits to begin with”, but that never seems viable to me. Code and design evolve as I build in ways that would make the review of the first 50 lines I commit on the project pointless since none were retained in the final product. So not only do I have to build this feature, and write 40 PR descriptions, and write an overall meta-PR-description for the change in aggregate, I also need to cut my finished code up into puzzle pieces for reviewers to mentally re-assemble.
Anyways, maybe for maintaining mostly-done features 50 lines is nice, but I’d much rather a new feature or larger change be a handful (1-3) of larger (500-2000 line) PRs possibly accompanied by pair-review/walkthrough discussion.
It cheers me up that people so naive exist.
Size is contextual to the thing the PR regards. Maybe it's one char. Maybe it's 500 lines. Maybe it's 10k lines. Screw this.
I’m reading this article on a break from reviewing a PR with more than 300 lines changed over 20+ files and I wonder if some people simply live in some sort of alternative wonderland universe. The “ideals” are almost never a thing I see in my daily job, and I’ve worked for countless companies as a contractor.
For me, I feel like the difficulty score of performing an effective review is slightly greater than nlogn but less than n*root(n).
300 is a bit beyond my favorite limit of 200.
Showing some context per commit makes all the difference when I decide to do a first commit which fixes all formatting in the file that I’m visiting (for the real/substantive change).