News for hackers. Stuff that matters.
Re:PR Process Is Killing Morale and Productivity by donatj on Friday November 29, 2024 @12:45PM (#42275545)
My team has a bike shedding sort of problem where a 100 loc PR will sometimes get scrutinized to hell, but a 3,000 loc PR will get LGTM'd by enough of the team to be merged before anyone that actually cares gets a chance to look at it. I would say the second half of that is the much bigger problem.

People know who to ask to get a quick lgtm.

I don't know what to do about it. I can't make people actually review. I've had unrealistic dreams like holding people responsible for things they approved that had bugs, but anything like that would be supremely unpopular. I could do something like requiring review from the team members that actually review, but they already feel overwhelmed by being the only ones that actually review.

We tried setting soft limits on the size of PRs, but that comes with a lot of PR that are hard to review because the work is poorly divided and doesn't make sense in isolation.

Re:PR Process Is Killing Morale and Productivity by js8 on Friday November 29, 2024 @12:55PM (#42275631)
The reality is that you can't make people to care more about quality than their (yours) management cares about it.

I would suggest discuss this with manager, and if he cares enough about quality, he can try to slowly coach the individuals to give better reviews. Or he can suggest that multiple experienced/reliable people also review the work.

Re:PR Process Is Killing Morale and Productivity by Kinrany on Friday November 29, 2024 @12:51PM (#42275592)
Anything you do to make people fix their 3000 loc PRs will be unpopular with those people
Re:PR Process Is Killing Morale and Productivity by sundbry on Saturday November 30, 2024 @12:35AM (#42279790)
This, you should make soft and hard limits on PR sizes, providing exceptions only for automated changes or rare situations such as structural changes.
Re:PR Process Is Killing Morale and Productivity by Leherenn on Friday November 29, 2024 @02:11PM (#42276216)
Something helpful I have seen in this situation is to require the reviewer to test the changes for long PRs, and post proofs (a screenshot, logs, ...).

Even with the best of intentions, it's easier to miss things in longer PRs, plus it gives more time for others if they want to have a look. It's not bulletproof of course, but at least it makes sure the basic functionality is working.

Re:PR Process Is Killing Morale and Productivity by rudasn on Saturday November 30, 2024 @12:56PM (#42282972)
Agree. I've made it a habit the past few years to include before/after screenshots in my PRs and sometimes a screen capture of the UI/workflow changes. Also, screenshots in the issues pages for bugs or anything UI related.

I find it makes it easier for people to see and understand the goal of your changes and helps them focus on the how in the code review.

When others do that in their PRs I can jump straight into the tests files and get a better understanding of the what is proposed (or at least start the review process in a more substantial way - get the important stuff addressed early on).

Re:PR Process Is Killing Morale and Productivity by simmschi on Friday November 29, 2024 @12:50PM (#42275582)
Have you tried the code owners feature (assuming you're on Github).

IMO a good approach is to have the actual code owners (i.e. the team responsible for a specific service or library) review the PR. If they think a shallow LGTM review of 3k LOC is enough, they can also deal with the bugs :-)

If you don't have specific ownership in your code base I'd start there.

Re:PR Process Is Killing Morale and Productivity by donatj on Friday November 29, 2024 @12:57PM (#42275653)
This is all within a relatively small single team.

As I said however, I could require review from specific people I know review but they're already at their wits end.

Also having to explain why certain devs are required without it smelling like some sort of favoritism seems fraught.

Re:PR Process Is Killing Morale and Productivity by Aeolun on Saturday November 30, 2024 @03:12PM (#42283734)
Normally you promote those people to ‘senior’ and then say that a review by at least one senior is required?
Re:PR Process Is Killing Morale and Productivity by lijok on Friday November 29, 2024 @01:47PM (#42276032)
Speaks volumes to the maturity of our work that a 3000 LOC PR is seen not only as acceptable, but expected.

Raising a planning change equivalent to a 3000 LOC PR in a civil engineering firm would get your assignment swiftly handed over to someone more competent.

Re:PR Process Is Killing Morale and Productivity by loa_in_ on Friday November 29, 2024 @04:55PM (#42277454)
LOC is a bad measure of literally anything. It's not related to productivity, efficiency, bug count, readability, etc. What it's most related to is the choice of language and libraries. For me, shunning big contributions just for being big just stifles my drive to contribute. If the solution does what it's supposed to, is valid, efficient and there's no obvious unnecessary duplication of codethen who cares if it's big. All valid contributions push the project forward.
Re:PR Process Is Killing Morale and Productivity by sundbry on Saturday November 30, 2024 @12:37AM (#42279803)
It is actually directly related to the time and complexity of code review. Just because LOC is an imperfect metric for productivity etc., does not mean it's a poor metric for risk, complexity, poor sequencing of changes, or other attributes.
Re:PR Process Is Killing Morale and Productivity by DontchaKnowit on Saturday November 30, 2024 @11:20AM (#42282342)
How are you supposed to write new code without hitting a 3k loc pr?
Re:PR Process Is Killing Morale and Productivity by Aeolun on Saturday November 30, 2024 @03:13PM (#42283741)
Merge before you’ve written 3000 lines of code?
Re:PR Process Is Killing Morale and Productivity by Olreich on Monday December 02, 2024 @11:22PM (#42303046)
But the planning change to add a new wing to the building? That can’t be made smaller. What about if a new regulation is put in effect requiring every fire door to have a corresponding fire window next to it? That’s either 3000 little changes that need reviewing or one big change. One big change is often the correct choice. Knowing when it’s appropriate and when it’s not is the job of the engineer, the team, and the lead.
Re:PR Process Is Killing Morale and Productivity by Aeolun on Saturday November 30, 2024 @03:11PM (#42283725)
I like how when you ask people why they approved something obviously broken, they’ll say something like “Oh, I didn’t really look at it.”

I mean, I do that too once in a while, but if bugs go through you own that.

Re:PR Process Is Killing Morale and Productivity by camgunz on Sunday December 01, 2024 @06:12AM (#42287748)
The answer is to require a high percentage of test coverage. Then you skim through the code to get a general idea of what it does and how, then scrutinize the tests to verify they enforce that understanding.

This does three things. First, it prevents people from dropping big chunks of code and asking their reviewers to make it work (if coverage is too low, it doesn't work). Second, the tests spare you the cognitive work of verifying the code does it's supposed to--especially edge and failure cases. Third, this frees you up to think about the bigger picture. Is this the right way to do this, should we be doing it at all, does this conflict with something else in the works, etc