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

121–130 of 431 posts

Re: The Theatre of Pull Requests and Code Review

#121

Post by g-b-r »

leoedin wrote: post #33
smashedtoatoms wrote: post #28

Earlier quoted context omitted.

I'm editing this to be nicer. I'm really trying to be nicer. Consider the possibility you're not the only one in the codebase and that the git history might provide the why to the code's what.

I do think commit messages should give some reference to what they're changing. However, in more than a decade of software development, I don't think I've ever got much use out of commit messages. The only reason I'd even look at them is if git blame showed a specific commit introduced a bug, or made some confusing code. And normally the commit message has no relevant information - even if it's informative, it doesn'…

> However, in more than a decade of software development, I don't think I've ever got much use out of commit messages.

> normally the commit message has no relevant information

Maybe that's why you've never got much use of them?

If your colleagues or yourself wrote better commits, they could have been of use.

An easily readable history is most important in open source projects, when ideally random people should be able to verify what changed easily.

But it can also be very useful in proprietary projects, for example to quickly find the reason of some code, or to facilitate future external security reviews (in the very rare case where they're performed).

Re: The Theatre of Pull Requests and Code Review

#122

Post by scuff3d »

xmcqdpt2 wrote: post #44
jvanderbot wrote: post #5

PR review is probably at least a little performative. But I trust my colleagues to do good reviews when I ask them to, and to ignore my PRs when I don't. That's kind of the way we all want it. I regularly ask for a review of specific changes by tagging them in a comment on the lines in question, with a description of the implications and a direct question that they can answer. This, "throw the code at the wall for in…

This is a huge pet peeve of mine. At work I'm an expert on part of the code base that sees a fair number of contributions.I get many private IMs from colleagues asking me to "Approve please" or something like that with a dump of 100s of lines, of which maybe 10 lines are relevant to me (touch files or behaviour that I'm an expert on, hence why they need my approval.) Minimally, I would like context for the change, wh…

Commit descriptions are criminally under used for this purpose. You can add so much more context if you don't limit yourself to just the short commit message.

Re: The Theatre of Pull Requests and Code Review

#123

Post by eterm »

sexyman48 wrote: post #104
eterm wrote: post #58

Earlier quoted context omitted.

So don't do it in parallel. Completely park other tasks, spend time on the review and record that time appropriately. There's nothing wrong with saying you spent the previous day doing a large review. It's a part of the job, it is "what you're working on".

You might as well go into HR. Everyone knows reviewing other people's PRs is like nurturing their kids at the expense of your own.

I don't understand what you're trying to say.

Re: The Theatre of Pull Requests and Code Review

#124

Post by g-b-r »

whstl wrote: post #42
smashedtoatoms wrote: post #28

Earlier quoted context omitted.

I'm editing this to be nicer. I'm really trying to be nicer. Consider the possibility you're not the only one in the codebase and that the git history might provide the why to the code's what.

As someone who dives in the commit story often: If it's a pristine, linear, commit story, sure. If it includes merges commits, "fix" commits, commits that do more than one thing, detours, side-quests, unrelated refactors then squashing is 100x better.

Merge commits allow you to nest series of commits, they're far from always bad

Re: The Theatre of Pull Requests and Code Review

#125

Post by manwe150 »

goosejuice wrote: post #103
tcoff91 wrote: post #92

Earlier quoted context omitted.

By self review, you mean that the developer adds comments in the code review tool? that is a great idea, I want to try this.

I thought everyone did this. I review twice. For each commit with -v and finally in GH/GL after I open the PR/MR. I often catch something on that last one. It's rubber ducking.

What is `-v` mean here? I was assuming `git show`, but that doesn't seem to have a `-v` parameter.

Re: The Theatre of Pull Requests and Code Review

#126

Post by scuff3d »

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…

> You end up doing work on branches of branches, and end up either having to become a rebase ninja or having tons of conflicts as each PR gets merged underneath you

This shouldn't matter unless you are squashing commits further back in the tree before the PR or other people are also merging to main.

If a lot of people are merging back to main so you're worried about those causing problems, you could create a long life branch off main, branch from that and do smaller PRs back to it as you go, and then merge the whole thing back to main when your done. That merge might 2k lines of code (or whatever) but it's been reviewed along the way.

I don't necessarily disagree with you. Just pointing out that there are ways to manage it.

Re: The Theatre of Pull Requests and Code Review

#127

Post by g-b-r »

Master_Odin wrote: post #116
parpfish wrote: post #99

i don't know how people can build something and leave behind a coherent series of commits that tell a nice story of progressively building a thing. my commits include lots of false-starts that get abandoned and "i need to commit this interim state because i deprioritized this and will come back later". the sequence of events that i used to build the thing isn't necessarily the best sequence of events to tell the stor…

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?

Re: The Theatre of Pull Requests and Code Review

#128

Post by scuff3d »

sebk wrote: post #73
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…

I agree with this. One way to keep changes small but still compose them into a coherent PR is to make each commit in the final PR independently meaningful, rather than what actually transpired during local development. TFA touches on this somewhat, contradicting the bit you quoted. A trivial example would be adding the core logic and associated tests in the first commit, and all the remaining scaffolding and ceremony…

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.

Re: The Theatre of Pull Requests and Code Review

#129

Post by candiddevmike »

WorldMaker wrote: post #118

I wish GitHub's PR UI was better at walking through a PR one commit at a time. As someone who does try to make good commits, and as someone who does try to read PRs sometimes a commit at a time, GitHub's UI gets in the way and keeps trying to drive you back to "whole PR" reviews. It is very telling in the article itself there is a screenshot of the commits tab in the PR workflow that many don't realize even exists an…

Can you describe a little more about why you walk through individual commits instead of just reviewing the latest commit only?

Re: The Theatre of Pull Requests and Code Review

#130

Post by g-b-r »

markus_zhang wrote: post #71

Story from friend but I can relate: Some teams and code are pretty much unreviewable and the best thing is to add CI for simple tests and lgtm if there is no glaring mistakes. My team only has 3 including the manager, so, eh, each one holds a lot of knowledge that only he or she knows. Documentation? Yeah that’s a good idea, but I don’t have time to read them because “We want to ship as fast as possible”. So I just p…

I mean, the reviewer is not supposed to know everything, right?

Uhm he kind of is, if you want a proper review...

Post reply on HN