Hacker News Reader

Comment by donatj | Hacker News Reader

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.

View on HN

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.

Anything you do to make people fix their 3000 loc PRs will be unpopular with those people

This, you should make soft and hard limits on PR sizes, providing exceptions only for automated changes or rare situations such as structural changes.

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.

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.

Normally you promote those people to ‘senior’ and then say that a review by at least one senior is required?

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

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.

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.

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.

How are you supposed to write new code without hitting a 3k loc pr?

Merge before you’ve written 3000 lines of code?

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.

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.

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).

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.

Reply on news.ycombinator.com