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

361–370 of 431 posts

Re: The Theatre of Pull Requests and Code Review

#361

Post by mixmastamyk »

singpolyma3 wrote: post #354
sfn42 wrote: post #313

Earlier quoted context omitted.

I'm not the person you asked, but I often don't. I personally don't care about git history - I read code not commit messages. I don't really care how it got the way it is I care about what it is, maybe once in a blue moon there could potentially be some useful information in a commit message but it's not enough. So, I'll often just make lots of changes and then commit them all at once with some vague commit message a…

If there is only useful information in the commit message "once in a blue moon" it means someone isn't writing good commit messages. The number of times I look at a change and all I can think is "why did they/I do that" is very very often. Having the answer to that question available saves re learning the lesson that led to the change.

Put that in the developer-docs/issue-tracker where it has chance of being seen again. And not only by developers.

Re: The Theatre of Pull Requests and Code Review

#362

Post by mixmastamyk »

imiric wrote: post #345
sfn42 wrote: post #313

Earlier quoted context omitted.

I'm not the person you asked, but I often don't. I personally don't care about git history - I read code not commit messages. I don't really care how it got the way it is I care about what it is, maybe once in a blue moon there could potentially be some useful information in a commit message but it's not enough. So, I'll often just make lots of changes and then commit them all at once with some vague commit message a…

> I personally don't care about git history - I read code not commit messages. Honest question: why do you even use version control? What do you get out of it? Based on your workflow, you could just as well not use it at all, and create zip files and multiple copies of files with names like `_final3_working_20250925`. Change history is the entire point of version control. It gives you the ability to revert to a speci…

We use it to save our progress, backup files, communicate with others. You know, the main benefits of version control?

Writing a story no one will ever see is not one of them. Write real docs and your PM, QA, SMEs will benefit as well, not only developers who bother to dig thru the history.

Re: The Theatre of Pull Requests and Code Review

#363

Post by mixmastamyk »

imiric wrote: post #327
mvdtnz wrote: post #274

Earlier quoted context omitted.

> You should care because if the author cared enough to make descriptive atomic commits, they will help you understand why a particular change was done. This can often avoid unnecessary discussions. That's what the description field is for. I never, ever inspect the "commits" tab in a PR unless I see some lucicrous number on it. And even then it's just to see what the heck happened. > If the PR includes superfluous c…

> That's what the description field is for. No. The PR description is for describing the overall change, which, again, may include multiple commits. The description can also include testing instructions, reviewing suggestions, and other information which is not suitable for a commit message. PR descriptions can be edited and updated during the review, which can be helpful. A commit message is immutable, and remains a…

Virtue signaling is not a business need.

If you want to communicate with others, write proper docs in a format that won't be lost to time, and are accessible to everyone, not merely investigative developers.

Re: The Theatre of Pull Requests and Code Review

#364

Post by mixmastamyk »

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…

I commit early and often, then push the feature branch to gitlab, etc. Then I look at the draft merge-req diff in the browser.

Meaning, I can keep committing while also able to see the full changes evolve.

Re: The Theatre of Pull Requests and Code Review

#365

Post by lamontcg »

mateo411 wrote: post #298
lamontcg wrote: post #257

Earlier quoted context omitted.

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.

I agree. When you have a large PR like this, here's how I like to get it reviewed. 1. Give reviewers sometime to become familiar with the PR. They might not understand all parts of it, but they should have at least a cursory understanding of the PR. 2. Have a meeting where the PR is explained in front of the group of reviewers. The reviewers will understand the PR better and they can ask questions in realtime. 3. Let…

Yeah, ideally the reviewers have been in standups with you so that it isn't all new as a concept to them to begin with, or there's generally been communication that you're going to land the plans for a nuclear reactor in their work queue.

Hopefully you've been going around and around at a high level communicating back all the problems that you've hit and the design issues that emerged during exploratory surgery.

Then, you definitely want to schedule at least one meeting to go over it. Which can become several meetings, including follow-up meetings with one or two individuals to pound out some specific issue. Depends on the complexity of the nuclear reactor.

Re: The Theatre of Pull Requests and Code Review

#366

Post by herval »

criemen wrote: post #233
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 agree with you that this shouldn't be 100% hard defaults, but it's a good standard to have, and imo it's valuable to be explain why one is deviating from it. > - smaller PRs aren’t necessarily easier to review (and this arbitrary obsession almost always leads to PR overload in chunks that don’t make any sense, reducing code quality as a result) Oh but they sure can be reviewed more easily, because they are shorter?…

> we don't squash merge PRs

you need to be careful with every single commit message, every commit must compile, etc, in your case. My comments apply if you squash-merge, in which case all that commit-level care is not necessary since intermediate commits go away on merge. You’re probably making your life harder for no reason for avoiding squash-merge, but that’s just my opinion

Re: The Theatre of Pull Requests and Code Review

#367

Post by matheusmoreira »

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.

> "commit A", "change parts of A because I misunderstood a requirement", "improve A based on code review" etc.

People are supposed to rebase all that noise away. Changes are supposed to be structured as sensible chunks that build up to the desired feature. It's like showing your work in a math exercise: you don't write out the final answer with no explanation, you demonstrate step by step how you reached it.

Re: The Theatre of Pull Requests and Code Review

#368

Post by Freedom2 »

siva7 wrote: post #299
agentultra wrote: post #281

Earlier quoted context omitted.

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.

If your PR is + 1000 code lines long, you already made a mistake at the requirements and planning stage (like many teams do).

I don't agree, for example my team includes yarn.lock in the commit which adds quite a few lines to the PR.

Re: The Theatre of Pull Requests and Code Review

#369

Post by Freedom2 »

fillmore wrote: post #226
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…

Replying to echo this, I also read every commit message, and review PRs commit by commit. This was common practice at my last job (a small, experienced team), and the expectation was that commits were atomic. Yes, we griped that GitHub would not allow us to merge individual commits, but if it was ever urgent or helpful to do so, we cherry-picked a commit into a separate PR. Everyone's workflow is a bit different, and…

Thank you for this! I find it disturbing that the original-original poster extrapolated their experience to the rest of us as if it were a fact.

Re: The Theatre of Pull Requests and Code Review

#370

Post by BrenBarn »

The idea of insisting on a "clean" commit history always rubs me the wrong way. In my mind a commit is like hitting Ctrl-S. It's just saving a checkpoint in your work. And the commit history is supposed to show what actually happened, not a cleaned-up story about what happened.

I can see that it's good to be able to see that cleaned-up story, but I don't think the commit history should be that. There should be something else, a meta-statement about a bunch of commits, that summarizes it. But the commit history is still there if people later need to dig into how something was done. I saw some other comments talking about PRs as this unit, which maybe works, but I've always thought it would make more sense if the VCS had a notion of "commit sets" to which metadata could be attached. Then you could either look at such a set in collapsed form, with a single summary description of the whole set, or you could expand it out to look at the individual commits. In theory these could even be nested, although that might get messy and I think even just two levels would be very useful.

Post reply on HN