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

151–160 of 431 posts

Re: The Theatre of Pull Requests and Code Review

#151

Post by Illniyar »

I disagree with practically everything suggested.

Reducing scope and splitting a single task into multiple PRs each small but part of a bigger picture makes it very hard to see the bigger picture.

You should try to make PRs small, but if a PR is big, then you just have to spend more time to review it.

Formatting commits as a story is a huge hurdle for the one making the changes. And unless every PR is meticulously prepared - going over the commits by the reviewer is a waste of time.

I agree you should return PRs you don't understand though. Or don't feel comfortable reviewing for whatever reason.

Re: The Theatre of Pull Requests and Code Review

#152

Post by CraigJPerry »

Pair programming > PR reviews.

I've heard people scoff at the $$ cost of "mob programming". I think that view is totally myopic, for appropriate problems there's just no faster nor higher bandwidth way to transfer code knowledge in a group.

Plenty of people dislike pair programming, i don't dislike it but i do find it mentally intense, tiring. I really really enjoy that it's an accelerator for getting to done - not just i wrote the code but the code is correct - sooner.

Long way to say don't rely on pull requests when you could be doing pairing for the important stuff.

Re: The Theatre of Pull Requests and Code Review

#153

Post by hypeatei »

roblh wrote: post #146

Maybe I'm just a scrub, but something that I find makes it harder to do smaller commits is that I frequently rely on being able to see which lines I've changed directly inline in my editor. When you commit, vscode now stops highlighting all of those lines, and that makes it much more difficult for me to orient myself relative to what I've already done. The individual lines, and the git pane that shows which files hav…

  git add --patch
...is your friend if you want to leave all your changes unstaged for awhile then break it out into multiple commits later.

Re: The Theatre of Pull Requests and Code Review

#154

Post by t-writescode »

roblh wrote: post #146

Maybe I'm just a scrub, but something that I find makes it harder to do smaller commits is that I frequently rely on being able to see which lines I've changed directly inline in my editor. When you commit, vscode now stops highlighting all of those lines, and that makes it much more difficult for me to orient myself relative to what I've already done. The individual lines, and the git pane that shows which files hav…

This sounds like a real and valid incompatibility with a high frequency commit cadence.

If you’re interested in trying these strategies anyway, does your editor of choice have an inline “git blame”? In IntelliJ, I can see who and when committed the most recent change in the line around the one I’m working on.

It doesn’t resolve the “which files have I worked on” issue; but it might help the others? Not as nice as a different colored line like uncommitted code would otherwise be highlighted, but it could be enough of a step in that direction?

Re: The Theatre of Pull Requests and Code Review

#155

Post by crazygringo »

roblh wrote: post #146

Maybe I'm just a scrub, but something that I find makes it harder to do smaller commits is that I frequently rely on being able to see which lines I've changed directly inline in my editor. When you commit, vscode now stops highlighting all of those lines, and that makes it much more difficult for me to orient myself relative to what I've already done. The individual lines, and the git pane that shows which files hav…

That's really interesting.

Seems like it would be even better if VS Code provided a way to highlight all lines changed relative to a particular commit like the start of a branch. Maybe it's worth filing a feature request?

(I don't use VS Code this way so I'm assuming it doesn't already have this.)

Re: The Theatre of Pull Requests and Code Review

#156

Post by rd »

roblh wrote: post #146

Maybe I'm just a scrub, but something that I find makes it harder to do smaller commits is that I frequently rely on being able to see which lines I've changed directly inline in my editor. When you commit, vscode now stops highlighting all of those lines, and that makes it much more difficult for me to orient myself relative to what I've already done. The individual lines, and the git pane that shows which files hav…

you could probably write an extension to accomplish this in a couple of days with GPT-5 now

Re: The Theatre of Pull Requests and Code Review

#157

Post by leetrout »

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…

My simple suggestion to my teams:

PRs are emails to your team and to your future self.

Framed in that context it's easier to carry the correct tone and think about scoping / what's important.

---

> pedantically apply DRY to every situation

I swear DRY has done more damage to the software industry from the developer side than it has done good because it has manifested into this big stick with which to bludgeon people without taking context into account.

Re: The Theatre of Pull Requests and Code Review

#158

Post by eCa »

flir wrote: post #49
pards wrote: post #7

Earlier quoted context omitted.

> and to ignore my PRs when I don't PRs should be optional, IMHO. Not all changes require peer review, and if we trust our colleagues then we should allow them to merge their branch without wasting time with performative PRs.

I always appreciate an extra pair of eyeballs, even on a one-liner. Everyone's an idiot sometimes.

I’m firmly in this boat too. If it’s a small change I can likely get it reviewed within minutes, if it isn’t small it should have a review regardless.

Re: The Theatre of Pull Requests and Code Review

#159

Post by t-writescode »

hypeatei wrote: post #153
roblh wrote: post #146

Maybe I'm just a scrub, but something that I find makes it harder to do smaller commits is that I frequently rely on being able to see which lines I've changed directly inline in my editor. When you commit, vscode now stops highlighting all of those lines, and that makes it much more difficult for me to orient myself relative to what I've already done. The individual lines, and the git pane that shows which files hav…

git add --patch ...is your friend if you want to leave all your changes unstaged for awhile then break it out into multiple commits later.

To add, when I’m breaking my changes down into multiple parts for review, I tend to:

  * squash everything I’ve done into one commit
  * create a new branch off main/master that will be the “first commit”
  * cherry-pick changes (easy from some git guis) that represent a modular change.
  * push and make an MR from the new branch
  * rebase “the big commit” on top of the partial change.
  * wash, rinse and repeat for each change, building each MR off its requisite branch.
The squashing part is vital because otherwise you enter merge conflict hell with the rebase.

Re: The Theatre of Pull Requests and Code Review

#160

Post by roguecoder »

frankc wrote: post #134
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…

I also feel like what gets lost in this is not everything you are building is a bite size feature in large existing project. Sometimes you are adding an entire subsystem that is large to something relatively greenfield. if you broke that down into features, you will need 20 PRs and if you wait for review, or even don't wait but have to circle back to integrate lots of requested changes, what might be a couple of week…

If you are the only developer who ever going to work on something, maybe. Even then, I will argue you are more likely to deliver successfully if you are cutting your work into smaller pieces instead of not delivering anything at all for weeks at a time.

But for the company, having two people capable of working on a system is better than one, and usually you want a team. Which means the code needs to be something your coworkers understand, can read and agree with. Those changes they ask for aren't frivolous: they are an important part of building software collaboratively. And it shouldn't be that much feedback forever: after you have the conversation and you understand and agree with their feedback, the next time you can take that consideration into account when you are first writing the code.

If you want to speed that process up, you can start by pair programming and hashing out disagreements in real time, until you get confident you are mostly on the same page.

Professional programming isn't about closing tickets as fast as possible. It is about delivering business value as a team.

Post reply on HN