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

131–140 of 431 posts

Re: The Theatre of Pull Requests and Code Review

#131

Post by maleldil »

danielfalbo wrote: post #78

Jane Street implements an awesome code review system: https://janestreet.com/tech-talks/janestreet-code-review > [...] Telling a Story with Commits [...] > [...] it should take the average reviewer 5-10 minutes [...] Jane Street code review system kinda solves this problem by - making each commit a branch, - stacking branches on top of each other (gracefully handling rebases and everything that comes with it), and -…

> handle the rebasing etc. manually myself for now

jj[1] is helpful for this. If you have a chain of commits like A -> B -> C, and you make a change to A, the rest of the chain is automatically rebased.

[1] https://jj-vcs.github.io/

Re: The Theatre of Pull Requests and Code Review

#132

Post by XorNot »

candiddevmike wrote: post #129
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?

Because the idea is that each commit that you submit is a coherent unit of work. That's why we have commit messages.

Re: The Theatre of Pull Requests and Code Review

#133

Post by [deleted] »

goosejuice wrote: post #106
ambicapter wrote: post #91

Earlier quoted context omitted.

> IMO many software developers are just not fast enough at writing or language I think this is the overwhelming factor, software engineering doesn't select for communication skills (and plenty of SEs will mock that as a field of study), or at least most SEs don't start out with them.

> SEs will mock that as a field of study Who are these people? I've never encountered that. In my experience engineers who aren't great at communication freely own up to it.

[deleted]

Re: The Theatre of Pull Requests and Code Review

#134

Post by frankc »

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 also feel like what gets lost in this is not everything you are building is a bite size feature in large existing project. Sometimes you are adding an entire subsystem that is large to something relatively greenfield. if you broke that down into features, you will need 20 PRs and if you wait for review, or even don't wait but have to circle back to integrate lots of requested changes, what might be a couple of weeks of work turns into 2 to 3 months of work. That just does not work unless you are in a massive enterprise that is ok with moving like molasses. Do you wind up with something not as high quality? Probably. But that is just the trade-off with shipping faster.

Re: The Theatre of Pull Requests and Code Review

#135

Post by [deleted] »

gorgoiler wrote: post #31
conartist6 wrote: post #29

If your goal is to lower the velocity of your organization, e.g. because in practice code churn or poor quality are major problems, then by all means do this. If you still need to move fast, then don't. This is the "don't run in the hallways" version of software culture, but I would contend that you should choose your pace based on your situation. It's just like gradient descent really. The be efficient sometimes you…

Your comment includes capital letters and punctuation! These are skills we all learn to ensure our writing is as legible as possible to the reader. Quotes, periods, commas, clauses, paragraphs etc. are all stylistic decisions that support the underlying text and message. I contend that learning the art of story telling through a stack of patches is just as important and, once learned, comes just as naturally as utili…

[deleted]

Re: The Theatre of Pull Requests and Code Review

#136

Post by disgruntledphd2 »

tcoff91 wrote: post #92
vinnymac wrote: post #70

Earlier quoted context omitted.

For any PR above a few line change, if a developer has not done a self review, I don’t review it all. Instead I request that it is self reviewed with context added, prior to requesting re-review. I also tend to ask the question, “are any of these insights worth adding as comments directly to the code?” 9/10 the context they wrote down should be well thought out comments in the code itself. People are incredibly lazy…

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've done this and strongly recommend.

Re: The Theatre of Pull Requests and Code Review

#137

Post by KronisLV »

> This makes logical sense, but it's challenging to implement because it can feel like admitting we're not smart enough to understand the code. However, saying "I don't understand this enough to approve it" is far more valuable than pretending with an empty "LGTM".

Sounds nice but I’m sure that there are projects out there that are like constantly being in the trenches, testing in prod and the original developers being long gone, where the devs manage to barely keep alive this WH40kesque monstrosity. Where all of the code has a lot of incidental complexity that will just never be resolved.

Alternatively, there’s probably countries and companies out there where that attitude would get you flagged as someone who is slowing down the velocity and get you fired - because there’s an oversaturation of devs in the local market and you’re largely an expendable cog.

Surely there’s other methods, formal or LLM driven that could be used to summarize changes and help explore everything bit by bit (especially when there’s just one big commit that says “fix” as a part of the PR).

Sometimes I get too caught up with coming up with contrived scenarios where “best effort” just will never happen but I bet someone out there is living that reality right now.

Re: The Theatre of Pull Requests and Code Review

#138

Post by ljosa »

manwe150 wrote: post #125
goosejuice wrote: post #103

Earlier quoted context omitted.

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.

`git commit` has a `-v` option that adds the diff to the bottom of the commit message template so you can see it while you write the message.
Post reply on HN