So if we fix a > that should have been >= we should add 49 lines of comments explaining this? Or perhaps the PR is doomed to never be ideal due to being short.
Claim: the ideal PR is 50 lines long
21–30 of 127 posts
Re: Claim: the ideal PR is 50 lines long
#22OP needs to discover the html tag or define their acronyms imo.
i thought they were talking about press releases. sheesh
Re: Claim: the ideal PR is 50 lines long
#23Just my comments are 50 lines, but thanks I'll never stop finding these "functions must be this long and classes must have that many properties, and PR should be so many lines" etc. rules hilarious. 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 just think the people who whole-heartedly believe in such "best practices" (and much more so the people who succumb to them) help justify my salary.
Re: Claim: the ideal PR is 50 lines long
#24I have found that teams who allow larger PR sizes without complaining about it can get a lot done a lot faster. Which is to say having 50 line PRs are not ubiquitously ideal, but are somewhere on the spectrum of acceptable but come with definitive tradeoffs. Reviewing and merging 10x60 line PRs is, in my experience, more time intensive than reviewing one 600 line PR. Most of the tests for and new functionality alone…
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 tests? Merging the code first and later the tests? Merging the broken, codeless tests first and then the code?
I find that test code is rarely reviewed properly. If you want it reviewed properly, then create MR's that are just tests. You can create an MR with the change and happy path code, and then subsequent MR's with each of the rest of the tests.
If you're fine with tests not getting the same level of attention (and that's likely OK), then tests don't count as part of the 50 lines, IMO.
Re: Claim: the ideal PR is 50 lines long
#25If someone sends me an multi-thousand line CR, I'll review it and give them impactful feedback. I may miss some details, but I guarantee I'll miss close to all of the details if that is split over 40 CRs. I wouldn't want to work with someone who has time to review like that unless it's a very niche domain.
Re: Claim: the ideal PR is 50 lines long
#26Forget about refactoring then
Big massive refactoring PR's have killed many projects. Sometimes you can't, but if you can split a big refactoring PR into multiple commits it usually works better. The first PR introduces a feature flag that lets you pick between old and new code.
Re: Claim: the ideal PR is 50 lines long
#27I have found that teams who allow larger PR sizes without complaining about it can get a lot done a lot faster. Which is to say having 50 line PRs are not ubiquitously ideal, but are somewhere on the spectrum of acceptable but come with definitive tradeoffs. Reviewing and merging 10x60 line PRs is, in my experience, more time intensive than reviewing one 600 line PR. Most of the tests for and new functionality alone…
"I have found that teams who allow larger PR sizes without complaining about it can get a lot done a lot faster".
Sure.
How many times is the code touched after the PR has been merged is a better question.
Faster is not better. Better is not faster.
Re: Claim: the ideal PR is 50 lines long
#28The response to seeing 10k lines of code in a PR is not to skim it, that is harming one's company. The response is to request the author to justify its size, and/or reject the PR and request that it be delivered in smaller sizes. If the author cannot, then the author needs their responsibilities reduced and to receive additional mentorship.
Skimming a PR is pure cognitive dissonance, dont do this and complain when your app turns into a dumpster fire.
Re: Claim: the ideal PR is 50 lines long
#29Just my comments are 50 lines, but thanks I'll never stop finding these "functions must be this long and classes must have that many properties, and PR should be so many lines" etc. rules hilarious. 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.
Seems like a skill issue, honestly. I feel like many of these arbitrary rules end up being made to account for low skill developers to curb their behaviors which seems like the wrong way to tackle that problem.
Re: Claim: the ideal PR is 50 lines long
#30I think this might just highlight how simple changes are indeed simpler instead of showing that slicing one big PR into severall smaller ones is actually good.