Hacker News Board Hacker News, browsed like a classic message board
Live data from Hacker News

The Theatre of Pull Requests and Code Review

meks.quest

Post reply on HN
Print view View original

271–280 of 431 posts

Re: The Theatre of Pull Requests and Code Review

#271

Post by singpolyma3 »

ownagefool wrote: post #263
William_BB wrote: post #256

Earlier quoted context omitted.

PR is the atomic level of work. I'd argue PR-level history (i.e. squash) is often enough and is way cleaner. Why would I care about "commit A", "change parts of A because I misunderstood a requirement", "improve A based on code review" etc.? If I want that granularity, I'd go read the original PR and the discussion that took place.

Yeah, I agree with both you and the GP. There's a mess of commits that usually don't matter because mostly only the before and after level of an actual viable PR does, ergo I squash them. I'm cool with other reasonble approaches though, but I'm pretty over pointless hoops because someone says so.

If the commits don't matter why did you make them separate to begin with?

Re: The Theatre of Pull Requests and Code Review

#272

Post by singpolyma3 »

BobbyJo wrote: post #171
MoreQARespect wrote: post #162

Earlier quoted context omitted.

The "mid levels who consider themselves senior" are the exact type of people who I see saying what you're saying, i.e. * Yes, TDD on production code is nice in theory, but it doesnt work in my case . * Yes, short PRs are nice in theory, but it doesnt work in my case . In every case, as far as I can see, it meant "It does work, I just dont know how to do it". When I say "if you dont think it works in your case, come t…

> Yes, TDD on production code is nice in theory, but it doesnt work in my case... Parent said something more along the lines of "they don't work in every case, and trying to force it in every case is misguided". I agree that too big is more common than too small with respect to PR size, but you aren't putting forward much of an argument against parents "there are no absolutes" argument by straw manning them.

I think "doesn't work in every case" is true for basically every rule of course. But 99% of people in the industry are not qualified to make that call because they will always choose "not" out of laziness rather than because it actually wasn't a good idea

Re: The Theatre of Pull Requests and Code Review

#273

Post by singpolyma3 »

DavidWoof wrote: post #187
herval wrote: post #150

I find the sort of opinions on this post quite common on a subset of engineers - namely mid levels with some time in the career, who start to consider themselves senior engineers and want everyone to follow the same set of strict rules they decided make sense. It’s the same mindset that makes people pedantically apply DRY to every situation or forcing others to TDD basic apps. In practice: - smaller PRs aren’t necess…

> “nobody reads intermediate commit messages one by one on a PR” I clean my history so that intermediate commits make sense. Nobody reads these messages in a pull request, but when I run git blame on a bug six months later I want the commit message to tell me something other than "stopping for lunch". > pedantically apply DRY to every situation or forcing others to TDD basic app Sure, pedantically doing or forcing an…

Yes. I think many people have no culture of good commits, so they never use bisect or blame, so they never see the use of good commits. It's a cycle

Re: The Theatre of Pull Requests and Code Review

#274

Post by mvdtnz »

imiric wrote: post #264
William_BB wrote: post #256

Earlier quoted context omitted.

PR is the atomic level of work. I'd argue PR-level history (i.e. squash) is often enough and is way cleaner. Why would I care about "commit A", "change parts of A because I misunderstood a requirement", "improve A based on code review" etc.? If I want that granularity, I'd go read the original PR and the discussion that took place.

You should care because if the author cared enough to make descriptive atomic commits, they will help you understand why a particular change was done. This can often avoid unnecessary discussions. And, no, PRs are not necessarily an atomic level of work. While they should contain a single feature, fix, etc., sometimes that work can span multiple commits. If the PR includes superfluous commits, then they should be squ…

> You should care because if the author cared enough to make descriptive atomic commits, they will help you understand why a particular change was done. This can often avoid unnecessary discussions.

That's what the description field is for. I never, ever inspect the "commits" tab in a PR unless I see some lucicrous number on it. And even then it's just to see what the heck happened.

> If the PR includes superfluous commits, then they should be squashed into the appropriate commit.

This happens on merge if your Github is set up correctly.

> Squashing the entire PR when it includes multiple changes is simply a bad practice.

The bad practice is the PR changing multiple distinct things.

> It's bad because you lose all the history of how the overall change was done, which will be useful in the future when you need to do a blame, cherry pick, bisect, etc.

It's not.

Re: The Theatre of Pull Requests and Code Review

#275

Post by bornfreddy »

DerArzt wrote: post #102
tcoff91 wrote: post #92

Earlier quoted context omitted.

By self review, you mean that the developer adds comments in the code review tool? that is a great idea, I want to try this.

I've been doing this as part of my workflow for a few years now. My coworkers have expressed appreciation around that effort. A nice side effect is that going through a self review and adding comments to the PR has helped me catch innumerable things that my coworkers never had to call me on.

