The Theatre of Pull Requests and Code Review

https://meks.quest/blogs/the-theatre-of-pull-requests-and-code-review

by todsacerdoti • 10 months ago

257 416 10 months ago

kaapipo• 10 months ago

I mean, stacked PRs are a thing for a reason

keriati1• 10 months ago

I can also recommend rather to use the stacked PR approach. We have it since years, PR review "issues" are not a thing for us.

I still encourage do to a lot of small commits with good commit messages, but don't submit more then 2-3 or 4 commits in a single PR...

epage4• 10 months ago

I see stacked PRs as independent of this. PRs are a good unit of cohesion of changes, particularly changes that only make sense if later changes are also merged.

kiitos12• 10 months ago

stacked PRs are wildly reviewer-hostile, please do not do them

ValleZ• 10 months ago

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.

fidotron4• 10 months ago

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.

StilesCrisis4• 10 months ago

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.

viraptor2• 10 months ago

only you hide the way all changes interact with each

Splitting the change does not prevent you from looking at diffs of any combination of those commits. (Including whole pr) You're not losing anything.

at least 10x longer because an average approval time is often more than a day.

Why do you think it would take longer to review? Got any evidence?

eigencoder• 10 months ago

I think approval time would take much longer. The issue is that while actual time spent in review may be shorter, there's a lot of context-switching time costs that increase with the number of PRs submitted.

No one on the team is just sitting there refreshing the list of PRs, ready to pick one up immediately. There's a delay between when the PR is marked as ready and when someone can actually get to it. Everyone is trying to get work done and have some time daily in flow state.

Imagine you have a change; you could do it as one PR that takes 1 hour to review, or 3 small PRs that each take 15 mins to review. The time spent in review may even be shorter for the small PRs, but if each PR has a delay of 1 hour before a reviewer can get to it, then the 3 PRs will take almost 4 hours before they're done, as opposed to just 2 hours for one big PR.

jvanderbot• 10 months ago

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 interpretation" style PR just seems like it's doomed to become lower priority for busy folks. You can add a fancy narrative to the comments if you like, but the root problem is that presenting a change block as-is and asking for a boolean blanket approval of the whole thing is an expensive request.

There's no substitute for shared understanding of the context and intent, and that should come out-of-band of the code in question.

pards• 10 months ago

and to ignore my PRs when I don't

PRs should be optional, IMHO. Not all changes require peer review, and if we trust our colleagues then we should allow them to merge their branch without wasting time with performative PRs.

fidotron4• 10 months ago

There is a difference between a code review and approval to merge/release.

Part of the difference is the idea you can catch all problems with piecemeal code review is nonsense, so you should have at least some sweeping QA somewhere.

flir• 10 months ago

I always appreciate an extra pair of eyeballs, even on a one-liner. Everyone's an idiot sometimes.

comprev• 10 months ago

Trust, but verify. We're only human after all :-)

At $DAY_JOB we need approvals from peers due to industry regulation.

gagabity2• 10 months ago

Yes! I once read a great article I can no longer find that talked about 3 types of PRs. Simple ones that you self approve. Ones that you tag someone because you want to spread the knowledge of what has been done. And ones that need actual review. Everything being reviewed is simply unnecessary and exhausting.

t-writescode5• 10 months ago

SOX compliance audit looks suspiciously at this comment.

No single person being able to make changes to a system is a tenant of that.

balamatom• 10 months ago

that should come out-of-band of the code in question.

Ideally, yes. After a decade and something' under ZIRP, seems a lot of workers never had incentive to remain conscious of their intents in context long enough to conduct productive discourse about them. Half of the people I've worked with would feel slighted not by the bitterness the previous sentence, but by its length.

There's an impedance mismatch between what actually works, and what we're expected to expect to work. That's another immediate observation which people to painfully syntaxerror much more frequently than it causes them to actually clarify context and intent.

In that case the codebase remains the canonical representation of the context and intent of all contributors, even when they're not at their best, and honestly what's so bad about that? Maybe communicating them in-band instead of out-of-band might be a degraded experience. But when out-of-band can't be established, what else is there to do?

I'd be happy to see tools that facilitate this sort of communication through code. GitHub for example is in perfect position to do something like that and they don't. Git + PRs + Projects implement the exact opposite information flow, the one whose failure modes people these days do whole conference talks about.

xmcqdpt22• 10 months ago

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, why it required a change to this part of the codebase, and the thought process behind the change. Sometimes but not often enough I send the review back and ask them for this info.

IMO many software developers are just not fast enough at writing or language so providing an explanation for their changes is a lot of work for them. Or they are checked out and they just followed the AI or IDE until things worked, so they don't have explanations to provide. People not taking the time is what makes reviews performative.

dijksterhuis• 10 months ago

Minimally, I would like context for the change, …

… the why is important

IMO many software developers … don't have explanations to provide. People not taking the time is what makes reviews performative.

… a lot of developers only consider the how.

i’ve had a lot of experiences of “once my PR is submitted that’s my work/ticket finished” kind of attitude.

i spent a year mentoring some people in a gaming community to become dev team members. one of the first things i said about PRs was — a new PR is just your first draft, there is still more work to do.

it helped these folks were brand spanking new to development and weren’t sullied by some lazy teacher somewhere.

ambicapter• 10 months ago

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.

scuff3d4• 10 months ago

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.

sardines• 10 months ago

My team uses a github PR template with the following sections. Answers to each can be short yet it has been extraordinarily helpful to pass over important info to the reviewer that's not captured in code. It also borders on "checklist" that the code author has actually done the bare minimum to think things through.

# Goal (why is this change needed at all)

# What I changed and why I did it this way

# What I'm not doing here and how I'll follow up

# How I know it works (optional section, I include this only for teams with lots of very junior engs: "added a test" is often sufficient)

vinnymac2• 10 months ago

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 sometimes, even unintentionally so. We need better lint tools to help the monkey brain perform better. I wish git platforms offered more UX friendly ways to force this kind of compliance. You can kind of fake it with CI/CD but that’s not good enough imo.

tcoff917• 10 months ago

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.

atoav• 10 months ago

Exactly, if you want people to think about your code/changes you should be able to give them the needed context.

If you don't know them, please realize your code isn't automatically a gift everybody waited for, you may see it that way, but from the other side this isn't clear until someone put in the work to figure out what you did.

In short: added code produces work. So the least you should do is try reducing that work by making it easy to figure out what your code is and isn't.

Sum up what changes you made (functionally), why you made them, which choices you made (if any) and why and what the state of the PR code is in your own opinion. Maybe a reasoning why it is needed, what future maintenance may look like (ideally low). In essence, ask yourself what you'd like to know if someone appeared at the door and gave you a thumb drive with a patch for your project and add that knowledge.

Also consider to add a draft PR for bigger features early on. This way you can avoid programming things that nobody wanted, or someone else was already working on. You also give maintainers a way to steer the direction and/or decline before you put in the work.

dakiol• 10 months ago

There’s a fine difference between a) splitting a big feature in parts that interact with each other via well defined interfaces, and b) splitting a big feature in parts that are suitable for PRs

You can split a big feature in N MRs and that doesn’t necessarily mean the N MRs are easier to understand (and so to review) than a single big MR. Taking into account the skills of the average software engineer, I prefer to review a single big MR than N different and not very well connected MRs (the classic example is that MR number N looks good and innocent, but then MR number N+1 looks awful… but since MR number N was already approved and merged the incentives are not there to redo the work)

nenenejej• 10 months ago

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 needs you can choose anything from no review at all to 3x time reviewing than coding. What is right depends on your situation. PR is a tool.

rustybolt• 10 months ago

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.

whstl2• 10 months ago

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.

enlyth• 10 months ago

I like to actually checkout the branch I'm reviewing and run the code myself to observe if it does what is claimed, that usually takes up at least 10 minutes in itself, sometimes more.

From my experience most of the issues I find are actually from this type of observation rather than actually reading the code and imagining what it does in my head.

qiine• 10 months ago

the mind remains a poor compiler ;p

franktankbank• 10 months ago

Your linter/tests are for catching real errors. Review is to understand the shape of it mostly IMO. I could probably fairly easily review 300 loc if its not a particularly confused shape.

tcoff917• 10 months ago

It all depends on the lines. It can take a long time to review a 1 line change to a critical function that is used everywhere in the app and it can take minutes to review 1000 lines of declarative UI code.

