Live data from Hacker News

The Theatre of Pull Requests and Code Review

meks.quest

21–30 of 431 posts

Re: The Theatre of Pull Requests and Code Review

#21
post #10

Agree with the overall sentiment but disagree with > A good rule of thumb is 300 lines of code changes - once you get above 500 lines, you're entering unreviewable territory. I've found LoC doesn't matter when you split up commits like they suggest. What does matter is how controversial a change is. A PR should ideally have one part at most that generates a lot of discussion. The PR that does this should ideally also…

It’s often an inverse correlation in well functioning environments. Controversial changes and bug fixes are small and targeted and are deeply reviewed for unintended side effects, while large change sets are often new work that’s been discussed upfront and follows well established patterns and gets waved through.

Re: The Theatre of Pull Requests and Code Review

#22
post #3

If you split all the changes for a feature this way not only you hide the way all changes interact with each other but also make the development at least 10x longer because an average approval time is often more than a day.

> only you hide the way all changes interact with each

Splitting the change does not prevent you from looking at diffs of any combination of those commits. (Including whole pr) You're not losing anything.

> at least 10x longer because an average approval time is often more than a day.

Why do you think it would take longer to review? Got any evidence?

Re: The Theatre of Pull Requests and Code Review

#24
A while ago at a past job, I was working on OpenERP (now called Odoo I think). This "community" had instated this kind of "mandatory community review" policy so that each change had to be reviewed by X developers from other organizations. I kind of virtuous web of review.

But the thing is: this code is terrible and huge chunks of it are a unholy mix and match of code written for very specific purpose for this or that client, with this very weird "falsely generalized" code. I don't know how to call that: you have some very specific code, but you insert useless and probably buggy indirections everywhere so that it can be considered "general". The worst kind of code.

Anyways, I was asked by my boss to do such a review. I look at it and I realize that building a database setup to be able to properly run that code is going to take me weeks because I'm going to have to familiarize myself with tons and tons of modules I don't know about.

So I write down my estimate in our tracker: 1 month.

He comes back to me, alarmed. A whole month? Well yeah, otherwise I can't even run the code.

All you have to do is look at the code! What? No way, that ain't a review. Well, I ask you to do it as such. I'm not writing LGTM there.

So I was never asked to do reviews there again (in fact, I stopped working on OpenERP at all), but I could see "LGTM" popping up from my colleagues. By the way, on OpenERP tracker, all you ever saw in review logs was "LGTM" and minor style suggestions. Nothing else. What a farce.

So yeah, as the article says, there are some "LGTM-inducing" type of PRs, but the core of the problem is developers accepting to do this "LGTM-stamping" in the first place. Without them, there would only be reviewable PRs.

Re: The Theatre of Pull Requests and Code Review

#25
post #10

Agree with the overall sentiment but disagree with > A good rule of thumb is 300 lines of code changes - once you get above 500 lines, you're entering unreviewable territory. I've found LoC doesn't matter when you split up commits like they suggest. What does matter is how controversial a change is. A PR should ideally have one part at most that generates a lot of discussion. The PR that does this should ideally also…

Good luck getting 90% of devs to commit (har har) to this level of history surgery. Of the ones that actually know how to do it (a small fraction of your typical engineer) an even smaller fraction of that is going to have the patience to do it correctly. You’ll tell devs to do this kind of thing and you’ll either have their eyes glaze over from lack of understanding, annoyance at the extra work, or nodding then apathetical disregard. The one top engineer will do it then be frustrated that the rest of the org doesn’t do proper commit hygiene.

It’s not really something you can easily enforce with automation, so basically unachievable unless you are like Netflix and only hiring top performers. And you aren’t like Netflix.

Re: The Theatre of Pull Requests and Code Review

#27

PR review is probably at least a little performative. But I trust my colleagues to do good reviews when I ask them to, and to ignore my PRs when I don't. That's kind of the way we all want it. I regularly ask for a review of specific changes by tagging them in a comment on the lines in question, with a description of the implications and a direct question that they can answer. This, "throw the code at the wall for in…

>that should come out-of-band of the code in question.

Ideally, yes. After a decade and something' under ZIRP, seems a lot of workers never had incentive to remain conscious of their intents in context long enough to conduct productive discourse about them. Half of the people I've worked with would feel slighted not by the bitterness the previous sentence, but by its length.

There's an impedance mismatch between what actually works, and what we're expected to expect to work. That's another immediate observation which people to painfully syntaxerror much more frequently than it causes them to actually clarify context and intent.

In that case the codebase remains the canonical representation of the context and intent of all contributors, even when they're not at their best, and honestly what's so bad about that? Maybe communicating them in-band instead of out-of-band might be a degraded experience. But when out-of-band can't be established, what else is there to do?

I'd be happy to see tools that facilitate this sort of communication through code. GitHub for example is in perfect position to do something like that and they don't. Git + PRs + Projects implement the exact opposite information flow, the one whose failure modes people these days do whole conference talks about.

Re: The Theatre of Pull Requests and Code Review

#28

> Story-Telling Commit Messages No thank you. Talking to future ME, I don't need to know how I got to what I want me to look at. A squashed ticket-by-ticket set of merges is enough for me.

I'm editing this to be nicer. I'm really trying to be nicer. Consider the possibility you're not the only one in the codebase and that the git history might provide the why to the code's what.

Re: The Theatre of Pull Requests and Code Review

#29
If your goal is to lower the velocity of your organization, e.g. because in practice code churn or poor quality are major problems, then by all means do this.

If you still need to move fast, then don't.

This is the "don't run in the hallways" version of software culture, but I would contend that you should choose your pace based on your situation. It's just like gradient descent really. The be efficient sometimes you need to make big hops, sometimes small ones.

Re: The Theatre of Pull Requests and Code Review

#30
What I've often found is that people only really accept feedback from the Tech Lead, and peers are dismissed (not outright and not obviously - kind of sealioning etc). Peer-to-peer code reviews are another instance implementing a thing that pretends hierarchy does not exist.

You can only get basic tweaks accepted. The sunk-cost fallacy is a huge force.

Maybe I've only worked at crappy places

Post reply on HN