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…
The Theatre of Pull Requests and Code Review
21–30 of 431 posts
Re: The Theatre of Pull Requests and Code Review
#22If 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.
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
#23No 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.
Re: The Theatre of Pull Requests and Code Review
#24But 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
#25Agree 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 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
#26Reviewing someone else's large pull request is like having a second task in parallel with what you are working on yourself!
Re: The Theatre of Pull Requests and Code Review
#27PR 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…
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.
Re: The Theatre of Pull Requests and Code Review
#29If 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
#30You can only get basic tweaks accepted. The sunk-cost fallacy is a huge force.
Maybe I've only worked at crappy places