epage4• 10 months ago

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 have the minimal number of commits (just what dossn't make sense standalone). Granted this take experience generally and experience with your reviewers which is where metrics like LoC counts can come in handy.

padjo• 10 months ago

It’s often an inverse correlation in well functioning environments. Controversial changes and bug fixes are small and targeted and are deeply reviewed for unintended side effects, while large change sets are often new work that’s been discussed upfront and follows well established patterns and gets waved through.

SOLAR_FIELDS• 10 months ago

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 apathetical disregard. The one top engineer will do it then be frustrated that the rest of the org doesn’t do proper commit hygiene.

It’s not really something you can easily enforce with automation, so basically unachievable unless you are like Netflix and only hiring top performers. And you aren’t like Netflix.

StilesCrisis4• 10 months ago

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.

stillworks2• 10 months ago

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.

osigurdson9• 10 months ago

Easy and fun to put metrics around random things. I'll start to squirm if you ask me to draw the connection between these metrics and NPV.

rjmunro• 10 months ago

Another tip: Use `git log --first-parent` and `git log --merges` to hide the intermediate commits. `--first-parent` also works with `blame` in modern git. These mean you don't have to look at all the small commits when browsing history, only when you want to dive in deeper.

btreecat• 10 months ago

My guide to good PRs:

- Keep PR messages short and to the point. - use as many commits as you need, it's all the same branch. Squash if you want, I think it hides valuable meta. - put the ticket in the branch name. Non negotiable. - Update your ticket with progres. Put as much details as you can, as if you were writing to someone who's picking up the task after you. - add links to your ticket. Docs, readme, o11y graphs, etc. - Link ticket in PR for easy access to additional info - Admit if you don't understand what your looking at. Better to pair and keep moving forward. - if you request changes, stay available for conversation and approval for the next few hours. - punt the review if you don't feel like you can legitimately engage with the content right now. Make sure you communicate it though. - Avoid nits. This causes a loss in momentum and v rarely is worth changing.

ekidd• 10 months ago

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 clear APIs. The worst damage is that one module might occasionally be a bit marginal, but that can be an acceptable tradeoff in large projects.

- Does every single change need to be thoroughly discussed? Then you may want up-front design discussions, pairing, illustrated design docs, and extremely close reviews (not just of the diffs, but also re-reviewing the entire module with the changes in context). You may even want the PR author to present their code and walk throuh it with one or more people. This can be appropriate for the key system "core" that shapes everything else in the system.

- Is half your code written by an AI that doesn't understand the big picture, that doesn't really understand large-scale maintainability, and that cuts corners and _knowingly_ violates your written policy and best practices? Then honestly you're probably headed for tech debt hell on the express train unless your team is willing to watch the AI like hawks. Even one clueless person allowing the AI to spew subtlety broken code could create a mess that no number of reviewers could easily undo. In which case, uh, maybe keep everything under 5,000 lines and burn it all down regularly, or something?

pm215• 10 months ago

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

bonzini3• 10 months ago

- 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

It's also more important to have good tools to analyze subtle problems down the line, thus increasing the importance of bisection and good commit messages.

An underrated benefit of "make it easy for reviewers" is that when a bug is found, everybody becomes a potential reviewer. Thus the benefit does not finish when the PR is merged.

surgical_fire2• 10 months ago

Can't relate. I take code reviews as possibly the most important part of my job as a developer. Suggesting extra tests, thinking about unintended side effects, and yes, aiming for consistency and readability, without being picky on style choices.

I trust my colleagues to do the same (and they often do).

I can't imagine working in an environment where this is a theater.

gjgtcbkj3• 10 months ago

It sounds like a good job where the most important part is finding other people’s mistakes.

Though I do appreciate the shoutout to adding tests in CR. But returning a PR solely because it doesn’t have tests, is effective, but a little performative too. It kind of like publicly executing someone, theirs gotta be some performance for it to be a deterrent. If something doesn’t have tests my review is going to be a very short performance where I pretend read the rest of the code. Then immediately send it back.

surgical_fire2• 10 months ago

It sounds like a good job where the most important part is finding other people’s mistakes.

And reviews are not that. Systems are complex, and having a mental model of complex systems is difficult. Everyone has blind spots. A fresh pair of eyes can often spot what who was coding would not.

But returning a PR solely because it doesn’t have tests, is effective, but a little performative too.

And this is not what I said. I spoke of suggesting extra tests. A scenario that wasn't covered, for example.

roguecoder13• 10 months ago

It's interesting: the original definition of "performative" is "a speech-act that changes something about the world".

And that basically describes all of programming: we are building metaphors that will run electricity at a higher or lower voltage, and be translated again into something meaningful to a different human.

In many ways, all we are doing is performing. And that is some of what makes this job challenging: the practices that build software well are all just ways of checking how humans will interact with the ones and zeros we've encoded.

Returning a PR because it doesn't have tests means that code will have automated validation, which is a real change. It also means the code will be written in a testable way: too often we don't realize we wrote code in a way that is hard to test unless we try to write the tests. And on a larger level, it means that this team of engineers will learn and use the practices and tools that lead to testable code and effective tests and more easily-changeable code.

It makes total sense to not keep reading if there aren't tests, because adding the tests can be expected to change the code. But just because that is a performance doesn't mean it doesn't profoundly change the world.

kiitos12• 10 months ago

i think you would benefit from some time in an organization that was not shitty

roguecoder13• 10 months ago

A lot of developers are working in hellish companies, micromanaged and beaten down into cogs.

One very valuable skill for those of us who have experienced productive collaboration is learning how to introduce it to new places.

Sometimes that means telling the executives that their promotion process is making them less successful. Sometimes it means wandering around PRs leaving useful positive comments to seed the idea that PRs can be useful. Sometimes it means pointing out tests only when there is a bug, so people can experience how great it is to follow the practices that keep us from introducing bugs in the first place.

I wish that more CS programs would explicitly teach their students critique skills, the way art and music and english and math programs do. But until then, we're counting on engineers getting lucky and landing in a functional workplace like yours.

2sk21• 10 months ago

Reviewing someone else's large pull request is like having a second task in parallel with what you are working on yourself!

chrisweekly• 10 months ago

It's not "like" another task, it IS another task!

osigurdson9• 10 months ago

Yeah but it is just a quick look, "yep", "yep", "oh what about this"?, "wow we dodged a bullet there". Its like self managed error correction that the collective does on its own. Fast, simple and produces good results. The less software you write the more this resonates.

eterm2• 10 months ago

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

sexyman484• 10 months ago

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.

Supermancho2• 10 months ago

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.

smashedtoatoms• 10 months ago

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.

leoedin• 10 months ago

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.

ziml772• 10 months ago

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.

whstl2• 10 months ago

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.

Supermancho2• 10 months ago

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 can't win with HN critics. If I talk about someone else looking, then I'm assuming. If I talk about myself, then I'm being too self-centered (in the oblique sense you reference). I am very aware of how this works across teams of people, not just myself, since I'm in industry.

vdupras• 10 months ago

A while ago at a past job, I was working on OpenERP (now called Odoo I think). This "community" had instated this kind of "mandatory community review" policy so that each change had to be reviewed by X developers from other organizations. I kind of virtuous web of review.

But the thing is: this code is terrible and huge chunks of it are a unholy mix and match of code written for very specific purpose for this or that client, with this very weird "falsely generalized" code. I don't know how to call that: you have some very specific code, but you insert useless and probably buggy indirections everywhere so that it can be considered "general". The worst kind of code.

Anyways, I was asked by my boss to do such a review. I look at it and I realize that building a database setup to be able to properly run that code is going to take me weeks because I'm going to have to familiarize myself with tons and tons of modules I don't know about.

So I write down my estimate in our tracker: 1 month.

He comes back to me, alarmed. A whole month? Well yeah, otherwise I can't even run the code.

All you have to do is look at the code! What? No way, that ain't a review. Well, I ask you to do it as such. I'm not writing LGTM there.

So I was never asked to do reviews there again (in fact, I stopped working on OpenERP at all), but I could see "LGTM" popping up from my colleagues. By the way, on OpenERP tracker, all you ever saw in review logs was "LGTM" and minor style suggestions. Nothing else. What a farce.

So yeah, as the article says, there are some "LGTM-inducing" type of PRs, but the core of the problem is developers accepting to do this "LGTM-stamping" in the first place. Without them, there would only be reviewable PRs.

conartist62• 10 months ago

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 need to make big hops, sometimes small ones.

gorgoiler• 10 months ago

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.

roguecoder13• 10 months ago

Changing cowboy code is much, much slower than changing good code.

If you need to move fast for the next two weeks, sure. If you need to move fast for the next year, you are better off collaborating.

OsrsNeedsf2P• 10 months ago

You're making the faulty assumption the whole project won't get scrapped

conartist62• 10 months ago

Yeah, cowboy coding as a strategy makes most sense if it's very likely that the next person to look at the code will be you.

I'm not advocating either way as superior, but cowboy coding shouldn't mean that you don't pay your tech debt. It just means that it's much more common to roll a bug fix or small factoring improvement in with a feature, probably because you were already touching that code and testing it.

If prod bugs are so critical that there will be a rollback and a forensic retrospective on each one, then yeah you should bite the bullet and use all the most defensive PR tactics. If prod bugs have small costs and you can quickly "roll forwards" (ship fixes) then it's better to get some free QA from your users, who probably won't mind the occasional rough edge if they're confident that overall quality is OK and bugs they do find won't stay unfixed for years.

octo888• 10 months ago

What I've often found is that people only really accept feedback from the Tech Lead, and peers are dismissed (not outright and not obviously - kind of sealioning etc). Peer-to-peer code reviews are another instance implementing a thing that pretends hierarchy does not exist.

You can only get basic tweaks accepted. The sunk-cost fallacy is a huge force.

Maybe I've only worked at crappy places

gjgtcbkj3• 10 months ago

Allen Iverson got criticized by the media for “letting down his teammates” for skipping a few practices. He famously said “We’re talking about practice, practice! not the game! How is MY going to practice gonna make THEM better?!” He got flack for those comments, but what he said was accurate. If we’re talking about outcomes you’re beholden to the person who is seen as the difference maker, all the teamwork in the world isn’t going to improve an individual’s the lack of ability.

sexyman484• 10 months ago

Every company depends on 3-5 guys for their competitive advantage. While the other thousand guys are interchangeable, they're just as necessary (and needful) to the company's survival.

roguecoder13• 10 months ago

For an alternative perspective, I recommend the business book The Captain Class.

If you aren't making your teammates better, and they aren't making you better, you will never be able to be as good as a team that is greater than the sum of its parts. Individual genius is consistently beat by professional collaboration.

roguecoder13• 10 months ago

That is a performance issue, for sure, and those developers are throwing away some of the best feedback they can get.

A decent programmer can write code they can effectively work with. The really great programmers write code that even interns can effectively work with. The only way to get to that level is to get good at eliciting and taking in feedback from the people we want to be effective with our code.

It doesn't necessarily mean doing exactly what we are told: it means understanding the why of a comment, what underlying flaws or confusions a comment is pointing towards. It means encouraging people to ask questions in code reviews, rather than just leave commands: often a "how does this manage to do X?" comment points to a place where bugs were hiding anyway, even if also it is a chance to share a language feature.

Many engineers work in companies where being a really great programmer doesn't get you any points. Often the only reward for writing code that is easily modified later is the gratitude of a future developer asked to make a change to it years down the line.

But I am that developer often enough that that's still made the journey worth it for me.

gjgtcbkj3• 10 months ago

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.

g-b-r12• 10 months ago

You only know poor developers then, I guess

watwut2• 10 months ago

That is 100% not my experience. People who do that tend to be perfectionists trying to cross all the ts.

Confused developers are just unable to create such reasoning.

osigurdson9• 10 months ago

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.

roguecoder13• 10 months ago

One of the advantages of small, frequent code reviews is that a team shares the broader context.

Which is far more valuable than ten minutes of extra typing.

osigurdson9• 10 months ago

There are better ways to get broader context. Such as getting people to work in other areas of the code. A 5 minute review isn't going to do much in this regard.

davedx• 10 months ago

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 of code, splitting that up across multiple PR's will result in:

- A big queue of PR's for reviewers to review

- The of the feature is split across multiple change sets, increasing cognitive load (coherence is lost)

- 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

The right answer for the size of a PR is NOT in lines of code. Exercise judgement as to what is logically easier to review. Sometimes bigger is actually better, it depends. Learn from experience, communicate with each other, try to be kind when reviewing and don't block things up unnecessarily.

CuriouslyC2• 10 months ago

100%. I think the right answer is to break features into atomic commits, but keep PRs at the feature level. This reduces the PR friction, while letting reviewers easily view change sets for specific features, and if a specific feature needs a patch you don't need to do any rebase gymnastics, just add a patch commit.

AI agents like frequent checkpoints because the git diff is like a form of working memory for a task, and it makes it easy to revert bad approaches. Agents can do this automatically so there isn't much of an excuse not to do it, but it does require some organization of work before prompting.

yunohn• 10 months ago

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

+100 to this. My job should be thoughtfully building the solution, not playing around with git rebase for hours.

tcoff917• 10 months ago

Just use jj instead of git and cut your rebasing time by 95%.

Suddenly rebasing a stack of branches becomes 1 command.

sebk• 10 months ago

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 in subsequent commits. I find this technique especially useful when an otherwise additive change requires refactoring of existing code, since the things I expect will be reviewed in each and the expertise it takes are often very different.

I don't mind squashing the branch before merging after the PR has been approved. The individual commits are only meaningful in the context of the review, but the PR is the unit that I care about preserving in git history.

scuff3d4• 10 months ago

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.

vault_• 10 months ago

- A big queue of PR's for reviewers to review

This is a feature. I would infinitely prefer 12 PRs that each take 5 minutes to review than 1 PR that takes an hour. Finding a few 5-15 minute chunks of time to make progress on the queue is much easier than finding an uninterrupted hour where it can be my primary focus.

- The of the feature is split across multiple change sets, increasing cognitive load (coherence is lost)

It increases it a little bit, sure, but it also helps keep things focused. Reviewing, for example, a refactor plus a new feature enabled by that refactor in a single PR typically results in worse reviews of either part. And good tooling also helps. This style of code review needs PRs tied together in some way to keep track of the series. If I'm reading a PR and think "why are they doing it like this" I can always peek a couple PRs ahead and get an answer.

- 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 is a tooling problem. Git and Github are especially bad in this regard. Something like Graphite, Jujutsu, Sapling, git-branchless, or any VCS that supports stacks makes this essentially a non-issue.

kiitos12• 10 months ago

code review isn't about diffs, it's about holistic changes to the project

the point is not queue progression, it is about dissemination of knowledge

one holistic change to a project = one PR

simple stuff really

computronus• 10 months ago

I do agree with the common refrain, actually, and disagree with the idea that work can be so big and complex that it has to be in one pull request.

A big queue of PR's for reviewers to review

Yes, yes please. When each one is small and understandable, reviewers better understand the changes, so quality goes up. Also, when priorities change and the team has to work on something else, they can stop in the middle, and at least some of the benefits from the changes have been merged.

The PR train doesn't need to be dumped out in one go. It can come one at a time, each one with context around why it's there and where it fits into the grander plan.

The [totality] of the feature is split across multiple change sets, increasing cognitive load (coherence is lost)

A primary goal of code review is to build up the mental map of the feature in the reviewers' brains. I argue it's better for that to be constructed over time, piece by piece. The immediate cognitive load for each pull request is lower, and over time, the brain makes the connections to understand the bigger picture.

They'll rarely achieve the same understanding of the feature that you have, you who created and built it. This is whether they get the whole shebang at once or piecemeal. That's OK, though. Review is about reducing risk, not eliminating it.

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

I've learned not to charge too far ahead with feature work, because it does get harder to manage the farther you venture from the trunk. You will get conflicts. Saving up all the changes into one big hunk doesn't fix that.

A big benefit of trunk-based development, though, is that you're frequently merging back into the mainline, so all these problems shrink down. The way to do that is with lots of small changes.

One last thing: It is definitely more work, for you as the author, to split up a large set of changes into reviewable pieces. It is absolutely worth it, though. You get better quality reviews; you buy the ability to deprioritize at any time and come back later; most importantly for me, you grasp more about what you made during the effort. If you struggle to break up a big set of changes into pieces that others can understand, there's a good chance it has deeper problems, and you'll want to work those out before presenting them to your coworkers.

tcoff917• 10 months ago

A stack of PRs is much better for reviewers than a single massive PR.

Use jujutsu and then stacking branches is a breeze

nerdponx• 10 months ago

Here's an alternative approach: Discuss the design with your team beforehand, and have active ongoing discussions, sanity checks, and even pair programming during the development process. That way the review is not an exhaustive end-to-end review with the reviewer coming in cold. It's instead the final approval step in a long chain of decisions that have already been discussed and agreed upon.

Of course that won't work for all projects/teams/organizations. But I've found that it works pretty well in the kinds of projects/teams/organizations I've personally been a part of and contributed to.

scuff3d4• 10 months ago

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.

frankc• 10 months ago

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.

roguecoder13• 10 months ago

If you are the only developer who ever going to work on something, maybe. Even then, I will argue you are more likely to deliver successfully if you are cutting your work into smaller pieces instead of not delivering anything at all for weeks at a time.

But for the company, having two people capable of working on a system is better than one, and usually you want a team. Which means the code needs to be something your coworkers understand, can read and agree with. Those changes they ask for aren't frivolous: they are an important part of building software collaboratively. And it shouldn't be that much feedback forever: after you have the conversation and you understand and agree with their feedback, the next time you can take that consideration into account when you are first writing the code.

If you want to speed that process up, you can start by pair programming and hashing out disagreements in real time, until you get confident you are mostly on the same page.

Professional programming isn't about closing tickets as fast as possible. It is about delivering business value as a team.

roguecoder13• 10 months ago

It's not about splitting up the PR: it is about splitting up the _work_.

If you don't have feature flags, that is step one. Even if you don't have a framework, you can use a Strategy or a configuration parameter to enable/disable the new feature, and still have automated testing with and without your changes.

ricardobeat• 10 months ago

Keep merging each PR into master under a feature flag, that's how it's done. Huge PRs that implement a feature in one swoop are pretty much the worst case scenario for every stage: review, testing, deployment and monitoring.

osigurdson9• 10 months ago

The PR type approach comes from the Linux kernel where there is essentially a hierarchy of gatekeepers with increasing trust and responsibility. It is a very individualistic type approach (i.e. if you merged it into your branch, it's on you and speed is a non-goal for the most part). This is often different from many software projects as it is like a collective where often, no one really has individual responsibility for anything (it is more like collective responsibility), speed is everything and there are certainly no gate keepers - maybe the review actually doesn't matter that much in this context.

g-b-r12• 10 months ago

If security or bugs don't matter much for your project, sure

osigurdson9• 10 months ago

I don't think "LGTM" helps with either of those. Making someone personally responsible is the real unlock here.

mtlynch2• 10 months ago

I love code reviews and blog posts about them, but I vehemently disagree with all of this advice.

His example PR[0] adds just 152 lines of code, removes 2 lines, but uses 13 thoughtful commits.
While some developers might understand those 152 lines from the final diff alone, I couldn't confidently approve it without the commit story.

This is ridiculous!

You absolutely can and should review a PR without demanding its "commit story."

Go read the PR under discussion here.[0] There's nothing about it that's hard to understand or that demands you go read the 13 intermediate steps the developer took to get there.

The unit of change in a code review is the PR itself, not the intermediate commits. Intermediate commits are for the author's benefit, not the reviewer's. If the author rewrote the code in FORTRAN to help them understand the problem, then converted it back to the codebase's language, that's 100% okay and is not something the reviewer needs to care about.

The PR should squash the individual PRs at merge time. The linked PR[0] is a perfect example, as the relevant change in the permanent commit history should be "Measure average scheduler utilization" and not "Collect samples" or "Mock sampling."

When you need to communicate extra context outside of the code, that should go in the PR description.[1] Your reviewer shouldn't have to go scour dozens of separate commit messages to understand your change.

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.

5-10 minute reviews are so low that it's basically thoughtless rubber-stamping.

If someone spent 5-10 hours making a change, the reviewer should definitely think about it for more than 5 minutes. If all the reviewer is doing is spot checking for obvious bugs, it's a waste of a code review. The reviewer should be looking for opportunities to make the code simpler, clearer, or more maintainable. 5-10 minutes is barely enough time to even understand the change. It's not enough time to think deeply about ways to improve it.

[0] https://github.com/sasa1977/hamlet/pull/3

[1] https://refactoringenglish.com/chapters/commit-messages/

tantalor• 10 months ago

Hard agree.

Commits are not important. As an author, you should not waste your time on this. As a reviewer, just ignore them.

g-b-r12• 10 months ago

Intermediate commits are for the author's benefit, not the reviewer's.

I don't even know how could commits only benefit the author; if they're poor they won't help him either, if not as a log of how much work he's done.

Unless you make a PR for every insignificant change, PRs will most often be composed of series of changes; the individual commits, if crafted carefully, will let you review every step of the work of the author quickly.

And if you don't eschew merges, with commits you can also group series of related modifications.

mtlynch2• 10 months ago

I don't even know how could commits only benefit the author; if they're poor they won't help him either, if not as a log of how much work he's done.

Intermediate commits are just checkpoints of unfinished code. The author knows that they made them and can revert back to them or use git log --pickaxe-S if there's code they saved to a checkpoint and want to recover.

Intermediate commits can have meaningful commit messages if the author chooses, but they could also just be labeled "wip" and still be useful to the author.

It's really easy for a note someone writes to themselves to be useful to that person without being useful to other people. If I write a post-it on my desk that says "beef," that can remind me I need to pick up beef from the store, even though if my co-worker reads it, they wouldn't know what the note is trying to communicate.

PRs will most often be composed of series of changes; the individual commits, if crafted carefully, will let you review every step of the work of the author quickly.

I don't understand this expectation that an author produce this.

What if the author experimented with a lot of approaches that turned out to be dead ends? Is it a good use of the reviewer's time to review all the failed attempts? Or is the author supposed to throw those away and reconstruct an imaginary commit history that looks like their clean, tidy thought process?

If someone sends me a short story they wrote and asks for feedback, I can give them feedback without also demanding to see every prior draft and an explanation for every change they made until it reached the version I'm reviewing.

The unit of change for a code review is a PR. The intermediate commits don't matter to the reviewer. The unit of change for a short story is the story. The previous drafts don't matter to the reader.

stack_framer• 10 months ago

My manager recently told our team that "AI usage" would be added to our engineering competencies, and we would all be expected to "use AI more."

When I said my top preference for AI usage, by far, would be to eliminate human code reviews, the response was basically, "Oh, not like that."

CuriouslyC2• 10 months ago

Don't worry, if you actually increase AI usage your team will be forced to automate code review, either explicitly or implicitly.

BowBun2• 10 months ago

That's a bummer! At my company we've started investing in what I'm calling 'semantic linting', which is basically running GPT over a PR with a set of rules that we iterate on. Already I'm finding huge value for style/pattern comments that linters can't easily catch, dropping warnings for common DB migration footguns, or notifying people of changing patterns/new ways of doing things. Been great so far!

reitanuki• 10 months ago

Do you have any write-up about this or more info? It sounds like a useful use case but I haven't yet got it right

Entelligence25• 10 months ago

Rn 70% of ai is being used only for code review documentation purpose, i think we really need entire engg workflows to be ai automated for better team insights, better team performace , sprint assesment etc and track entire engg lifecycle.

Vision should not be AI code, but it should be AI beyond code.

karel-3d• 10 months ago

If you want to let the review in, you need to make some obvious issues - typo or space/tab thing - so you give the reviewer some bone so they feel like they did something and accept your request otherwise.

patrickmay• 10 months ago

This is the drawback of teaching some software developers social skills.

bonzini3• 10 months ago

Kinda like "the queen's duck" (https://bwiggs.com/notebook/queens-duck/).

iamleppert• 10 months ago

I've tried his advice several times and it's been a complete failure. Splitting up a PR into a bunch of little PR's causes more problems than it solves, and it makes it 10x harder for the reviewer, no matter how much they complain about long PR's. Now they need to suss out some kind of ordering of the PR's, and navigate between multiple change sets for changes that depend on one another. It doesn't matter how well you can isolate the "concerns" a frontend depends on backend API, etc.

You end up creating more work for the reviewer, and most people just simply won't do the work of a proper review. You also don't have the advantage of any CI or tests running across the entire set of changes, so you also have separate CI reports to review. All this adds up for more places for bugs to hide or happen. All the same risks are still there, and you've also added a few more points of failure in the splitting process.

And for what? To end up, most likely merging in one PR after the next, for a feature that should just be all logically grouped together, or just squashing and merging the PR's together anyway.

jghn• 10 months ago

This. I have colleagues who helpfully break things into small PRs, but more often than not I wish they hadn’t. I usually want to review things in context of the big picture and that gets lost.

Usually what I do is check out their last PR, figure out what I want to say, and then identify the appropriate place to leave a comment in their stack of PRs. Which is a lot more work for me. And this assumes that they’ve even finished all their PRs instead of expecting them to merge in one at a time

tcoff917• 10 months ago

What I find helps with stacked PRs is it helps with getting code review incrementally as the various changes for a larger effort come together.

williamcotton• 10 months ago

Pro tip:

Get your potential code reviewers involved before you even start coding. Keep them abreast of the implementation. When it comes time for a review they should be acquainted with your work.

ed_blackburn• 10 months ago

This. Absolutely this. The PR should be the final step in the process. Never the first. Ambushing people with PRs and then demanding their attention is a massive time and mood sink. It is ineffective and counter-productive. You may as well just commit to main, and honestly, in so many situations I think that's perfectly rational thing to do, so much about PR culture is theatre.

Jcowell• 10 months ago

This seems like a bit of an over exaggeration. A pull request isn’t just a chunk of code thrown at people. it’s an entire process with a Title , description, pipeline checks that all come together to say I want this in another branch.

As the PR author it should be your job to: * Self Review the PR * Ensure you adhere/fulfill to all the expectations and requirements a PR should have before it’s pulled out of draft * That all pipeline test pass * That for a given request X there is test that validate X, Not X , and edge cases of X and are ran in the pipeline. * Has a clear description of what you’re changing/adding/removing, why, how, and the rollout plan , roll back plan , & the risk level.

The peer review process should make the reviewer engage in a rubber duck process to review their code , loop the team in for changes that can change their mental model of how a system they own works, and to catch things that we might not catch ourselves.

Not to the mention the security implications

keeda• 10 months ago

I feel like this is a good use of standups rather than just status updates. "I have started working on X" should ideally be accompanied by "I am planning on doing this using Y and Z" if there is any chance that it could be contentious. There should ideally be no contentions, but the moment any questions come up, they should be tabled for a post-standup huddle amongst the relevant stakeholders.

If you don't do standups, just do the same thing ad-hoc on the team chat channel.

Larger tasks and discussions are probably the purview of RFCs.

ziml772• 10 months ago

This might be why I'm finding a lot of these comments so confusing. Because what you're saying here is how I've always operated.

If the first time your reviewer sees what decisions you've made is when the review happens, then of course it will be overwhelming if the merge request is large.

If you keep your reviewer in the loop, and have bounced implementation ideas off of them, then the review basically just becomes a sanity check.

markus_zhang3• 10 months ago

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 put up a precommit for testing and plug in the same tests for the CI and call it a day. If you pass the CI I’ll take a cursory look and LGTM.

Some code is unreviewable. I work as a DE and it’s all business logic entangled. Why is there a specific id excluded? What is the purpose of this complex join? I mean, the reviewer is not supposed to know everything, right? So the best thing, again, is to only look at the technical things (is the join done properly), let the CI figure out the weird stuffs and LGTM.

g-b-r12• 10 months ago

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

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

roguecoder13• 10 months ago

"Why is the specific id excluded?" is exactly the kind of thing we can capture in code, and that a good code review will flag.

It takes two seconds to write `EVIL_IDS_THAT_STOLE_OUR_LUNCH_MONEY=[1] [...] NOT IN EVIL_IDS_THAT_STOLE_OUR_LUNCH_MONEY` instead of `NOT IN [1]` and then not hate your past self six months from now when you have to figure out why something has been excluded.

If some code can't be understood today, it's not going to be able to be understood when someone comes to modify it. Maybe in your domain all the code is write-once-read-never, but for those of us maintaining enterprise software that isn't an option.

We absolutely expect our reviewers to fully understand the code. If you want a quick shortcut to make that cultural change, have any bugs in code go to the code reviewer instead of the original author. You will find people start taking code reviews a whole lot more seriously.

markus_zhang3• 10 months ago

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.

danielfalbo• 10 months ago

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

- reviewing one iteration at a time

So one reviews single commits independently (takes probably around 5mins), and "forces" the reviewer to re-live the story that led to the bigger diff.

I do not work at Jane Street but I frequently find myself pondering on how broken the common code review system/culture is. I've heard of tools like graphite.dev that build on top of git to provide a code review system similar to the Jane Street one, but I'm not an active user yet (I just manually stack PRs, keep them small and ask for review to one at a time to my colleagues, and handle the rebasing etc. manually myself for now).

maleldil• 10 months ago

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/

mtoner23• 10 months ago

This is also the system we used with gerrit. Googles PR system. It uses cherry picks instead of branches but it's fantastic and gracefully splits the PR work between reviewer and committer

jmull• 10 months ago

I think a more fundamental and important aspect to this is developing a shared understanding of the design the change is ultimately intended to accomplish and the roadmap to achieving that.

Shared between the implementer and the reviewers, that is, which means design brainstorming, design formalization of some kind (writing the significant aspects down, or recording them in some other way), and a review process.

I should also say: this process doesn't have to be any larger or more heavy-weight than the change itself. And changes that don't have a design aspect can skip it entirely. But this article is talking about building a story with commits, and at that level you're almost surely talking about a significant design aspect.

bob10293• 10 months ago

I see the primary value of a pull request being the simple awareness of what is being worked on. I aggressively sync my local changes to PRs that are marked draft in GitHub. Other developers I work with do the same. Throughout the day we asynchronously check in on the scope of the others' work. If there is something that looks like it might conflict, we call a meeting.

The actual code review phase for me is more about making sure the checkin is clean and that what I am intending to work on wont get caught up in a conflicting mess. The code review is NOT a recurring opportunity to purity test my teammates. Presumably, the reason they are working with us in the first place is because they already succeeded at this. "Trust but verify" is a fun trope if you are working somewhere the consequences of a mistake are one-way and measured in millions of dollars. However, a bad commit can be reverted in 10 seconds. Builds of software can be easily recreated. Deploying to production is still sensitive, but why get all weird about rapidly iterating through dev or QA environments?

roguecoder13• 10 months ago

Why do you see feedback as "purity test"ing?

Newspaper reporters are professionals and they still have editors. We've all seen the disaster that results when an author gets too famous to edit. And we don't have to go in and work with their prose later.

Code reviews are where "my" code becomes "our" code: I want my coworkers to feel comfortable with and fully understand and be happy to support the changes I am proposing to our software.

bob10293• 10 months ago

Why do you see feedback as "purity test"ing?

I didn't say this.

There are many forms of extremely valuable feedback that do not involve subjecting your teammates to a ritual of "clean code" every time a PR is submitted.

saghm• 10 months ago

I feel like quite a lot of the pain is best solved for both sides by inverting the expectations a little towards the author of the PR like this mentions, but around communication, not the substance of the changes itself. Over the years I've built up a bit of a reputation on some teams I've worked on for crafting PRs that are disproportionately easy to review relative to the scope, and it pretty much is entirely from just spending a bit of extra time explaining things in the review comments itself. In addition to the top-level description (which in practice I've doing found often is something people who are understandably busy will just glance through quickly if not skip it entirely to jump right into the diff), I always go through my own code in the review tool without publishing and tend to add a fairly high number of comments explaining things that might stick out as odd in the context of the diff specifically (with comments in the code making more sense for things that will stick out regardless of context). My experience is that for a lot of potential review comments, it's not particularly hard to anticipate just from looking at the diff from the perspective of the reviewer, and it takes far less time as the author to look through and add comments explaining those cases than it does as a reviewer to go through and write up comments on all of those places (especially given that as a reviewer, I do think it makes sense to be thoughtful about how exactly to phrase comments on a PR given how easily tone can be misunderstood over text). My perception is that even going a bit overkill with the self-review doesn't hurt too much; often I'll notice that certain comments get a "thumbs-up" reaction compared to others that don't, which is a nice quick way for reviewers to signify that they understand what I've said and find it reasonable (compared to the comments with no reaction that I assume didn't end up being necessary to address a potential concern).

I picked up this habit from an early teammate (and manager, who eventually went back to just being a teammate because he didn't love being a manager) who recommended it, and in places I've worked where they've had struggles with their review culture, I've had colleagues express to me how much they love that they do this and mention to me that they've sometimes started asking other teammates to do it for certain changes (e.g. "some of this code looks like it might have gotten moved around without changing, but it's not obvious from the diff, do you think you could go through and note wherever that happened?").

At the end of the day, teams will function best when there's mutual good faith and respect for each other's time. (Obviously some teams are lacking this to various degrees, but at that point I don't don't think code review is really the larger problem, but just symptom of the larger underlying dynamic that either needs to somehow be addressed or the team will never work well). Recognizing where you can save your team time overall by spending some of your own is a pretty useful with that in mind, and code review ends up having quite a lot of low-hanging fruit in this regard both because the context that the PR author has tends to make the amount of effort needed to preemptively help the reviewers understand things is quite low compared to the reviewer needing to ask, and because the return on time spent by the author scaling with the number of reviewers.

manoDev• 10 months ago

Code review is a good way to ensure 1) the team has context 2) double-check the approach taken 3) keep conceptual integrity high. For that, you should make atomic PRs (not focus on minimal LoC, but rather a coherent patch set for a feature/bugfix), agree with your team that reviewing code is work and set appropriate time aside for it.

Also, code review should be ego free: 1) criticize the code, not the author 2) don’t be too attached to code written, the objective is the product and not number of LoC contributed 3) it’s okay to start from scratch after learning about a better approach, or even make more than one and compare approaches.

