Live data from Hacker News

The Theatre of Pull Requests and Code Review

meks.quest

211–220 of 431 posts

Re: The Theatre of Pull Requests and Code Review

#211
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 think it's fine to have a whole bunch of "WIP" commit messages on intermediate commits while the PR is in a draft stage, but then all of those garbage commits should really be squashed down into one commit and you should at least write a one liner that describes what the whole change is doing. I think it does materially make repo history harder to understand to merge in PR's with 10 garbage commits in them.

Re: The Theatre of Pull Requests and Code Review

#212
post #98

I actually didn't ever realize that some people dreaded code reviews. To me, PRs are one of the most exciting parts of the process. That's where you have the most potential to learn from your colleagues and/or teach them something you know.

I am very glad that has been your experience!

It depends very much on your coworkers, unfortunately. When a team is all pulling in the same direction, and is kind and constructive and rigorous, code reviews can be awesome.

In some companies, especially ones with stack ranking where one person doing better means someone else does worse, it is easy for them to go horribly awry and become an ordeal of a bug hunt. The obvious solution is to not work at horrible companies that pit engineers against each other, but when that describes some of the biggest employers in the industry it's easier said than done.

Re: The Theatre of Pull Requests and Code Review

#213

Pro tip: Get your potential code reviewers involved before you even start coding. Keep them abreast of the implementation. When it comes time for a review they should be acquainted with your work.

I feel like this is a good use of standups rather than just status updates. "I have started working on X" should ideally be accompanied by "I am planning on doing this using Y and Z" if there is any chance that it could be contentious. There should ideally be no contentions, but the moment any questions come up, they should be tabled for a post-standup huddle amongst the relevant stakeholders.

If you don't do standups, just do the same thing ad-hoc on the team chat channel.

Larger tasks and discussions are probably the purview of RFCs.

Re: The Theatre of Pull Requests and Code Review

#215

Earlier quoted context omitted.

Give me one example then. One is all it takes to disprove a rule. Im fairly sure that I could explain how to break up any long PR in a sensible way. Parent thinks couldnt be done, so do you - what is an example? The only exception i can think of is something where 99.9% of the changes are autogenerated (where i wouldnt really be reading it carefully anyway, so the length is immaterial...).

> Im fairly sure that I could explain how to break up any long PR in a sensible way. Parent thinks couldnt be done, so do you - what is an example? Not couldn't - but shouldn't , such as when there's tight coupling across many files/modules. As an example, changing the css classes and rules affecting 20+ components to follow updated branding should be in one big PR[1] for most branching strategies. Sometimes it's eas…

You can and should break that up because I'm probably going to want to see screenshots to ensure that the branding changes make sense in context and everything looks consistent.

How would you do this? You'd either

1. Create N pull requests then merge all of them together into a big PR that would get merged into mainline at once 2. Do the same thing but do a bit of octopus merging since git merge can take multiple branches as arguments. Since most source control strategies are locked down, this isn't usually something that I can tell my juniors to do

The point of breaking things down like this is to minimize reviewer context. With bigger PRs there's a human tendency to try and hold the whole thing in your head at once, even if parts of the pull request are independent from others.

Re: The Theatre of Pull Requests and Code Review

#216
post #141

Earlier quoted context omitted.

> Intermediate commits are for the author's benefit, not the reviewer's. I don't even know how could commits only benefit the author; if they're poor they won't help him either, if not as a log of how much work he's done. Unless you make a PR for every insignificant change, PRs will most often be composed of series of changes; the individual commits, if crafted carefully, will let you review every step of the work of…

> I don't even know how could commits only benefit the author; if they're poor they won't help him either, if not as a log of how much work he's done. Intermediate commits are just checkpoints of unfinished code. The author knows that they made them and can revert back to them or use git log --pickaxe-S if there's code they saved to a checkpoint and want to recover. Intermediate commits can have meaningful commit mes…

> Intermediate commits can have meaningful commit messages if the author chooses, but they could also just be labeled "wip" and still be useful to the author.

> It's really easy for a note someone writes to themselves to be useful to that person without being useful to other people.

After a few months it will probably be as useful to you as to anyone else; if you only use commits as some sort of help while developing, you might as well just squash them before making a PR.