Do you see any added value in adding the comments? I just fix the original commits and force push (each dev owns their branches at $JOB), but I'm wondering if I'm missing something?

Re: The Theatre of Pull Requests and Code Review

#276

Post by kiitos »

vault_ wrote: post #76
davedx wrote: post #41

It's a very common refrain but I don't really agree with it: "How do you create a PR that can be reviewed in 5-10 minutes? By reducing the scope. A full feature should often be multiple PRs. A good rule of thumb is 300 lines of code changes - once you get above 500 lines, you're entering unreviewable territory." The problem with doing this is if you're building something a lot bigger and more complex than 500 lines o…

> - A big queue of PR's for reviewers to review This is a feature. I would infinitely prefer 12 PRs that each take 5 minutes to review than 1 PR that takes an hour. Finding a few 5-15 minute chunks of time to make progress on the queue is much easier than finding an uninterrupted hour where it can be my primary focus. > - The of the feature is split across multiple change sets, increasing cognitive load (coherence is…

code review isn't about diffs, it's about holistic changes to the project

the point is not queue progression, it is about dissemination of knowledge

one holistic change to a project = one PR

simple stuff really

Re: The Theatre of Pull Requests and Code Review

#277

Post by DSMan195276 »

William_BB wrote: post #256
jonahx wrote: post #200

Earlier quoted context omitted.

> nobody reads intermediate commit messages one by one on a PR, period. I worked on a team where the lead was adamant about this and started to write messages in the vein of “if you’re reading this message, I’ll give u $5”. I never paid anyone a dollar. Don’t waste your time writing stuff for no one. I do. Especially if the author is competent. That said, empirically, you're correct most people don't. However, that s…

PR is the atomic level of work. I'd argue PR-level history (i.e. squash) is often enough and is way cleaner. Why would I care about "commit A", "change parts of A because I misunderstood a requirement", "improve A based on code review" etc.? If I want that granularity, I'd go read the original PR and the discussion that took place.

> I'd go read the original PR and the discussion that took place.

Until your company switches code repos multiple times and all the PR history is gone or hard/impossible to track down.

I will say, I don't usually make people clean up their commits and also usually recommend squashing PRs for any teams that aren't comfortable with `git`. When people do take the time to make a sensible commit history (when a PR warrants more than one commit) it makes looking back through their code history to understand what was going on 1000% easier. It also forces people to actually look over all of their changes, which is something I find a lot of people don't bother to do and their code quality suffers a lot as a result.

Re: The Theatre of Pull Requests and Code Review

#278

Post by [deleted] »

imiric wrote: post #264
William_BB wrote: post #256

Earlier quoted context omitted.

PR is the atomic level of work. I'd argue PR-level history (i.e. squash) is often enough and is way cleaner. Why would I care about "commit A", "change parts of A because I misunderstood a requirement", "improve A based on code review" etc.? If I want that granularity, I'd go read the original PR and the discussion that took place.

You should care because if the author cared enough to make descriptive atomic commits, they will help you understand why a particular change was done. This can often avoid unnecessary discussions. And, no, PRs are not necessarily an atomic level of work. While they should contain a single feature, fix, etc., sometimes that work can span multiple commits. If the PR includes superfluous commits, then they should be squ…

[deleted]

Re: The Theatre of Pull Requests and Code Review

#279

Post by kiitos »

WorldMaker wrote: post #224
candiddevmike wrote: post #129

Earlier quoted context omitted.

Can you describe a little more about why you walk through individual commits instead of just reviewing the latest commit only?

Good commits can tell a story. (The article here discusses this, too, and suggests that good commits should tell a story.) When a commit author takes the time to fill out the commit message, it will include things like the "how" and "why" of each step, what they were thinking about as they worked on that part of the whole PR. Often I find that will save you from asking questions like "Why did you take this approach?"…

> But yes, not everyone can or will write good commits.

some people treat commits as meaningful units of independent review, and some people treat them as savepoints and the PR as the only meaningful unit of review, it's a distinction of process, not purity -- both approaches are totally fine, one is not better than the other

git and commits and prs are means, not ends

Re: The Theatre of Pull Requests and Code Review

#280

Post by reitanuki »

BowBun wrote: post #61
stack_framer wrote: post #55

My manager recently told our team that "AI usage" would be added to our engineering competencies, and we would all be expected to "use AI more." When I said my top preference for AI usage, by far, would be to eliminate human code reviews, the response was basically, "Oh, not like that."

That's a bummer! At my company we've started investing in what I'm calling 'semantic linting', which is basically running GPT over a PR with a set of rules that we iterate on. Already I'm finding huge value for style/pattern comments that linters can't easily catch, dropping warnings for common DB migration footguns, or notifying people of changing patterns/new ways of doing things. Been great so far!

Do you have any write-up about this or more info? It sounds like a useful use case but I haven't yet got it right
Post reply on HN