Where most teams fail is treating it as a gatekeeping process rather than context sharing, make PRs too small to be meaningful or only waste time arguing about code style and other minutiae.

Dan42• 10 months ago

I see posts like this one pop up from time to time. I love it. Based on my 30y of exp that's also the workflow I converged on. It seems to me like every experienced and skilled developer is converging on this. jujutsu is entirely built to accommodate this workflow.

There are no silver bullets or magical solutions, but this is as close to one as I've ever seen. A true "best practice" distilled from the accumulated experience of our field, not from someone with something to sell.

devmor• 10 months ago

I actually didn't ever realize that some people dreaded code reviews. To me, PRs are one of the most exciting parts of the process. That's where you have the most potential to learn from your colleagues and/or teach them something you know.

roguecoder13• 10 months ago

I am very glad that has been your experience!

It depends very much on your coworkers, unfortunately. When a team is all pulling in the same direction, and is kind and constructive and rigorous, code reviews can be awesome.

In some companies, especially ones with stack ranking where one person doing better means someone else does worse, it is easy for them to go horribly awry and become an ordeal of a bug hunt. The obvious solution is to not work at horrible companies that pit engineers against each other, but when that describes some of the biggest employers in the industry it's easier said than done.

parpfish• 10 months ago

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 story of what the final thing is.

