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

411–420 of 431 posts

Re: The Theatre of Pull Requests and Code Review

#411

Post by jonahx »

bccdee wrote: post #410
jonahx wrote: post #405

Earlier quoted context omitted.

This is simply not true. There is a "commits" tab and next button to quickly go through commits on every PR. It's very easy to use. All that you mean is most people ignore it.

And then when people put comments on your commits and you force-push new ones, does it link the updated version of each commit to its previous self, giving a clear timeline of comments and changes? I don't think so. But if people write comments at the MR level and you fix them in new commits, then the throughline is clear. I think a workflow like this for atomic commits would be nice. tangled.sh supports it for jujut…

I don't know -- these are fair points.

I do know that when I was using GH regularly on a team where a number of people wrote clean history, the problems you mentioned didn't come up, not that I can recall. So for the 90% case, let's say, you can do clean history on GH and get the majority of its benefits. But yes, I'm sure it's flawed especially in workflows where those types of problem arise often.

Re: The Theatre of Pull Requests and Code Review

#412

Post by rurban »

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 never work with squashers. There's no open source project I know of, which accepts squashers. Many companies do so, because their management is fucked, and if you see such a thing, look for another job immediately.

Re: The Theatre of Pull Requests and Code Review

#413

Post by whatevaa »

rurban wrote: post #412
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 never work with squashers. There's no open source project I know of, which accepts squashers. Many companies do so, because their management is fucked, and if you see such a thing, look for another job immediately.

Baseless claim, just your opinion.

Re: The Theatre of Pull Requests and Code Review

#414

Post by whatevaa »

michaelcampbell wrote: post #396
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…

> - “every commit must compile” - again, unnecessary overzealousness. Until you need to `git bisect`. Then you'll require that every commit compile, pass tests, etc.; even if that means rebase/squashing to do it.

You don't bisect a merge/pull request. There is no need for it, unless it is a giant, but then your workflow is different.

Main has clean history and every commit is good.

Re: The Theatre of Pull Requests and Code Review

#415

Post by krzyk »

themk wrote: post #355
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.

> Why would I care about ... You would not allow those commits. Code review improvements should appear as fixup commits which should be autosquashed on merge. It is a shame that GitHub does not support autosquash though.

Not sure, squashing is hiding history, I prefer to see history, even if it is not clean or buildable.

Re: The Theatre of Pull Requests and Code Review

#416

Post by krzyk »

singpolyma3 wrote: post #273
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…

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

Good commits are not a requirement form bisect. I commit when I think something more or less completed, or I want to start a major refactoring and I'm afraid I might need to revert it.

I don't always check if commits are buildable, PR should be, because that is what is merged to master and tip of master should be buildable.

Re: The Theatre of Pull Requests and Code Review

#417

Post by herval »

jiveturkey wrote: post #395
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…

I write intermediate commit messages as notes to self. You don't always work continuously on the same PR. The commit messages are a useful context refresher. Why advocate against this anyway? If no one reads them, it harms no one. Just like personal blogs. However, the writing of the blog is the useful act, not the reading. Ironic that you are accusing TFA article of being an expert novice. I don't disagree your take…

You missed the point entirely. The point is forcing others to do something that has no inherent value to them or to your process, just because you like it, is junior behavior.

Re: The Theatre of Pull Requests and Code Review

#418

Post by jiveturkey »

herval wrote: post #417
jiveturkey wrote: post #395

Earlier quoted context omitted.

I write intermediate commit messages as notes to self. You don't always work continuously on the same PR. The commit messages are a useful context refresher. Why advocate against this anyway? If no one reads them, it harms no one. Just like personal blogs. However, the writing of the blog is the useful act, not the reading. Ironic that you are accusing TFA article of being an expert novice. I don't disagree your take…

You missed the point entirely. The point is forcing others to do something that has no inherent value to them or to your process, just because you like it, is junior behavior.

Who's forcing? I might have misread TFA I guess. My reading was that the guy attended a conference, enjoyed the storytelling kind of talk (I mean this is a tried and true approach, there are even flash card decks on the story technique), and wrote a blog to capture and crystallize what he liked about it as it applies to his daily activity of writing code. I didn't read anything there claiming it was the one true way and anything else is a bankrupt approach.

If the point is about forcing someone to write commit essays, then yes I did miss it.

Re: The Theatre of Pull Requests and Code Review

#419

Post by bccdee »

jonahx wrote: post #411
bccdee wrote: post #410

Earlier quoted context omitted.

And then when people put comments on your commits and you force-push new ones, does it link the updated version of each commit to its previous self, giving a clear timeline of comments and changes? I don't think so. But if people write comments at the MR level and you fix them in new commits, then the throughline is clear. I think a workflow like this for atomic commits would be nice. tangled.sh supports it for jujut…

I don't know -- these are fair points. I do know that when I was using GH regularly on a team where a number of people wrote clean history, the problems you mentioned didn't come up, not that I can recall. So for the 90% case, let's say, you can do clean history on GH and get the majority of its benefits. But yes, I'm sure it's flawed especially in workflows where those types of problem arise often.

Yeah that's probably fair. I haven't used a commit-centric workflow in a professional context, so I can't really say how often, if ever, these issues come up.

Re: The Theatre of Pull Requests and Code Review

#420

Post by singpolyma3 »

krzyk wrote: post #416
singpolyma3 wrote: post #273

Earlier quoted context omitted.

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

Good commits are not a requirement form bisect. I commit when I think something more or less completed, or I want to start a major refactoring and I'm afraid I might need to revert it. I don't always check if commits are buildable, PR should be, because that is what is merged to master and tip of master should be buildable.

If a commit isn't buildable then when you get to it with bisect you have to skip. If this happens once in awhile it's not fatal, but it's sure not helpful
Post reply on HN