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

351–360 of 431 posts

Re: The Theatre of Pull Requests and Code Review

#351

Post by lucyjojo »

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

> PR is the atomic level of work. The atomic level of work should be a single, logically coherent change to the codebase. It's not managerial, it's explanatory. As you work things naturally arise. Over here a reformatted file, over there comments to clarify an old function that confused you, to help the next developer who encounters it. Cleaning, preparatory refactoring that is properly viewed as a separate action, a…

maybe if the majority of devs switched to jj or something similar your idea might become semi-common. but it seems to me that currently for most most project, it generates a lot of cost for very little gain.

My personal idea is that the code in each PR should be well documented enough for a review, but also for when people join the team and need to learn. Or when a pour soul needs to check upon your code while you are out on a holiday. This personal rule does not apply to all projects, but for bread and butter stuff I tend to go by it and not care about clean commit histories. The cost to reward seems way better.

Re: The Theatre of Pull Requests and Code Review

#352

Post by pcthrowaway »

TeMPOraL wrote: post #341
pcthrowaway wrote: post #332

Earlier quoted context omitted.

A lot of engineers do what you suggest rather than `git add; git commit --amend` This is why commits are often noise. If people are using commits well, they tell a story. The fact that people often use the tool wrong certainly begs some criticism of the tool, but when used correctly commits are certainly worth looking at one by one

> The fact that people often use the tool wrong certainly begs some criticism of the tool, but when used correctly commits are certainly worth looking at one by one What do you consider correct usage of git, and why? In this very discussion, I can see at least two distinct purposes that, more often than not, are mutually exclusive: - To "tell a story" for other people - To checkpoint units of work as individual perce…

In my usage, the story is for myself first and foremost. Telling the story helps keep me organized and helps me remember what I've done and where I'm going. I don't need to know that I fixed a typo in a comment, I need to know what the changes are overall doing.

Sometimes I go down a dead end, reverse out, and leave a comment about why a different approach would be a dead end. I (and others) don't need a record of the work I did on that path, just the synthesis (an explanatory comment)

There are multiple systems for structuring commits, but the commit message body content approximates to the same in all of them. The classic advice is https://tbaggery.com/2008/04/19/a-note-about-git-commit-mess... , but I find https://www.conventionalcommits.org/en/v1.0.0/ useful for looking at the oneline log

To address this point:

> To checkpoint units of work as individual perceives them, helping them deal with interruptions (which include running out of work day).

Yes, commits can be used like this! But once you have a chunk of work ready for review, cleaning up the commit log/history, grouping related changes, and describing them is useful for maintaining the software.

I don't like squash merges personally, though they have their merits. But regardless, I would copy the commit subject/body content into the PR message, which then puts everything into the PR commit also, so technically the granular commits are less relevant when one merges, but occasionally are still useful to refer to

Re: The Theatre of Pull Requests and Code Review

#353

Post by singpolyma3 »

TeMPOraL wrote: post #341
pcthrowaway wrote: post #332

Earlier quoted context omitted.

A lot of engineers do what you suggest rather than `git add; git commit --amend` This is why commits are often noise. If people are using commits well, they tell a story. The fact that people often use the tool wrong certainly begs some criticism of the tool, but when used correctly commits are certainly worth looking at one by one

> The fact that people often use the tool wrong certainly begs some criticism of the tool, but when used correctly commits are certainly worth looking at one by one What do you consider correct usage of git, and why? In this very discussion, I can see at least two distinct purposes that, more often than not, are mutually exclusive: - To "tell a story" for other people - To checkpoint units of work as individual perce…

There are not mutually exclusive. While I don't personally make the checkpoint style commits ever, I work with those who do. But they re-create a set of logical atomic story commits before submitting as a PR or otherwise

Re: The Theatre of Pull Requests and Code Review

#354

Post by singpolyma3 »

sfn42 wrote: post #313
singpolyma3 wrote: post #271

Earlier quoted context omitted.

If the commits don't matter why did you make them separate to begin with?

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.

Re: The Theatre of Pull Requests and Code Review

#355

Post by themk »

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.

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

Re: The Theatre of Pull Requests and Code Review

#356

Post by herval »

m000 wrote: post #349
herval wrote: post #339

Earlier quoted context omitted.

