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

281–290 of 431 posts

Re: The Theatre of Pull Requests and Code Review

#281

Post by agentultra »

DSMan195276 wrote: post #277
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.

> 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 warrant…

It also enables bisect to work properly.

Bisecting on squashed commits is not usually helpful. You can still narrow down to the offending commit that introduced the error but… it’s somewhere in +1200/-325 lines of change. Good luck.

Re: The Theatre of Pull Requests and Code Review

#282

Post by kiitos »

conradludgate wrote: post #196
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…

My recommendation: don't treat the development process as the final draft. Write everything in one go, and then afterwards rework the commits to tell the story you want to tell. This does require getting very comfortable with git rebase, but I can absolutely recommend it. Using this technique, I've managed to merge several large refactors lately with no issues, and the reviewers didn't hate me.

holy moly absolutely not

git is a means, not an end

code review is about the code as a unit whole, not the steps along the way!

Re: The Theatre of Pull Requests and Code Review

#283

Post by dkarl »

There's a chicken-and-egg problem with "story-telling commits." Codebases don't contain commits that tell stories, so engineers don't look for them, so engineers don't understand the value of them, so engineers don't learn the skills of rewriting commits, so engineers just squash everything, so codebases don't contain commits that tell stories.

Re: The Theatre of Pull Requests and Code Review

#284

Post by Qerub »

roguecoder wrote: post #149
scuff3d wrote: post #128

Earlier quoted context omitted.

The problem that I find myself in is that I almost always run into stuff I didn't expect. Some integration that I thought would be minor turns out to slowly get out of hand, and before I know it I've made way more changes than I meant to. And then it all gets tangled together. Maybe it's just a me problem, maybe I need to be more disciplined. Not sure but it catches me quite often.

That's one of the challenges with making changes all at once: it is a lot easier for one thing going wrong to suddenly result in thousands of lines of changes. One technique I use when I find that happening is to check out a clean branch, and first make whatever structural change I need to avoid that rabbit hole. That PR is easy to review, because it doesn't change any behavior and there are tests that verify none of…

There's a nice Manning book from 2014 about this way of working named The Mikado Method.

Re: The Theatre of Pull Requests and Code Review

#285

Post by ricardobeat »

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…

Keep merging each PR into master under a feature flag, that's how it's done. Huge PRs that implement a feature in one swoop are pretty much the worst case scenario for every stage: review, testing, deployment and monitoring.

Re: The Theatre of Pull Requests and Code Review

#286

Post by William_BB »

singpolyma3 wrote: post #269
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.

The PR UI on GitHub definitely leads to treating it as the unit of work. I consider this unfortunate for the most part, but the basic side effect is that I'll often end up submitting every commit as a separate PR so they actually get looked at

Respectfully, I disagree. I understand people have vastly different experiences and preferences. In my ideal world, a PR is a unit of work that has an in-state and an out-state. I don't have to see an initial commit within a PR with a full fledged spec and then wonder if any future commits within the same PR overrode those changes. A PR will rarely be clean from the start.

The way it appears to me, if there's multiple commits submitted as separate PRs, then maybe the PR wasn't so atomic to begin with.

Re: The Theatre of Pull Requests and Code Review

#287

Post by kiitos »

scottgg wrote: post #232

As a jujutsu fan, I like to prepare a nice clean commit history for my reviewers to probably ignore. But I kind of assume - there’s _some tool_ we could use - perhaps on top of GitHub - to make stacked reviews not suck, either actually stacked PRs or just incrementally reviewing a nicely divided branch PR. I thought it might be graphite but it seems that needs some whole other tooling _stuff_ on top? Any recommendati…

yeah, for sure -- avoid stacking changes in the first place! the purpose of PRs and code review is to establish a single unified shared context between multiple stakeholders. it's the author's responsibility to propose changes that are independent and coherent and easy to understand.

Re: The Theatre of Pull Requests and Code Review

#288

Post by 1-more »

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.

Very common practice at my old company, and one I continue in my current role.

> “every commit must compile”

sucks ass for anyone else trying to rebase your branch onto the update main/master when they don't. Once your PR is out of "working on the feature" and into the "getting it merged" phase, do a little `git rebase -i` and squash your really intermediate commits into ones that compile. Ignore this if you have real grown up CI where your PRs never stay open for more than a day.

Re: The Theatre of Pull Requests and Code Review

#289

Post by kiitos »

g-b-r wrote: post #127
Master_Odin wrote: post #116

Earlier quoted context omitted.

I'm a strong believer that PRs should be merged via a "squash and merge" strategy, with the singular commit being descriptive of the overall change and having a link back to the PR for deeper story analysis as needed. I'm also a staunch believer at this point that PRs should really focus on one thing as well. If when working on a bug you discover another semi-related bug? Open two PRs. Let main be the story of how co…

PRs only live on GitHub, what happens if it gets shut down or it accidentally loses some data?

the entire repo lives only on github as well, there is no meaningful difference between git commits and PR comments in a github-hosted repo
Post reply on HN