> What if the author experimented with a lot of approaches that turned out to be dead ends? Is it a good use of the reviewer's time to review all the failed attempts? Or is the author supposed to throw those away and reconstruct an imaginary commit history that looks like their clean, tidy thought process?

Yes, except that it doesn't matter if it's their thought process or not; it doesn't take a ton of time to reorder your commits, if you had some care for them in the first place.

It doesn't make much sense to place failed attempts in a series of commits (and of their reverts), just go back to the last good commit if something was a dead end (and save the failed attempt in a branch/tag, if you want to keep it around).

> If someone sends me a short story they wrote and asks for feedback, I can give them feedback without also demanding to see every prior draft and an explanation for every change they made until it reached the version I'm reviewing.

It's not the individual commits themselves that you need to review (although you can do that, if you place a lot of value to good histories); going through each commit, if they're indeed not just random snapshots but have been made thoughtfully, can let you review the PR a lot faster, because they'll be small changes described by the commits' messages.

Re: The Theatre of Pull Requests and Code Review

#217

I wish GitHub's PR UI was better at walking through a PR one commit at a time. As someone who does try to make good commits, and as someone who does try to read PRs sometimes a commit at a time, GitHub's UI gets in the way and keeps trying to drive you back to "whole PR" reviews. It is very telling in the article itself there is a screenshot of the commits tab in the PR workflow that many don't realize even exists an…

If faced with this issue, I would probably just pull the remote/branch locally and step through it, commit by commit, using my preferred Git manager (Lazygit).

My big complaints are with the default experience in how that affects everyone's expectaions, but I do this sometimes, yes. The VS Code "GitHub Pull Requests and Issues" extension has gotten really good and gives a lot of tools for this. If using github.com (and not GHE) you can even use the "." shortcut in any PR to open that PR in github.dev, which is a web version of VS Code (that you can Settings Sync) to review the PR there without even needing the local checkout. But also that's a bit of a "hidden shortcut" and very few developers know about it, which again gets back to what the GitHub interface provides by default and how it makes things discoverable being something of an underlying issue.

Re: The Theatre of Pull Requests and Code Review

#218

Earlier quoted context omitted.

> Im fairly sure that I could explain how to break up any long PR in a sensible way. Parent thinks couldnt be done, so do you - what is an example? Not couldn't - but shouldn't , such as when there's tight coupling across many files/modules. As an example, changing the css classes and rules affecting 20+ components to follow updated branding should be in one big PR[1] for most branching strategies. Sometimes it's eas…

You can and should break that up because I'm probably going to want to see screenshots to ensure that the branding changes make sense in context and everything looks consistent. How would you do this? You'd either 1. Create N pull requests then merge all of them together into a big PR that would get merged into mainline at once 2. Do the same thing but do a bit of octopus merging since git merge can take multiple bra…

> The point of breaking things down like this is to minimize reviewer context.

This principle is much more important than some rule that says "Merges to main should not be more than 150 lines long". Sticklers for hard-and-fast rules usually haven't achieved the experience to know that adhering to fundamental principles will occasionally direct you to break the rules.

Re: The Theatre of Pull Requests and Code Review

#219
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…

I actually find the relevant PR/MR discussion a lot more useful than the commit messages themselves. So any git blame is just to get a commit hash and look that up in GitLab/GitHub to see the entire change set and any comments around it. It makes me wish those comments were bundled with the merge commit somehow and could easily be accessed in the terminal where I'm viewing the git history.

Re: The Theatre of Pull Requests and Code Review

#220
post #92

Earlier quoted context omitted.

For any PR above a few line change, if a developer has not done a self review, I don’t review it all. Instead I request that it is self reviewed with context added, prior to requesting re-review. I also tend to ask the question, “are any of these insights worth adding as comments directly to the code?” 9/10 the context they wrote down should be well thought out comments in the code itself. People are incredibly lazy…

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.

Yep!

Adding context to both your commits and your code review tools pull requests / merge requests makes everyone's lives better. Including future you, who inevitably is looking at the PR or commit in the future due to an incident caused by said change.

I have been following this personal rule for well over a decade, and have never regretted it.

Post reply on HN