I would kill myself if I worked somewhere where they actually did this type of analysis. I cannot wait for AI to replace me and I become homeless but free from a horrible choice to get involved with computers many years ago. Damning me to these type of just asinine bullshit conversations and posts. I wish I had made different choices.
Claim: the ideal PR is 50 lines long
121–127 of 127 posts
Re: Claim: the ideal PR is 50 lines long
#122sounds like the ideal PR includes no testing
Had the same thought reading this, maybe it’s because I work in OSS and write tests with changes and fix tests for fixes that weren’t caught. That adds 20-40 lines very fast alone. Guessing that is not common.
Re: Claim: the ideal PR is 50 lines long
#123We merged a PR a few days ago that was 40,000 lines added and about 13k lines removed. Tbf, there was quite a bit of config and generated code in there, but the actual changes were at least half of that.
It was tested in smaller chunks in a feature branch, then the larger feature was merged. There was some testing done on the huge feature branch, but not everything that was present in the smaller PRs.
In our case, this new "feature" was a complete overhaul of our business logic engine. Over time, we had essentially developed 3 or 4 different API versions for various business tasks, with a lot of logic being duplicated between them. We want to bring it all under one umbrella, and after about 8 years, we think that we have a good idea of what sort of abstraction can work for the next 8 years.
Now that the engine is merged, we need to start the grueling task of actually moving logic from the old APIs into it and removing them from our codebase. We spent about 6 months (between 2 devs) writing the engine and I suspect it will take 2 years to move all of the logic into the new engine.
Idk where I was going with this comment anymore, but needless to say, I don't often see 50 line PRs at work!
Re: Claim: the ideal PR is 50 lines long
#124I'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…
I too am very surprised by what I'm reading. For contrast, here's a recent thread of people explaining how beneficial it is to work in small commits: https://news.ycombinator.com/item?id=39187487
Re: Claim: the ideal PR is 50 lines long
#125Earlier quoted context omitted.
> and the article has data How does that data prove anything? The ideal PR size is 50 lines because that’s the median size on Github. Seem like a pretty worthless claim.
> The ideal PR size is 50 lines because that’s the median size on Github. ??? No, that's not why it's arguing that it's the ideal size. Where did you get that idea? (TBH I doubt it's the case - apparently the average PR size on GH is nearly 1000 lines [0]) It's arguing that it's the ideal size because (1) PRs of that size end up getting reviewed and merged the fastest; (2) PRs of that size end up getting reverted the…
> (1) (2) (3)
Which IMO is still not worth much or hardly anything at all if we ignore the content of those PRs. I would assume that most are bugfixes or trivial changes (since no tests are needed) so it makes sense that they are causing the least issues.
That’s hardly relevant if you are trying to introduce actual features which require hundreds if not thousands of new lines.
Re: Claim: the ideal PR is 50 lines long
#126Earlier quoted context omitted.
Lines with actual code changes aren't a problem for me, it's the automated IDE indent/spacing/bracketing that really drives me up a wall and fatigues the hell out of me. But this might only be a problem with those of us working on legacy codebases. The kind of PRs I see in OSS projects I could review 1000 lines at a time - it's so clean!
Split the reformatting and real change. Make the reformatting change first and have a policy of merging those change fast.
Some teammates agreed to do this, but it's not known what files will be edited ahead of time, so a large job often has formatting edits anyway.
Re: Claim: the ideal PR is 50 lines long
#127Earlier quoted context omitted.
Split the reformatting and real change. Make the reformatting change first and have a policy of merging those change fast.
Some teammates agreed to do this, but it's not known what files will be edited ahead of time, so a large job often has formatting edits anyway.
Yeah, it's really something that you have to get buy-in for.
You could always make a commit that just has the reformatting when you do start editing a file. If it turns out you didn't need to edit that file after all, then you can revert just that commit easily. And if you did, you can collect up all the reformatting commits into a PR as a first step.
The flip side of this is making minimal surgical small changes, and then doing the refactoring after the change. The key message though is split them.
Refactoring doesn't change behavior, so refactoring efforts should be judged only on "tests exist that cover the functionality, those tests continue to pass, the code is easier to maintain". It's when you add "changed functionality, so we changed the test" to review at the same time, that this becomes a hassle.