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

31–40 of 431 posts

Re: The Theatre of Pull Requests and Code Review

#31

Post by gorgoiler »

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 utilizing vocabulary, grammar, syntax and style with the written word.

Re: The Theatre of Pull Requests and Code Review

#32

Post by pm215 »

ekidd wrote: post #14

A lot depends on your goals for your code reviews. And your goals might even be different for different parts of the code base. - Are you trying to make sure that more than one human has seen the code? Then simply reading through a PR in 10 minutes and replying with either a LGTM or a polite version of WTF can be fine. This works if you have a team with good taste and a lot of cleanly isolated modules implementing cl…

I think the other thing that often muddies the waters in discussions of code review is that open source projects and internal codebases are generally in rather different situations. An internal codebase is usually worked on by a fairly small group of experienced people, who are both creating and also reviewing PRs for it. So:

- the baseline "can I assume this person knows what they're doing?" level is higher

- making the "create PR" process take longer in order to make the review process faster is only a tradeoff of the time within the team

- if something is wrong with the committed code, the person who wrote it is going to be around to fix it

But in open source projects, there are much more often contributions by people outside the "core" long-term development team, where:

- you can't make the same assumptions that the contributor is familiar with the codebase, so you need to give things extra scrutiny

- there are often many fewer people doing the code review than there are submitting changes, so a process that requires more effort from the submitter in order to make the reviewer's job easier makes sense

- if there's a problem with the code, there's no guarantee that the submitter will be available or interested in fixing it once it's got upstream, so it's more important to catch subtle problems up front

and these tend to mean that the code-review process is tilted more towards "make it easy for reviewers, even if that requires more work from the submitter".

Re: The Theatre of Pull Requests and Code Review

#33

Post by leoedin »

smashedtoatoms wrote: post #28
Supermancho wrote: post #23

> Story-Telling Commit Messages No thank you. Talking to future ME, I don't need to know how I got to what I want me to look at. A squashed ticket-by-ticket set of merges is enough for me.

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't discuss the one line that I care about. Perhaps the only exception would be one line changes - perhaps a change which changes a single configuration value alongside a comment saying "Change X to n for y reason".

Comments can be a bit better - but they have the nasty habit of becoming stale.

Re: The Theatre of Pull Requests and Code Review

#34

Post by stillworks »

epage wrote: post #10

Agree with the overall sentiment but disagree with > A good rule of thumb is 300 lines of code changes - once you get above 500 lines, you're entering unreviewable territory. I've found LoC doesn't matter when you split up commits like they suggest. What does matter is how controversial a change is. A PR should ideally have one part at most that generates a lot of discussion. The PR that does this should ideally also…

In some parts of the industry number of CRs and revisions-per-cr is tracked as a performance metric.

Many people learn to game this to make their "numbers" appear good i.e. high number of CRs and low revisions per CR.

Re: The Theatre of Pull Requests and Code Review

#35

Post by StilesCrisis »

SOLAR_FIELDS wrote: post #25
epage wrote: post #10

Agree with the overall sentiment but disagree with > A good rule of thumb is 300 lines of code changes - once you get above 500 lines, you're entering unreviewable territory. I've found LoC doesn't matter when you split up commits like they suggest. What does matter is how controversial a change is. A PR should ideally have one part at most that generates a lot of discussion. The PR that does this should ideally also…

Good luck getting 90% of devs to commit (har har) to this level of history surgery. Of the ones that actually know how to do it (a small fraction of your typical engineer) an even smaller fraction of that is going to have the patience to do it correctly. You’ll tell devs to do this kind of thing and you’ll either have their eyes glaze over from lack of understanding, annoyance at the extra work, or nodding then apath…

You also need tools that support the workflow. I love small self-explanatory commits. In git, it's easy to do. Recently I switched to a Perforce organization and it's a disaster. P4 doesn't support stacked CLs and it dramatically hurts engineering quality. Everyone lands mega-CLs because it's the only supported workflow.

Re: The Theatre of Pull Requests and Code Review

#36

Post by StilesCrisis »

fidotron wrote: post #9
ValleZ wrote: post #3

If you split all the changes for a feature this way not only you hide the way all changes interact with each other but also make the development at least 10x longer because an average approval time is often more than a day.

This will be accompanied by the sort of dev manager that thinks a KPI for "number of PRs merged" won't in any way be gamed or backfire. I don't know what they're doing where you can do code reviews in 5-10 minutes, but in my decades doing this that only works for absolutely trivial changes.

The goal here is to make almost all CLs trivial enough that they can be reviewed quickly. You can compose almost any feature out of many small simple changes.

Re: The Theatre of Pull Requests and Code Review

#37

Post by gjgtcbkj »

Every developer I know who applies this sort of “highly documented development” approach where they “work through their thought process openly.” Is only doing it because their thought processes are already so funky and counterintuitive that reviewers actively reject their work unless have written evidence that the developer didn’t just entirely change the scope of their assignment to justify the bizarre decisions.

Re: The Theatre of Pull Requests and Code Review

#38

Post by osigurdson »

Unless you have a broader context, reviewing 300 line PRs in 5 minutes is going to be surface level at best. Plus that time comes with an expensive context switch so the actual cost is likely more like 20 - 30 minutes. At this point, I think a reasonable question is why not just use AI for shallow reviews like this? This would free up bandwidth in situations where you really want code reviewed by someone else.

Re: The Theatre of Pull Requests and Code Review

#39

Post by ziml77 »

smashedtoatoms wrote: post #28
Supermancho wrote: post #23

> Story-Telling Commit Messages No thank you. Talking to future ME, I don't need to know how I got to what I want me to look at. A squashed ticket-by-ticket set of merges is enough for me.

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 a reviewer, I don't care how you got to the end result. I want to see the final code. If you settled on something in your code that was unintuitive, because you tried simpler ideas that didn't work, then note that in a comment. Comments provide the info inline and don't require someone reviewing the code now or working on the code 15 years later to read your "commit story" to understand it.

Re: The Theatre of Pull Requests and Code Review

#40

Post by whstl »

rustybolt wrote: post #17
nenenejej wrote: post #8

300 LOC in 10 minutes. Or 2 sec per loc. Or for average 30 char line, 600wpm reading speed. OK. There is little you can review properly in 10 minutes unless you were already pairing on it. You might have time to look for really bad production-breaking red flags maybe. Remember the underlying reasons for PR. Balance between get shit done and operational, quality and tech debt concerns. Depending on what your team need…

Agreed, 300 lines will take me a lot more than 10 minutes to review properly! Depends on the specific changes of course, but generally speaking.

Also depends on the codebase.

300 lines is nothing in some boilerplate-heavy codebases I've worked at.

After seeing the same patterns for the hundredth time, it's easy to detect deviations. Not to mention linters and typing helps a lot too.

Not a fan of those but oh well.

Post reply on HN