sometimes you can get a good story by stacking PRs, but if the stack gets too deep you can end up with some rebase nightmares.

Master_Odin• 10 months ago

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 code got from point A to B, and PRs be the story of how each incremental step was made.

g-b-r12• 10 months ago

PRs only live on GitHub, what happens if it gets shut down or it accidentally loses some data?

niko_dex• 10 months ago

This post came up at a perfect time for me. I'd been feeling like my commits were too small, littering the activity pane on GitLab.

I guess I was worried that nobody would want to read the commits, but I really like the thought that what I've been providing is a simple narrative thread to help guide a reader through my train of thought.

necrotic_comp• 10 months ago

My preferred way of doing PRs/Code Review echoes some of the statements below, but also requires the engineer to do the following:

1) Each PR (even if it's part of a larger whole) can be committed and released independently. 2) Each PR has a description of what it's doing, why it's doing what it's doing, and if it's part of a larger whole, how it fits into the broader narrative. 3) PRs are focused - meaning that QOL or minor bugfixes should not be part of a change that is tackling something else. 4) PRs are as small as possible to cover the issue at hand. 5) All PRs are tested and testing evidence is presented. 6) When a PR is committed to master, the final narrative in step 1) is the Git commit, along with the testing evidence, includes the JIRA ticket number in the headline, and the reviewer in the body of the git commit.

This way we have a clean, auditable, searchable history with meaningful commit history that can help reconstruct the narrative of a change and be used as a guide when looking at a change in, say, a year.

gagabity2• 10 months ago

I never read the commit messages always straight to changed files.

Also find doing it like this either incredibly hard or have to do a ton of git magic after I'm done to get commits into this state which is very frustrating.

I think it might be the codebase I work on but who knows.

WorldMaker4• 10 months ago

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 and/or never think to use.

In the Files tab the commit picker has gotten better in recent years, but it is still overly focused on selecting ranges of commits over individual ones, and there's no shortcuts to easily jump Next or Previous commit, you have to remember your place and interact with the full pulldown every time. Also, it's hard to read the full descriptions of commits in the Files view and I find I often have to interrupt my flow to open the commit in another browser tab or flip back and forth between the Commits tab and the Files tab in the PR. The Commits tab also defaults to hiding most of the commit descriptions, so it's still not a particularly great reading experience.

It feels like a bit of a bad feedback loop that GitHub's UI doesn't make commit-by-commit reviewing clean/easy because GitHub themselves don't expect most developers to write good commits, but a lot of developers don't write good commits today simply because GitHub's PR interface is bad at reviewing individual commits and developers don't see as much of a point in it if they aren't going to be reviewed in that way.

candiddevmike• 10 months ago

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

XorNot• 10 months ago

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

WorldMaker4• 10 months ago

Good commits can tell a story. (The article here discusses this, too, and suggests that good commits should tell a story.) When a commit author takes the time to fill out the commit message, it will include things like the "how" and "why" of each step, what they were thinking about as they worked on that part of the whole PR. Often I find that will save you from asking questions like "Why did you take this approach?" which I see all the time in PRs (and make all the time in PRs with "bad" or just "save point" commits).

The PR should be the smallest unit of integration (this works and builds and is ready to merge to the next steps), but the commit is the smallest unit of any progress at all. Ideas don't come fully formed and ready to compile. Progress sometimes includes back tracks and experiments. Good commits say things "hey, I learned from this thing that wasn't working and that's what pushed me into this next direction". They document the journey, why a specific path was taken, what obstacles were in the way, what other paths were explored and dismissed.

Some PR authors can capture a lot of that in a PR description as well, but commits tie that to specific context of the code at a point in time in ways that a PR description often can't, without linking to commits (in which case the commits again speak for themselves).

But yes, not everyone can or will write good commits. I see that partly as a tooling failure because our tools themselves like GitHub PRs don't encourage it, often fail to reward it. I've seen plenty PRs full of commits named only "WIP" and "Fix" and "oops", but the best PRs tell a story in the commits, have meaningful descriptions on each commit. I would love for our tools to encourage more of those because I think they are better PRs; often easier to review PRs and enough good PRs like that form a string of documentation that you can search through (through git blame and git log if nothing else, but that's still a lot of useful research data). (If you keep that data, I know a lot of people like squash merges because they distrust the git DAG and how many tools show ugly or hard to read "subway diagrams" for it instead of simpler collapsible views. But that's another long conversation.)

DenisM• 10 months ago

Graphite addressees this issue in a different way - it lets the author split a pr into several smaller prs, so one can review it piece by piece, but still see the whole super-pr in one place. It does a decent job rebasing stacked prs as well.

arcanemachiner• 10 months ago

If faced with this issue, I would probably just pull the remote/branch locally and step through it, commit by commit, using my preferred Git manager (Lazygit).

WorldMaker4• 10 months ago

My big complaints are with the default experience in how that affects everyone's expectaions, but I do this sometimes, yes. The VS Code "GitHub Pull Requests and Issues" extension has gotten really good and gives a lot of tools for this. If using github.com (and not GHE) you can even use the "." shortcut in any PR to open that PR in github.dev, which is a web version of VS Code (that you can Settings Sync) to review the PR there without even needing the local checkout. But also that's a bit of a "hidden shortcut" and very few developers know about it, which again gets back to what the GitHub interface provides by default and how it makes things discoverable being something of an underlying issue.

KronisLV2• 10 months ago

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.

roguecoder13• 10 months ago

If that's what the code is like and you aren't allowed to make it better, that is not a safe workplace.

Especially if the software matters at all.

I purposefully find jobs in fields like healthcare and physics and finance where it actually matters that the software works. Right now, if there is a bug in my code people could die.

And in that case, there are worse things than being fired.

If people do find themselves in that situation, the best answer is "unionize". The second best answer is to work with your coworkers to adopt better practices (it is very unlikely that they are going to fire you all all at once). And the third best answer is to do the job well, regardless of what is going on around you, and if you get fired you get fired.

KronisLV2• 10 months ago

If people do find themselves in that situation, the best answer is "unionize". The second best answer is to work with your coworkers to adopt better practices (it is very unlikely that they are going to fire you all all at once). And the third best answer is to do the job well, regardless of what is going on around you, and if you get fired you get fired.

That's a pretty nice way to view things! I've been lucky enough to mostly be in environments where I can work towards that (even if there is the occasional enterprise legacy project that can feel as a maze).

But at the same time, even in better circumstances, people can get lazy, I certainly do, and are sometimes also set in their ways. In those cases, I'll take any automated tools that will make it easier to do things in more organized and better ways - e.g. automatic code formatting with a linter for consistency, or various scripts to run on the CI server, to check codebase against a specific set of rules (e.g. "If you add a reusable component, you should also add it somewhere in the showcase page").

I wonder what automation and tooling could be used to manage the complexity of some pull requests, even when people have deadlines or sub-optimal understanding of the whole codebase, or any number of other reasons that make things more difficult. Things like better code navigation tools (even in the web UI), or more useful dependency or call graphs, a bit like what SourceTrail did back in the day, before going bust: https://github.com/CoatiSoftware/Sourcetrail

dvcoolarun• 10 months ago

It’s a challenge: you put the craftsmanship into a PR, but the reviewer might not share that dedication—they just want to give an LGTM bypass to clear their queue. That's why working at a company that truly values these practices is essential.

javier_e06• 10 months ago

I see Pull Requests and Code Reviews the same way as making a sandwich for someone else. My team wants for me prepare a sandwich ASAP.

They, the reviewers, have to eat it, not me.

Some reviewers want wonder bread bread with a slice of spam.

Some want hand-made spreads with home-grown vegetables.

Is easier to fix a sandwich than a wedding cake.

The take-away

Don't do wedding cakes.

roblh2• 10 months ago

Maybe I'm just a scrub, but something that I find makes it harder to do smaller commits is that I frequently rely on being able to see which lines I've changed directly inline in my editor. When you commit, vscode now stops highlighting all of those lines, and that makes it much more difficult for me to orient myself relative to what I've already done. The individual lines, and the git pane that shows which files have been changed, act as waypoints for me while I'm working on stuff. It's particularly important on more complicated features that span more files, and I'll often intentionally commit stuff I feel like I'm not likely to touch again to reduce some visual noise.

hypeatei• 10 months ago

  git add --patch

...is your friend if you want to leave all your changes unstaged for awhile then break it out into multiple commits later.

t-writescode5• 10 months ago

To add, when I’m breaking my changes down into multiple parts for review, I tend to:

  * squash everything I’ve done into one commit
  * create a new branch off main/master that will be the “first commit”
  * cherry-pick changes (easy from some git guis) that represent a modular change.
  * push and make an MR from the new branch
  * rebase “the big commit” on top of the partial change.
  * wash, rinse and repeat for each change, building each MR off its requisite branch.

The squashing part is vital because otherwise you enter merge conflict hell with the rebase.

t-writescode5• 10 months ago

This sounds like a real and valid incompatibility with a high frequency commit cadence.

If you’re interested in trying these strategies anyway, does your editor of choice have an inline “git blame”? In IntelliJ, I can see who and when committed the most recent change in the line around the one I’m working on.

It doesn’t resolve the “which files have I worked on” issue; but it might help the others? Not as nice as a different colored line like uncommitted code would otherwise be highlighted, but it could be enough of a step in that direction?

crazygringo2• 10 months ago

That's really interesting.

Seems like it would be even better if VS Code provided a way to highlight all lines changed relative to a particular commit like the start of a branch. Maybe it's worth filing a feature request?

(I don't use VS Code this way so I'm assuming it doesn't already have this.)

rd• 10 months ago

you could probably write an extension to accomplish this in a couple of days with GPT-5 now

TiredGuy• 10 months ago

I agree this can be a bit of a pain if you're used to that. There are ways to partially reduce it: 1. use the timeline panel for an individual file to see all the historical changes to a file, and you can highlight multiple ones to see cumulative changes, and you can filter to only git commits or only local changes etc. 2. use the commit history panel in the source control area to do the same across commits, but it doesn't allow you to highlight across commits for cumulative changes

It does require a bit of a paradigm shift sometimes to not rely as much on seeing all cumulative changes for the ticket highlighted as you code, and instead compartmentalize your immediate view to the current commit's task, but often the above 2 alternatives help suffice. Of course, you did mention that you'll commit stuff you're not likely to touch again, which helps a lot too

conradludgate2• 10 months ago

My recommendation: don't treat the development process as the final draft.

Write everything in one go, and then afterwards rework the commits to tell the story you want to tell. This does require getting very comfortable with git rebase, but I can absolutely recommend it.

Using this technique, I've managed to merge several large refactors lately with no issues, and the reviewers didn't hate me.

kiitos12• 10 months ago

holy moly absolutely not

git is a means, not an end

code review is about the code as a unit whole, not the steps along the way!

mnembrini• 10 months ago

In Intellij if you open a PR it will highlight lines that have changed in the PR differently (even from multiple commits) and you can click on the colored border to see the version of the code on the main branch

notapenny• 10 months ago

Couldn't you use something like GitLens for that? I haven't used it in a bit but IIRC it lets you see your changes versus any branch pretty easily. Personally if I do feel the need for a view of what I've touched, I just open up a draft PR.

roblh2• 10 months ago

You certainly can, I specifically like being able to see it in real time though. It’s less useful if it isn’t constantly present without having to bring it up with a click/command.

mixmastamyk7• 10 months ago

I commit early and often, then push the feature branch to gitlab, etc. Then I look at the draft merge-req diff in the browser.

Meaning, I can keep committing while also able to see the full changes evolve.

herval9• 10 months ago

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 necessarily easier to review (and this arbitrary obsession almost always leads to PR overload in chunks that don’t make any sense, reducing code quality as a result)

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

- “every commit must compile” - again, unnecessary overzealousness. Every commit on the MAIN branch definitely should compile. Wasting your time with this in a branch, as you work towards a solution, is focusing on the wrong thing

You want PRs because they help others absorb what you’re doing (they’ll have to read that same code sooner or later). You don’t want to create a performance theater.

leetrout2• 10 months ago

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 without taking context into account.

BobbyJo3• 10 months ago

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"

m0008• 10 months ago

PRs are emails to your team and to your future self.

This should be commits though. Typically, developers would look for clues in this order:

code -> code comment -> commit message -> PR text -> external document

So commit messages puts the information closer to the user. One hop doesn't seem much, but the time saved adds up as you go.

Also, as some other reader mentioned anecdotally, PRs may not be there forever. E.g. your team may migrate to a new platform PR text and reviews were left behind.

bear8642• 10 months ago

PRs are emails to your team and to your future self.

Indeed! I've found many point on this discussion answered by the linux kernel idea of mailing lists where a change is discussed then approved, often with feedback acknowledged

MoreQARespect4• 10 months ago

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 to me, Ill show you" they often demur and I end up with a huge PR anyway.

In practice I dont think ive ever seen a long PR that wouldnt have benefitted from being strategically broken up, but every other day I see another one that should have been.

herval9• 10 months ago

Sure.

BobbyJo3• 10 months ago

Yes, TDD on production code is nice in theory, but it doesnt work in my case...

Parent said something more along the lines of "they don't work in every case, and trying to force it in every case is misguided".

I agree that too big is more common than too small with respect to PR size, but you aren't putting forward much of an argument against parents "there are no absolutes" argument by straw manning them.

throwaway314155• 10 months ago

This doesn't match my experience and I assume is deeply cultural and subjective.

crowbahr• 10 months ago

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.

mountainriver• 10 months ago

Thank you, more people need to read this. The software industry seems packed with these strange gatekeeping structures that only hinder development.

Focus on customer outcomes, and keep main clean.

SkyBelow• 10 months ago

nobody reads intermediate commit messages one by one on a PR, period.
Don’t waste your time writing stuff for no one.

I've thought about that as I continue to write them. I think I can justify it by saying they are mostly for me. Can I describe what I'm trying to do with a specific push into a few items. It let's me reflect if I'm waiting too long between commits or if my ideas are getting too spread apart and really should be in two different branches that each have their own PRs. Then there is the rare case on a slower project where an item gets deprioritized and I come back to it weeks or even months later. Having the messages help me catch back up to speed.

As such, I find the 20 seconds or so to type out 1 to 2 sentences to be worthwhile, even if the ones reviewing the eventual PR never check. I'm also not above throwing in a "ditto" or "fixed issue" when a single commit really is that small or insignificant.

“every commit must compile”

I agree with your take this is overzealous, but to expand upon my previous point, if I know a commit on a branch won't compile (say just had something else come up and need to swap focus for a few days), then I'll try to make sure I call that out in my last message just in case anyone else happens to get put on the project.

If I were to summarize my approach, treat PR messages seriously, but treat branch commit messages like sticky notes that will likely end up in the trash by week's end.

DavidWoof• 10 months ago

“nobody reads intermediate commit messages one by one on a PR”

I clean my history so that intermediate commits make sense. Nobody reads these messages in a pull request, but when I run git blame on a bug six months later I want the commit message to tell me something other than "stopping for lunch".

pedantically apply DRY to every situation or forcing others to TDD basic app

Sure, pedantically doing or forcing anything is bad, but in my experience, copy-paste coding with long methods and a lack of good testing is a far more common problem.

You may be 100% correct in your particular case, but in general if senior devs are complaining that your code is sloppy and under-tested, maybe they aren't just being pedantic.

tossandthrow• 10 months ago

Sure, pedantically doing or forcing anything is bad, but in my experience, copy-paste coding with long methods and a lack of good testing is a far more common problem.

This is a false dichotomy and an unproductive thing to focus at.

Experienced engineers know when to make an abstraction and to not. It is based in the knowledge about project.

Abstarct well and don't do compression. Easy said, and good engineers know how to do it.

m4r71n• 10 months ago

I actually find the relevant PR/MR discussion a lot more useful than the commit messages themselves. So any git blame is just to get a commit hash and look that up in GitLab/GitHub to see the entire change set and any comments around it. It makes me wish those comments were bundled with the merge commit somehow and could easily be accessed in the terminal where I'm viewing the git history.

singpolyma311• 10 months ago

Yes. I think many people have no culture of good commits, so they never use bisect or blame, so they never see the use of good commits. It's a cycle

chrislo• 10 months ago

nobody reads intermediate commit messages one by one on a PR, period.

I do! I find it the easiest way to review code when the author has taken the time to structure it in that way. I'm lucky to work with some great people.

jonahx4• 10 months ago

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 said, I think changing the culture rather than throwing away the practice would be a better response.

Reading and reviewing clean history is really so much nicer. I'd also argue that actually making your history clean (as opposed to theatrically and thoughtlessly making small commits, say) forces you as the author to review it more carefully.

IshKebab2• 10 months ago

IMO there's no point having a clean history of commits within a PR. With rare exceptions, if you have a PR with a clean history of commits and each commit compiles and passes the tests... they should be separate PRs! If it isn't clean then it should be squashed.

A few exceptions:

1. When refactoring often your PR is "do an enormous search and replace, and then fix some stuff manually". In that case it's way easier to review if the mechanical stuff is in a separate commit.

2. Similarly when renaming and editing files, Git tracks it better if you do it in two commits.

3. Sometimes you genuinely have a big branch that's lasted months and has been worked on by many people and it's worth preserving history.

Also I really really wish GitHub had proper support for stacked PRs.

fillmore• 10 months ago

Replying to echo this, I also read every commit message, and review PRs commit by commit. This was common practice at my last job (a small, experienced team), and the expectation was that commits were atomic.

Yes, we griped that GitHub would not allow us to merge individual commits, but if it was ever urgent or helpful to do so, we cherry-picked a commit into a separate PR.

Everyone's workflow is a bit different, and it can be hard to redirect organizational inertia. But without a doubt, reading a clean commit history is a pleasure.

sodapopcan4• 10 months ago

Also replying to echo this. Hate these blanket statements.

sodapopcan4• 10 months ago

Reading and reviewing clean history is really so much nicer.

You can have both with git and it's not even hard. Unfortunately it seems many people pride themselves in what little they know of git. I'm not being sarcastic, I've read people say this almost word-for-word.

William_BB2• 10 months ago

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.

siva73• 10 months ago

I've met thousands of developers over my career and i could put them into two categories: those who don't give a shit about intermediate commit messages (majority) and those who browse every single intermediate commit message in a PR (very few). To be honest, the latter had some tendency to be difficult to work with. It was also a useful discriminator to avoid getting those into my teams.

mdavid626• 10 months ago

With commit messages you miss the point. It’s more like the final test of the commit. If you can’t formulate easily what you did and why, then you need to rethink your changes.

sleepybrett• 10 months ago

- 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 often do, In a larger PR or in one where it's hard to tell what is being accomplished, like this article articulates the commits can tell a story of the engineers journey to solution. Even if I review a commit that is largely undone by future commits that piece of history is often key to my understanding.

bornfreddy2• 10 months ago

This. Just last week I have split a coworker's single-commit-MR into multiple commits so that I could distinguish between unrelated changes and to check smaller chunks of code. It worked beautifully.

joshlemer• 10 months ago

nobody reads intermediate commit messages one by one on a PR

I think it's fine to have a whole bunch of "WIP" commit messages on intermediate commits while the PR is in a draft stage, but then all of those garbage commits should really be squashed down into one commit and you should at least write a one liner that describes what the whole change is doing. I think it does materially make repo history harder to understand to merge in PR's with 10 garbage commits in them.

gwbas1c• 10 months ago

“every commit must compile” - again, unnecessary overzealousness. Every commit on the MAIN branch definitely should compile. Wasting your time with this in a branch, as you work towards a solution, is focusing on the wrong thing

(With few exceptions,) I generally follow this practice; BUT, I think enforcing this on other developers feels like micromanagement. That being said, with few exceptions, committing code that doesn't compile feels like an incomplete sentence.

(Sometimes on massive refactors I make commits that don't compile. It gives me a place to roll back to. If someone thinks this is poor practice, than I think they're putting principles in place of practicality.)

herval9• 10 months ago

that's the whole point.

A _branch_ is a unit of work that should be merged when done.

As the owner of a branch, an engineer has the ability to move into intermediate states. The larger the codebase, the larger the possibility of something unexpected breaking or not compiling. Just like editing a large body of text - you will have "incomplete sentences" through the process. It's part of writing. Expecting others to write their drafts the same way you like is just silly - it's putting rigid principles ahead of anything else that matters.

epage4• 10 months ago

While I agree about not having hard and fast rules, like LoC per PR, the principles of this are very relevant.

When reviewing a conglomerate commit in a PR, I have to reverse engineer how the different changes interact to figure out the intent. I then have to do this on each update they make. Contrast that to when someone breaks up their commits where I can zoom through variable renames, extracting functions, etc to see the one line that change that all of that unblocked that makes the difference. Then if updates are pushed, I only have to worry about the commits that were updated.

As for all commits compiling, that is helpful to review the individual commits.

Both of these (small commits, all compiling) are also great for bisecting. You get pointed to a very small change that you can more easily analyze vs dealing with breakages or having to analyze a large change to find what the problem is.

criemen• 10 months ago

I agree with you that this shouldn't be 100% hard defaults, but it's a good standard to have, and imo it's valuable to be explain why one is deviating from it.

- smaller PRs aren’t necessarily easier to review (and this arbitrary obsession almost always leads to PR overload in chunks that don’t make any sense, reducing code quality as a result)

Oh but they sure can be reviewed more easily, because they are shorter? Doing so feels like less effort, and you get a dopamine hit from hitting that "submit review" button faster/more often (improved morale, and PR turnaround time!). Plus, if there's a longer discussion about X, it's great if it's not tangled up with Y and Z at the same time - allowing you to dig into X.

- nobody reads intermediate commit messages one by one on a PR, period.

Come on, that's intellectually dishonest. 1. VSCode displays commit messages inline as blame for me (and many of my colleagues), so even when we don't read the commit messages one by one _on a PR_, I often read them later in the IDE (we don't squash merge PRs). I spend significantly more time reading code than writing, and commit messages, PR descriptions and linked issues provide extra context that is useful to me especially for complex code. If those messages were entirely unreadable, I'd be annoyed. 2. When someone invests time into telling a good story commit by commit, in my team they write "Review commit-by-commit is encouraged" in the PR description, to tell the reviewers that yes, they should read the individual commits, as that'll make understanding the PR easier. Often as reviewer, I follow that suggestion.

Wasting your time with this in a branch, as you work towards a solution, is focusing on the wrong thing

It seams you're conflating "working on a feature" with "presenting it as PR to review". That's two very different things, and Edamagit in VSCode makes it so so easy to provide a reasonable commit history that hides some of your missteps, and to fill in commit messages.

herval9• 10 months ago

we don't squash merge PRs

you need to be careful with every single commit message, every commit must compile, etc, in your case. My comments apply if you squash-merge, in which case all that commit-level care is not necessary since intermediate commits go away on merge. You’re probably making your life harder for no reason for avoiding squash-merge, but that’s just my opinion

muxator• 10 months ago

- nobody reads intermediate commit messages one by one on a PR, period [...] > - “every commit must compile” - again, unnecessary overzealousness. [...]

In my part of the world both of these are true, and proudly so. We keep catching a myriad of errors, big and small. The history is easy to read, and helps anyone catching up with how a certain project evolved.

I understand it might not be true for everyone, every team, in every line of business; but this sort of discipline pays off in quality oboth of the code _and_ the team members' abilities.

lamontcg2• 10 months ago

Sometimes you're overhauling something which you can't do in chunks less than something 2,000 line long PR. There's no intermediate working system. The problem is, "take this very large bit of code and throw it entirely away and rebuild it completely differently". Trying to craft some evolutionary step between A and B is just going to take 10x longer and won't help any code reviewers.

mateo411• 10 months ago

I agree.

When you have a large PR like this, here's how I like to get it reviewed.

1. Give reviewers sometime to become familiar with the PR. They might not understand all parts of it, but they should have at least a cursory understanding of the PR.

2. Have a meeting where the PR is explained in front of the group of reviewers. The reviewers will understand the PR better and they can ask questions in realtime.

3. Let folks review the PR after the meeting in case they spot anything else, or think of additional questions.

Most of the time PR review is done asynchronously, but doing most of the review in the meeting can also be a decent team building exercise.

jcalvinowens• 10 months ago

It's clear you've never worked on a large open source project... There are good reasons for all the practices you're thoughtlessly dismissing.

I agree that for a common team of programmers working for a single company, the value isn't always there. But that's the easiest and least interesting case... in big distributed projects this stuff really matters.

1-more2• 10 months ago

nobody reads intermediate commit messages one by one on a PR, period.

Very common practice at my old company, and one I continue in my current role.

“every commit must compile”

sucks ass for anyone else trying to rebase your branch onto the update main/master when they don't. Once your PR is out of "working on the feature" and into the "getting it merged" phase, do a little `git rebase -i` and squash your really intermediate commits into ones that compile. Ignore this if you have real grown up CI where your PRs never stay open for more than a day.

bob10293• 10 months ago

Ignore this if you have real grown up CI where your PRs never stay open for more than a day.

A vast majority of the drama that comes out of source control is associated with branches living for far too long.

I've got an internal alarm that starts to go off somewhere around 72 hours. If something takes longer than this, I've probably screwed up in my planning phase. There are some things that do need to sit, but they should be rebased every morning like clockwork. The moment things start to conflict, the PR gets closed and the branch is now a reference for how to do it again when whatever blocker is cleared.

Another way to think about all of this is to pretend like everything you are touching is taking a synchronous lock out (even if it's not), similar to how tools like Perforce behave. So, you generally want to move as quickly as possible to get out from under lock contention. Git allows you to pretend like you aren't conflicting for a really long time, but at some point you must answer for all of this debt (with interest).

herval9• 10 months ago

sucks ass for anyone else trying to rebase your branch onto the update main

why would anyone else rebase your branches? YOU should rebase your branches.

zzzeek• 10 months ago

I disagree with everything you wrote here. Prs must absolutely limit their scope both in terms of length as well as what they accomplish (e.g. don't do unrelated refactorings in a pr delivering something else), large features must absolutely be broken into individual commits if not individual PRs, I definitely read each one and each one definitely has to complete and pass tests 100%, else they are leaking details into the next or that would "fix" the problem. This is also absolutely nothing like "forcing TDD" in people and these are all practices that junior devs should absolutely be doing since it will help them to think about code, change and maintainability a ton.

keybored2• 10 months ago

- smaller PRs aren’t necessarily easier to review (and this arbitrary obsession almost always leads to PR overload in chunks that don’t make any sense, reducing code quality as a result)

This is why I gave up reading the article shortly after reaching the point about making a history with commit messages. The comments—even if it is on a Git forum—will just be full of people that either say that it’s a waste of time or that it is literally impossible for this to be practiced by anyone.[1]

Your best bet is to find projects where this is practiced (and you don’t have to look far). But making the case to a general audience? No, too many loud voices that treat version control like “I am committing now because I need to pick up the dry-cleaning” arbitrary/random snapshot-maker.

[1] No one, period? Sounds like a bit of a strict ontological rule to me.

smcameron2• 10 months ago

- “every commit must compile” - again, unnecessary overzealousness.

So you're the one breaking git bisect all the time. Grrrr.

Use stgit and make decent commits instead of rolling in the dirt like an animal.

osigurdson9• 10 months ago

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

osigurdson9• 10 months ago

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

theonething• 10 months ago

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

watwut2• 10 months ago

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.

DarkNova6• 10 months ago

Thank you so much. You speak from the bottom of my heart.

jiveturkey3• 10 months ago

I write intermediate commit messages as notes to self. You don't always work continuously on the same PR. The commit messages are a useful context refresher.

Why advocate against this anyway? If no one reads them, it harms no one. Just like personal blogs. However, the writing of the blog is the useful act, not the reading.

Ironic that you are accusing TFA article of being an expert novice. I don't disagree your take on him / the article, but you are committing the same sin.

herval9• 10 months ago

You missed the point entirely. The point is forcing others to do something that has no inherent value to them or to your process, just because you like it, is junior behavior.

michaelcampbell2• 10 months ago

- “every commit must compile” - again, unnecessary overzealousness.

Until you need to `git bisect`. Then you'll require that every commit compile, pass tests, etc.; even if that means rebase/squashing to do it.

whatevaa2• 10 months ago

You don't bisect a merge/pull request. There is no need for it, unless it is a giant, but then your workflow is different.

Main has clean history and every commit is good.

ornornor• 10 months ago

reads intermediate commit messages
every commit must compile

I’m in the opposite camp. Following these two practices often doesn’t make any difference but the few times it did saved me a ton of time.

Dropping commits or rebasing is much easier when you have descriptive, atomic commits. It’s also helpful when performing git blame archeology to try and understand why this code looks so weird and has no context. It’s also useful when bisecting (not so much a problem with small PRs, quite handy as they grow bigger and bigger)

As with everything it’s about context and circumstances. As you gain expérience you can appreciate and gauge when it’s required. When you don’t have the expérience then you follow rules so that you gain said expérience. That’s how I see it.

Illniyar• 10 months ago

I disagree with practically everything suggested.

Reducing scope and splitting a single task into multiple PRs each small but part of a bigger picture makes it very hard to see the bigger picture.

You should try to make PRs small, but if a PR is big, then you just have to spend more time to review it.

Formatting commits as a story is a huge hurdle for the one making the changes. And unless every PR is meticulously prepared - going over the commits by the reviewer is a waste of time.

I agree you should return PRs you don't understand though. Or don't feel comfortable reviewing for whatever reason.

CraigJPerry• 10 months ago

Pair programming > PR reviews.

I've heard people scoff at the $$ cost of "mob programming". I think that view is totally myopic, for appropriate problems there's just no faster nor higher bandwidth way to transfer code knowledge in a group.

Plenty of people dislike pair programming, i don't dislike it but i do find it mentally intense, tiring. I really really enjoy that it's an accelerator for getting to done - not just i wrote the code but the code is correct - sooner.

Long way to say don't rely on pull requests when you could be doing pairing for the important stuff.

EasyMark• 10 months ago

The only time I like pair programming is when I (or someone else) is stuck on an issue and we need to work together and bounce ideas off each other; otherwise I prefer to work on a problem myself rather than trying to be in constant contact with me paired person.

arnorhs• 10 months ago

This idea of every PR being a small chunk that you can review in 5-10 minutes is completely ridiculous. It is reasonable for bug fixes or small improvements, but the review time you should expect for a PR should reflect the size and impact of a feature, not some arbitrary number.

Yes, everybody would love it if every PR was small enough. In reality that is not a good way to build substantial features.

Often, fully building out a substantial feature, causes you to change your mind and completely changing your approach the further along you are. You don't want to be muddying up the PR pipeline with a bunch of half-assed changes.

Doing that just makes reviewers less inclined to give good feedback on a PR, because they "know it's going to change so much anyways".

If you are building a substantial feature, it is reasonable that the PR is large and reviewers will have to dedicate substantial time to reviewing it. Reviewing it is work on its own and hopefully your engineers have dedicated time to review substantial features.

Of course, you should make sure your substantial feature is as minimal as possible, for whatever is needed to ship the feaure - but not any less than that.

crazygringo2• 10 months ago

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 approach as a whole or making sure the parts fit together right.

And then the idea of "telling a story with commits" feels like a waste of time to me. I have no interest in the order in which you wrote the code, or what you wrote and then rewrote. The code itself needs to be legible. Your final code with its comments should speak for itself. Code is the what and comments are the why.

Now, what I will say is that the more junior the developer, the smaller their commits should be. But that's because they should be assigned smaller features, and have more handholding along the way. And when people are making larger architectural changes, they should be getting signoff on their overall approach from the start -- if you're frequently rejecting the whole approach to a problem in code review, something's going wrong with your communications processes.

Too• 10 months ago

Yeah. While this narrative style tries to explain what things are done, it instead often leaves the question: Why are we doing this at all?

Commit #1 adds a helper function for whatever, looks innocent enough, implementation is correct. Believe it or not, it even has tests, lgtm. Then only by commit #8 do you realize this helper function is not needed at all and the entire approach is wrong. Happens every time.

I started reviewing these chains backwards and refuse starting a review until the whole chain is available. That’s however not always easy either, when commit #2-#5 has incrementally refactored everything into something unrecognizable, so that both the left and right side of the diff are wrong! No, I’m not interested in ”this will be fixed 2 commits down the chain”. I just want to review the final state that goes into production, nothing else matters.

Yes, commits should be made small whenever possible and not include unrelated fixes or refactors. Just please, keep them meaningful on their own.

jordwest• 10 months ago

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?"

lo_zamoyski• 10 months ago

It's a nice thought, but PRs, when more than performative, are often more about basic sanity checks than anything else.

There are two things that can be involved when a change is submitted: a design change and an implementation change.

Design changes that are significant enough to warrant review should be expressed and discussed in text, not code, in design documents of some kind. These tend to be quite general, but they establish agreement about general structure, which tends to be the most difficult to change later on, as it circumscribes and guides the implementation that follows. Writing is also a way of working out ideas. The very act of having to explain something clearly to someone else forces you to confront your own ignorance and the consequences of your proposal. Besides, it's the job of the person proposing the design to work out those consequences so that others can verify whether they're true. (1)

Reviewing implementation changes with an understanding of design allows the reviewer to understand how changes relate to the whole as well as the aim. This is an insider's perspective. (2)

Reviewing implementation changes without a good understanding of the context will be limited to general technical remarks, but can descend into excess attention given to style or inconsequential matters of taste. This is the outsider's view. (3)

The question, I think, that looms in the background is whether familiarizing yourself with the context sufficiently well so that you can judge the PR submission is reasonable for a PR. In many cases, it isn't. It's too time consuming and the context is too big. If we had infinite time, this would be great: having to explain to an outsider what you've done forces you to give a much more thorough account, if your goal is to achieve thorough understanding. It also exposes your thinking to someone who doesn't necessarily share your assumptions. But this can be a Herculean task for anything sufficiently complex. So the criticality of the change must be weighed against the effort needed to explain or learn. Are you making a change to something with high tolerance for error, or a small margin of error?

Two pieces of advice...

Since it is unrealistic to expect an exhaustive verification all the time, focusing more on tests will be more fruitful. You still need context to judge whether they're exhaustive or test the right things, but it's the one place where correctness criteria are expressed in code, apart from type signatures, in a clear enough manner that expectations can be judged. If they aren't clear, you should ask. It's a good locus for discussion.

The second: code doesn't include the rationale or "why" for what it is. It just is. Context goes a long way to help infer reason for the change. This means we should use comments, either in the code or the PR submission itself, to explain changes. If something isn't sufficiently clear, ask.

But the key is prudential judgement. You have to determine how to limit your verification efforts and when to begin accepting on trust for practical reasons.

And do away with your pride. It's only difficult to ask questions if you suffer from pride, and pride is a sure sign of mediocrity. You're also lying through omission.

chickenzzzzu• 10 months ago

Clean coders, PR reviewers, and architecture astronauts deliver astonishingly useless code at incredibly slow rates.

Features are everything. Don't give me that bullshit about refactoring and generifying and DRY and TDD.

anal_reactor• 10 months ago

My biggest problem with reviews is that I just don't care. I work for a company that does nothing interesting with a coworker that doesn't like me under a manager that is incompetent. I have zero reasons to do a good job, and all the reasons to do bare minimum not to get fired.

citizenpaul• 10 months ago

I've seen numerous comments over the years on HN where people admit that their process for review is:

Yea, this person knows x so blindly approve.

Hey jane/john did you remember to check x before this? Yes! Ok, blindly approve.

flatline• 10 months ago

To pile on to the litany of complaints, I think PRs are a rather poor abstraction. It’s not a good way to review code. If I’m doing an actual review of anything complex, I’ll check out the branch, step through the code, jump around in the IDE: this is how we work with code, not through a fairly unreadable git diff.

PRs have no scope and no way to designate scope. What do I want from each reviewer? At some point, the PR becomes superfluous to adequate code review and communication around said review.

Ultimately they can be used to micromanage and gatekeep merges to main. This is the PR at its worst.

Overall I’m not a big fan, it feels like a necessary evil to meet the demands of Big Agile.

mercurialsolo• 10 months ago

This seems a lot like fragmenting context across the edit and the commit itself. With coding agents in the loop now

1. Why do you want to fragment context? Why not add this to the main file with inline docs?

tibbar• 10 months ago

I would argue that code review of large changes is hard because we don't write good PR descriptions. If you just get 200 files to review in alphabetical order, sure, that's basically an impossible task. Instead, the PR description should have the narrative of what the PR is about, what is the high-level strategy and key decisions, what are the entrypoint(s) to reading the code. What does correctness look like, and what have we done to ensure that? How does this PR fit into the important problems we're working on and what follow-ups or maintenance are implied by it?

If you have a good description, then you understand (a) what parts are important to think about and (b) do you agree with the approach and (c) is there anything in the code that doesn't line up with the approach.

This strategy scales up pretty well to even large PRs.

scottgg• 10 months ago

As a jujutsu fan, I like to prepare a nice clean commit history for my reviewers to probably ignore. But I kind of assume - there’s _some tool_ we could use - perhaps on top of GitHub - to make stacked reviews not suck, either actually stacked PRs or just incrementally reviewing a nicely divided branch PR. I thought it might be graphite but it seems that needs some whole other tooling _stuff_ on top? Any recommendations ?

kiitos12• 10 months ago

yeah, for sure -- avoid stacking changes in the first place! the purpose of PRs and code review is to establish a single unified shared context between multiple stakeholders. it's the author's responsibility to propose changes that are independent and coherent and easy to understand.

lijok• 10 months ago

The only advice worth following here is: send shit PRs back.

If you don’t understand the change, it’s too large, it contains multiple independent changes that should’ve been separate PRs, anything that doesn’t smell right - send it back.

My expectation when you review my PR is that your ass is on the line just as much as mine if something goes wrong.

PRs aren’t a checkmark exercise that validates you’re not trying to backdoor an exploit into the system. A reviewer that accepts a change is committing themselves to maintain said change going forward.

If you let your kids get a dog and the kids don’t take care of it, you will. There’s no two ways about it.

riyanapatel• 10 months ago

This post hits on something crucial - the difference between performative code review and substantive collaboration. The "theatre" metaphor is spot-on. The core issue Saša identifies - PRs that are "unreviewable" getting rubber-stamped with "LGTM". Teams go through the motions without the actual substance.

The storytelling approach through commits is brilliant, but it only works if you solve the human factors too. Even perfectly crafted PRs with great commit narratives get surface-level reviews. The friction kills engagement.

A few complementary approaches I've seen work: pair reviewing for complex changes that are hard to break down, AI pre-screening for basic issues so humans can focus on architecture/business logic, and synchronous review sessions when async back-and-forth is just burning time.

The key insight: good PR structure needs to be paired with removing tooling friction. When review is painful, people default to "LGTM" regardless of how well the story is told.

dkarl• 10 months ago

There's a chicken-and-egg problem with "story-telling commits." Codebases don't contain commits that tell stories, so engineers don't look for them, so engineers don't understand the value of them, so engineers don't learn the skills of rewriting commits, so engineers just squash everything, so codebases don't contain commits that tell stories.

constantcrying• 10 months ago

Great advice, especially I, as a new developer, had to learn.

At first it was a mystery why some other dev reviewing my PR wanted me to split a commit into separate parts. It's also not something you learn in university and definitely requires some git knowledge to do properly.

But after doing it once or twice i definitely understood, that it was also helping *me and forced me into better version control practices. Definitely a good lesson to learn, but more valuable to me was that the job taught me, that I absolutely despise doing software development.

jongjong• 10 months ago

I really enjoy doing code reviews. I don't mind reading other people's code. If you do it enough, it becomes easier. Also it's good for your own coding style if you read other people's code as it helps you to normalize your style an become more minimalist.

I enjoy LLM vibe coding now because I know approximately what code to expect for any given prompt. Basically if the agent doesn't give me code I expect, I usually reject it. It's rare when it comes up with a solution I didn't expect though. I think this is because reading other people's code trains you to be minimalist because it's easier to spot unnecessary complexity when it's from someone else.

I think the skill of being able to read code quickly and applying your intuition is going to be increasingly valuable.

Nowadays, I don't even need to read the whole code to sense when there are issues. I usually have a 'gut feeling' when the code has problems by glancing over it; though of course, I need to invest some effort to list out specific issues because I can't say "this doesn't feel right" in my code review. But even with this, you can become better. Developers who write a certain way tend to make the same kinds of mistakes.

haglin2• 10 months ago

Splitting up a PR is a lot of work, especially if you want each commit to compile, but it would be great if you could categorize changes using 5–10 colors and then have checkboxes that you can toggle to hide code corresponding to a color.

What each color means would depend on the PR, but, for instance, yellow = refactoring, brown = test code, blue = drive-by fix, orange = more efficient data structure etc.

The colors and their meanings could be set by either the author or the reviewers. It would be similar to the file checkboxes that exist today, but in this case, it would be per concept, not per file.

m0008• 10 months ago

How it started: "It would be great if you could categorize changes using 5–10 colors and then have checkboxes that you can toggle to hide code corresponding to a color."

How it's going: https://gitmoji.dev/

IMHO, the novelty wears out fast. Especially when your git history starts looking like a Messages thread.

haglin2• 10 months ago

It would be in the "Files changed" section on GitHub, not in the git history. If you don't want to see the context, you can turn it off.

bvrmn• 10 months ago

If you could make a good "story by commits" I assure you: a combined single commit would be as much readable as multiple. Good story is a result of composable changes highly visible without artificial splitting.

Funny thing I never seen a good PR with meaningful multiple commits. At least you need to be quite proactive in history rewrite to make it presentable. Usually it's mere fix/fix/fix looking amazingly bad without squash.

If you want to comment on this matter, you are welcome to share your github examples.

BrenBarn• 10 months ago

The idea of insisting on a "clean" commit history always rubs me the wrong way. In my mind a commit is like hitting Ctrl-S. It's just saving a checkpoint in your work. And the commit history is supposed to show what actually happened, not a cleaned-up story about what happened.

I can see that it's good to be able to see that cleaned-up story, but I don't think the commit history should be that. There should be something else, a meta-statement about a bunch of commits, that summarizes it. But the commit history is still there if people later need to dig into how something was done. I saw some other comments talking about PRs as this unit, which maybe works, but I've always thought it would make more sense if the VCS had a notion of "commit sets" to which metadata could be attached. Then you could either look at such a set in collapsed form, with a single summary description of the whole set, or you could expand it out to look at the individual commits. In theory these could even be nested, although that might get messy and I think even just two levels would be very useful.

rdiddly• 10 months ago

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.

selinkocalar• 10 months ago

We've definitely fallen into the trap of performative code reviews where everyone feels obligated to find something to comment on

Quickly learned the best code reviews are focused on logic and architecture, not formatting. But it's easy to slip into nitpicking because those comments are easier to write

The Theatre of Pull Requests and Code Review

Related Stories