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

251–260 of 431 posts

Re: The Theatre of Pull Requests and Code Review

#251

Post by sodapopcan »

jonahx wrote: post #200
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, 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…

> Reading and reviewing clean history is really so much nicer.

You can have both with git and it's not even hard. Unfortunately it seems many people pride themselves in what little they know of git. I'm not being sarcastic, I've read people say this almost word-for-word.

Re: The Theatre of Pull Requests and Code Review

#252

Post by sodapopcan »

m4r71n wrote: post #219
DavidWoof wrote: post #187

Earlier quoted context omitted.

> “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.

Not my experience. Often the single commit is all the context I need. If it's not, follow the merge to the ticket number to get more context.

Re: The Theatre of Pull Requests and Code Review

#253

Post by wpollock »

thayne wrote: post #225
MoreQARespect wrote: post #180

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...).

A widely used class has a bad API. You refactor it to make a cleaner API, but your change isn't backwards compatible. Your options are: - Change everything all at once. This creates a large PR. - Split it up into multiple small PRs. Now your individual PRs don't compile, and make less sense on their own. - Create a new class, and then split up multiple PRs that transition code to use the new class, and then finally a…

This is interesting. I believe one way to deal with such breaking changes is to have multiple PRs, where the breaking change in each is hidden (protected) by a feature flag and tested by unit tests. Once all the PRs are committed, end to end testing can be done by enabling the flag. Any problems in production can be quickly reverted by disabling the flag. Eventually, a final PR removes the now-useless flag.

Of course, your mileage may vary; this technique is certainly not suitable for all breaking changes or all workfkows.

Re: The Theatre of Pull Requests and Code Review

#254

Post by ziml77 »

williamcotton wrote: post #67

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.

This might be why I'm finding a lot of these comments so confusing. Because what you're saying here is how I've always operated.

If the first time your reviewer sees what decisions you've made is when the review happens, then of course it will be overwhelming if the merge request is large.

If you keep your reviewer in the loop, and have bounced implementation ideas off of them, then the review basically just becomes a sanity check.

Re: The Theatre of Pull Requests and Code Review

#255

Post by throwaway314155 »

MoreQARespect wrote: post #162
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…

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…

This doesn't match my experience and I assume is deeply cultural and subjective.

Re: The Theatre of Pull Requests and Code Review

#256

Post by William_BB »

jonahx wrote: post #200
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, 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.

Re: The Theatre of Pull Requests and Code Review

#257

Post by lamontcg »

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…

Sometimes you're overhauling something which you can't do in chunks less than something 2,000 line long PR. There's no intermediate working system. The problem is, "take this very large bit of code and throw it entirely away and rebuild it completely differently". Trying to craft some evolutionary step between A and B is just going to take 10x longer and won't help any code reviewers.

Re: The Theatre of Pull Requests and Code Review

#258

Post by riyanapatel »

This post hits on something crucial - the difference between performative code review and substantive collaboration. The "theatre" metaphor is spot-on. The core issue Saša identifies - PRs that are "unreviewable" getting rubber-stamped with "LGTM". Teams go through the motions without the actual substance.

The storytelling approach through commits is brilliant, but it only works if you solve the human factors too. Even perfectly crafted PRs with great commit narratives get surface-level reviews. The friction kills engagement.

A few complementary approaches I've seen work: pair reviewing for complex changes that are hard to break down, AI pre-screening for basic issues so humans can focus on architecture/business logic, and synchronous review sessions when async back-and-forth is just burning time.

The key insight: good PR structure needs to be paired with removing tooling friction. When review is painful, people default to "LGTM" regardless of how well the story is told.

Re: The Theatre of Pull Requests and Code Review

#259

Post by ants_everywhere »

overfeed wrote: post #218
status_quo69 wrote: post #215

Earlier quoted context omitted.

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.

> Merges to main should not be more than 150 lines long

This can be done by allowing a flag in the commit message that bypasses the 150 line long (or whatever example) rule in the CI that enforces it. Then the reviewers and submitter can agree whether or not it makes sense to bypass the rule for this specific case.

In many cases like this, it's okay to override a rule if the people in charge of keeping the codebase healthy agree it's a special case.

Re: The Theatre of Pull Requests and Code Review

#260

Post by notapenny »

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…

Couldn't you use something like GitLens for that? I haven't used it in a bit but IIRC it lets you see your changes versus any branch pretty easily. Personally if I do feel the need for a view of what I've touched, I just open up a draft PR.
Post reply on HN