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

371–380 of 431 posts

Re: The Theatre of Pull Requests and Code Review

#371

Post by rdiddly »

Jeez, are there really people out there so afraid of looking unsmart that they won't stand up against the demon Complexity? Complexity is an evil whose presence you tolerate, not something you go chasing after like a trophy. Everybody is always trying to introduce more of it, especially noobs, inadvertently. It must be fought. And when you fight something, you don't blame yourself ("I'm too dumb to get this"). You blame the thing, you blame the complexity. "This shit is too complicated." Because guess what, chances are pretty high that if it's so sprawling, so bifurcated, so confusing, that you can't wrap your mind around it, it's because it's a piece of shit. E = mc² is what a finished product looks like. It has 2 variables and describes the whole universe. Whatever, you can argue all you want with me here, I'm being provocative, but the bottom line, regardless, is take yourself and your ego out of it. It's not you. You're not even important.

Edit to add: By fighting complexity, you're demanding more and you're right back to looking smart again. So actually your precious ego can still save face.

Re: The Theatre of Pull Requests and Code Review

#372

Post by theonething »

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.

my understanding is that you commit when you are at the "good place", where the part of the code you are working on works. That way when you keep going and find yourself going in a direction that is not right, you can go back to the last good place. If your code doesn't even compile, that doesn't seem like a good place.

Re: The Theatre of Pull Requests and Code Review

#373

Post by crowbahr »

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…

If I'm refactoring, truly refactoring, a 10k line PR where all the renaming happens at once is mandatory or else it won't compile. The only other option would be incremental refactors with an intermediate, parallel state that adds complexity, increases the likelihood of missing something and makes a 30 minute, 1 time PR review become 12x10 minute PR reviews.

Obviously it has to be a pure refactor, entirely isolated from functional changes but there are plenty of similar cases where doing it once is the least effort.

Re: The Theatre of Pull Requests and Code Review

#374

Post by [deleted] »

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.

[deleted]

Re: The Theatre of Pull Requests and Code Review

#375

Post by imiric »

mixmastamyk wrote: post #363
imiric wrote: post #327

Earlier quoted context omitted.

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

Virtue signalling? What are you on about?

Everything I said has direct benefits for the team, and hence for the company.

> If you want to communicate with others, write proper docs in a format that won't be lost to time

You have a severe misunderstanding of what commit messages are for. They're meant to describe changes that can be used as historical reference by developers. They're not meant to be read by non-developers, serve as replacement for "proper docs", or for general communication.

A VCS history is by definition never "lost to time". It is an immutable record of the development process of the project. If you don't find that useful, choose not to use it to its full potential, and strangely relish in that fact, you might as well use another tool.

Re: The Theatre of Pull Requests and Code Review

#376

Post by tetha »

BobbyJo wrote: post #175
leetrout wrote: post #157

Earlier quoted context omitted.

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…

A great way to frame DRY that I heard from hackernews: "DRY things that are supposed to have the same behavior, not things that happen to have the same behavior"

I enjoy Sandi Metz' point there as well: Code just looking the same is not enough to call it duplication. Once you have to change two places looking the same to add a new feature or to fix a bug, then you have duplication and should centralize it.

Re: The Theatre of Pull Requests and Code Review

#377

Post by imiric »

mixmastamyk wrote: post #362
imiric wrote: post #345

Earlier quoted context omitted.

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

> We use it to save our progress

Saving progress is useless if your history is a mess and you have no idea what a previous state contains.

> backup files, communicate with others.

You do know that there are better tools than a VCS specifically built for these use cases, right?

> You know, the main benefits of version control?

No, I don't think you understand what version control is for.

You can use a knife to open a wine bottle, but that doesn't mean it's a good idea.

> Writing a story no one will ever see is not one of them.

You won't. I definitely will, and I take the liberty to be as verbose as I need in personal projects.

> Write real docs and your PM, QA, SMEs will benefit as well, not only developers who bother to dig thru the history.

You should write "real docs", but that's not what commit messages are for. They're not meant to be read by non-developers either. And developers don't have to "dig thru the history" to see them. Commits are easily referenced and accessible.

Re: The Theatre of Pull Requests and Code Review

#378

Post by sfn42 »

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…

Honestly I didn't even know you were allowed to write that much. I've always tried to make the commit message fit in a sentence or two, what you showed here looks more like what I'd write on a PR description. Except I wouldn't write that much there either.

> Honest question: why do you even use version control? What do you get out of it? > Change history is the entire point of version control. It gives you the ability to revert to a specific point in time, to branch off and work on experiments, to track down the source of issues, and, perhaps most useful of all, to see why a specific change was done.

You answered your own question here. I get pretty much all of that stuff. Maybe you get some of that stuff a bit better than I do but I don't really think there's much of a difference. I can still go back in time and make changes etc, I can't necessarily revert every specific small change ever made using git alone but I can easily just make that change as a new commit instead - which is probably faster than scanning through a million commit messages trying to find the one I want to revert anyway.

I can go back to some arbitrary point in time just like you can. My resolution may not be as fine as someone who makes more commits but so what? Being able to go back to an arbitrary day and hour would be timetravel enough for me, I don't need to be able to choose the specific second.

Just to be clear I do make commits and I do try to write descriptive messages on them - I just also try to avoid spending more than a few seconds deciding what to write. That commit you just showed is larger than most of mine. That's a whole PR for me, which is pretty much what I said initally: I'll just do the whole task and commit it all in one go - what I'm not doing is splitting it up into 50 individual commits like some people would want.

I think the primary difference between the two of us is that you write huge commit messages and I don't, aside from that our commits seem very similar to me.

Re: The Theatre of Pull Requests and Code Review

#379

Post by watwut »

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…

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

Meh, most people wont address it or ask that dollar. It does not mean I did not read it, I chuckled and moved on.

I do read every commit on PR chain and every line. I am not necessary super attentive reviewer or something, but I never accept it without at least formally looking at it.

Re: The Theatre of Pull Requests and Code Review

#380

Post by rowanG077 »

imiric wrote: post #377
mixmastamyk wrote: post #362

Earlier quoted context omitted.

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.

> We use it to save our progress Saving progress is useless if your history is a mess and you have no idea what a previous state contains. > backup files, communicate with others. You do know that there are better tools than a VCS specifically built for these use cases, right? > You know, the main benefits of version control? No, I don't think you understand what version control is for. You can use a knife to open a…

IF you truly think the main point of version control is to maintain a coherent commit history than you are deluded. For most teams if it can do:

1. allow collaboration

2. Have branching and merging

3. Have diffs between two points in time/branches/tags

4. Allow release tagging

it is enough to work with it. Not to say that a coherent git history is great, but to call it the main point is something else. As that is definitely not how a lot of teams are using git or any version control.

Post reply on HN