In most sane development cycles I've seen (from 2-people teams to 100k people teams), intermediate commits disappear as soon as you merge your branch (in other words, you do short lived branches + squash merge). If you decide to do merges without squashing, then yes, you gotta have to have more hygiene on each individual commit. It creates a lot of unnecessary friction and it's guaranteed to be slower (devs can't use…

> devs can't use commits as checkpoints/savepoints on their work But they can! In git you can do whatever you want with your local/remote working branch. And after you're done it's pretty straightforward to massage it into a coherent series of commits (especially if you had been working with that in mind). > each commit becomes a fully fleshed out "intermediate final state" This is really a team decision. You can all…

> And after you're done it's pretty straightforward to massage it into a coherent series of commits

Why waste time? Just squash and merge, you have a single commit and it WORKS. Intermediate messages disappear and you have a single, atomic rollback point on your main branch

> You can allow intermediate commits to e.g. fail the tests, and add a tag to your main/master after each merge. Then you know that only the tagged commits are guaranteed to be fully functional.

OR… squash and merge. Block merging with tests and compilation passing

For anything in tech, there’s the frictionless way and the busywork way. Both of your examples are busywork that’s completely unnecessary if you just… squash and merge

The best process is the process nobody needs to remember to do shit for it to work

Re: The Theatre of Pull Requests and Code Review

#357

Post by osigurdson »

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

If every commit on the main branch must compile then why wouldn't it also compile in the PR branch? It doesn't make sense to ask people to review, then after that rebase and merge imo.

Re: The Theatre of Pull Requests and Code Review

#358

Post by osigurdson »

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…

>> You want PRs because they help others absorb what you’re doing

That isn't really where it came from though. The idea was, if I want an open source maintainer to accept my changes, I make a request to pull them from my branch. Once the open source maintainer has merged it in, they own it. If they don't like it (even one little bit), they can reject it because quality / ownership / maintenance is completely on them.

On a team environment where no one owns anything it is a little less clear what the value is. You want to incentivize the "betterness" of "something" and are using "broadened knowledge" as a proxy for that. Usually this just goes unexamined but really it would be good to establish how broad and deep you want this knowledge to be and work back from there - is the 5 minute PR review the best way to achieve it?

Re: The Theatre of Pull Requests and Code Review

#359

Post by jordwest »

crazygringo wrote: post #174

I can't agree with a lot of this. PR's should generally be the size of a feature, or a meaningful subfeature for large features. When you arbitrarily split up PR's into something "300 lines" or "5-10 minutes" you can miss the forest for the trees. The little thing looks fine in isolation but doesn't make any sense as part of a larger approach. Different people are reviewing it piecemeal but nobody is reviewing the ap…

Thank you for saying this. This drove me up the wall everywhere I worked that had those arbitrary ~300 line rules as all that happened was code review devolved into "I don't like this code pattern, use another code pattern". While the larger architectural ideas that actually became problematic were rarely reviewed since they weren't obvious when split across 5 PRs often with different reviewers.

Honestly I think it would be far more effective to just review a paragraph and maybe a diagram that explains "here's how I think I'm going to tackle this problem" and forget about line-by-line code review entirely. Other than for training juniors, I don't think there's much long term value in "I think you should use an anonymous function here instead of a named function".

The kinds of things that are usually brought up in code review are not what contributes to real long term technical debt, because a function name or code formatting can be changed in an instant, while larger architectural decisions cannot.

The other thing I noticed is that even when an architectural issue is obvious, there's a tendency to not want to change it because so much of the work has already been done in preparing the PR for review. If you point out a flaw in an architectural decision, it's not unusual for the person to reasonably say "I've just put together a chain of 5 PRs and now you're asking me to rewrite everything?"

Re: The Theatre of Pull Requests and Code Review

#360

Post by markus_zhang »

kiitos wrote: post #291
markus_zhang wrote: post #239

Earlier quoted context omitted.

> We absolutely expect our reviewers to fully understand the code. I suspect you don’t work as a DE or your company has top notch culture. I have never worked in any companies (including some big ones) that encourage a full understanding of any code submitted to PR review.

more or less the entire point of code review is to ensure that the reviewer fully understands the code that they review i know that lots of orgs don't do it this way, but it's really important to make this point very clear: those orgs are wrong, and pathological, no matter how many of them there are

I want to agree with you. But I'd argue that for some career paths it is just impossible, and it is definitely impossible for my company. That's why I never consider myself as an engineer -- and I'm fine with it, because I don't love what I do. I take my side projects more engineer-ish.
Post reply on HN