[HN Gopher] The Theatre of Pull Requests and Code Review
___________________________________________________________________
The Theatre of Pull Requests and Code Review
Author : todsacerdoti
Score : 208 points
Date : 2025-09-25 10:35 UTC (12 hours ago)
(HTM) web link (meks.quest)
(TXT) w3m dump (meks.quest)
| kaapipo wrote:
| I mean, stacked PRs are a thing for a reason
| keriati1 wrote:
| 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...
| epage wrote:
| 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.
| kiitos wrote:
| stacked PRs are wildly reviewer-hostile, please do not do them
| ValleZ wrote:
| 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.
| fidotron wrote:
| 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.
| StilesCrisis wrote:
| 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.
| fidotron wrote:
| > You can compose almost any feature out of many small
| simple changes.
|
| You can?
| StilesCrisis wrote:
| Follow these guidelines and you'll be surprised how far
| you can go.
|
| https://google.github.io/eng-
| practices/review/developer/smal...
| fidotron wrote:
| So you want code added in 100 line sections behind one
| feature flag which we then suddenly turn on?
|
| Isn't that exactly how Google's latest big cloud outage
| happened?
|
| EDIT: referring to
| https://news.ycombinator.com/item?id=44274563
|
| I should add that the idea complexity relates to LoC is
| also nonsense. Everyone that's been doing this for a
| while knows that what kills people are those one line
| errors which make assumptions about the values they are
| handling due to the surrounding code.
| SAI_Peregrinus wrote:
| Yes, and you can waste a _ton_ of a coworker 's time
| doing it.
|
| Say you're upgrading to a new library, it has a breaking
| change to an API. First, add `#if`s or similar around
| each existing change to the API that check for the
| existing library vs the new version, and error if the new
| version is found. No behavior change, one line of code,
| trivial PR. One PR per change. Bug your coworker for
| each.
|
| Next, add calls to the new API, but don't actually change
| to use them (the `#if`s won't hit that condition).
| Another stack of trivial PRs. Your coworker now hates
| you.
|
| Finally, swap the version over. Hopefully you tested
| this. Then make a final PR to do the actual upgrade.
|
| For something less trivial than a single breaking
| upgrade, you can do the same shit. Conditionally compile
| so that your code doesn't actually get used in the
| version you do the PR in, you can split out to one PR per
| character changed! It'll be horrible, everyone will hate
| you, but you can split changes down to ridiculously small
| sizes.
| viraptor wrote:
| > 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 wrote:
| 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.
| viraptor wrote:
| I don't think that's a realistic view of the timeline. I've
| done features as multiple PRs and there are really two
| cases:
|
| 1. I can't submit pieces until I have the final version.
| PRs go up at the same time and can be reviewed one after
| another immediately.
|
| 2. There's a very specific split that makes that feature
| two features in reality. Like adding a plugin system and
| the first plugin. Then the first part gets submitted while
| I still work on the second part and there's no delay on my
| side, because I'm still developing anyway.
|
| Basically, I've never seen the "but if each PR has a delay
| of 1 hour before a reviewer can get to it," getting
| serialised in practice. It's still either one time or
| happening in the background.
| jvanderbot wrote:
| 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 wrote:
| > 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.
| fidotron wrote:
| 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 wrote:
| I always appreciate an extra pair of eyeballs, even on a one-
| liner. Everyone's an idiot sometimes.
| eCa wrote:
| I'm firmly in this boat too. If it's a small change I can
| likely get it reviewed within minutes, if it isn't small it
| should have a review regardless.
| comprev wrote:
| Trust, but verify. We're only human after all :-)
|
| At $DAY_JOB we need approvals from peers due to industry
| regulation.
| goosejuice wrote:
| In my experience, US healthcare, that box can be checked at
| later stages, namely deployment to production. It's a
| choice to add it earlier.
| eCa wrote:
| If it is for checking a box, sure. If it is part of a
| process that aspires to deliver projects with quality and
| with somewhat predictable release dates, that seems way
| too late, imho.
| goosejuice wrote:
| Sadly it often is box checking, code review or not. I'm
| only stating that there is no requirement in US
| healthcare that I'm aware of that requires approvals
| before merging code. Maybe that's not true in other
| industries. But most regulatory frameworks that I'm aware
| of are flexible, ambiguous, on implementation details by
| design.
|
| If you find that outcomes are the same by making
| approvals optional at that stage, then do so with
| accompanied justification.
| t-writescode wrote:
| And a great way to end up leaking customer data from a
| SQL injection or other error that could have easily been
| caught during a more piece-wise analysis and vetting of
| the related code nearer to time of writing.
| gagabity wrote:
| 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-writescode wrote:
| 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 wrote:
| >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.
| xmcqdpt2 wrote:
| 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 wrote:
| > 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.
| stronglikedan wrote:
| > ... the why is important > ... a lot of developers only
| consider the how.
|
| The _why_ is someone else 's job, so the developers should
| just ask them for a blurb to put in the PR for context,
| along with a note to the reviewer to ask that person for
| even more context if necessary.
| pas wrote:
| I think there's a _why_ with regard to the code. Why this
| "how" and not some other "how". (Why did you pick this
| algorithm, this pattern, this solution to the bigger
| business why.)
| ambicapter wrote:
| > 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.
| goosejuice wrote:
| > SEs will mock that as a field of study
|
| Who are these people? I've never encountered that. In my
| experience engineers who aren't great at communication
| freely own up to it.
| scuff3d wrote:
| 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 wrote:
| 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)
| xmcqdpt2 wrote:
| That's a great idea, I should do that. There is another
| team that does something similar but everyone complains
| about it (it's not as good as your template).
| vinnymac wrote:
| 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.
| tcoff91 wrote:
| 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.
| DerArzt wrote:
| I've been doing this as part of my workflow for a few years
| now. My coworkers have expressed appreciation around that
| effort.
|
| A nice side effect is that going through a self review and
| adding comments to the PR has helped me catch innumerable
| things that my coworkers never had to call me on.
| bornfreddy wrote:
| Do you see any added value in adding the comments? I just
| fix the original commits and force push (each dev owns
| their branches at $JOB), but I'm wondering if I'm missing
| something?
| goosejuice wrote:
| I thought everyone did this. I review twice. For each
| commit with -v and finally in GH/GL after I open the PR/MR.
| I often catch something on that last one.
|
| It's rubber ducking.
| manwe150 wrote:
| What is `-v` mean here? I was assuming `git show`, but
| that doesn't seem to have a `-v` parameter.
| ljosa wrote:
| `git commit` has a `-v` option that adds the diff to the
| bottom of the commit message template so you can see it
| while you write the message.
| jeffbee wrote:
| Unfortunately, these days, I am getting a lot of PRs
| where _nobody_ has read the code, which came straight out
| of a robot. This makes me really angry.
| tcoff91 wrote:
| I self review but I don't write comments. I simply fix
| the code as I see the problems that I find self
| reviewing.
| nicksergeant wrote:
| Yep. Self-reviewing your own PRs is a large boost to both
| yourself and the team, and often one of the first things I
| encourage new-ish developers to do.
|
| - 90% of the time when you self-review your own PR, you're
| going to spot a bug or some incorrect assumption you made
| along the way. Or you'll see an opportunity to clean things
| up / make it better.
|
| - Self-reviewing and annotating your reasons/thought
| process gives much more context to the actual reviewer, who
| likely only has a surface level understanding of what
| you're trying to do.
|
| - It also signals to your team that you've taken the time
| to check your assumptions and verify you're solving the
| problem you say you are in the PR description.
| t-writescode wrote:
| Even when I worked for myself and had CodeRabbit help me
| do MRs, I still did a self-review before pushing any
| change to main.
|
| Self-review is very, very helpful.
| sfn42 wrote:
| I always review my own PR before I expect someone else
| to, but I generally don't add comments. I just look it
| over and if I see something I want to fix I fix it.
| Adding comments for things I specifically want feedback
| on or am unsure about seems like a nice addition to the
| process though. I might start doing that too.
| ljm wrote:
| I do it quite often and it's great, because it helps
| contextualise some changes that might not seem to be
| intuitive.
|
| You could argue this is what commits are for, but given how
| people use GitHub and PRs, it gives some extra visibility.
|
| And if you're going to use AI to assist you when writing
| the code I would argue this self-review step is 100%
| mandatory.
| Fishkins wrote:
| Yeah, I never send a PR out without reviewing each commit
| myself and adding GitHub comments when I think it's
| relevant. Sometimes a PR is clear enough that I don't feel
| the need to add comments, though.
| tcoff91 wrote:
| I self review but I don't add comments I just fix the
| problems that I find. I should add clarifying comments.
| disgruntledphd2 wrote:
| I've done this and strongly recommend.
| vinnymac wrote:
| Yep!
|
| Adding context to both your commits and your code review
| tools pull requests / merge requests makes everyone's lives
| better. Including future you, who inevitably is looking at
| the PR or commit in the future due to an incident caused by
| said change.
|
| I have been following this personal rule for well over a
| decade, and have never regretted it.
| atoav wrote:
| 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 wrote:
| 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 wrote:
| 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 wrote:
| 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.
| whstl wrote:
| 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 wrote:
| 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 wrote:
| the mind remains a poor compiler ;p
| franktankbank wrote:
| 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.
| tcoff91 wrote:
| 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.
| epage wrote:
| 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 wrote:
| 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 wrote:
| 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.
| StilesCrisis wrote:
| 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.
| epage wrote:
| Sorry to hear about the use of Perforce. Even the dinosaur
| of a company I used to work at shifted away.
|
| (granted, I know VCS like it are still good for assets)
| skirmish wrote:
| Have you tried using the git adapter to Perforce backend
| [1]? It should let you avoid mega-CLs. When Google used to
| use Perforce, an internal somewhat similar but a bit more
| advanced tool git5 was very popular.
|
| [1] https://git-scm.com/docs/git-p4
| stillworks wrote:
| 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.
| osigurdson wrote:
| 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.
| stillworks wrote:
| The interesting phenomenon is the discovery of gamification
| of such metrics.
|
| I do see the value in breaking down larger chunks of work
| into logically smaller units of work and then produce
| multiple pull requests where needed.
|
| But some people are really clever and influential and
| manage to game these numbers into "apparent success".
| osigurdson wrote:
| It just becomes a huge cycle which is easier for everyone
| involved than doing actual work that benefits your
| customers.
| rjmunro wrote:
| 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 wrote:
| 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 wrote:
| 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 wrote:
| 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".
| bonzini wrote:
| > - 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_fire wrote:
| 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.
| gjgtcbkj wrote:
| 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_fire wrote:
| > 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.
| roguecoder wrote:
| 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.
| kiitos wrote:
| i think you would benefit from some time in an organization
| that was not shitty
| roguecoder wrote:
| 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 wrote:
| Reviewing someone else's large pull request is like having a
| second task in parallel with what you are working on yourself!
| chrisweekly wrote:
| It's not "like" another task, it IS another task!
| osigurdson wrote:
| 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.
| eterm wrote:
| 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".
| sexyman48 wrote:
| 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.
| eterm wrote:
| I don't understand what you're trying to say.
| mystifyingpoi wrote:
| Well, then just don't play the game. Make a decision in the
| team, that everyone accepts everyone's PR immediately
| without any review. At least you won't have to wait.
| sexyman48 wrote:
| I'm semi-retired now, but I spent most of my career at a
| Bell Labs-caliber place (I was the dumbest person there)
| before "PR" and "code review" became part of the lexicon,
| and yes, everyone was good enough not to mess things up
| too badly.
| Supermancho wrote:
| > 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 wrote:
| 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 wrote:
| 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.
| g-b-r wrote:
| > However, in more than a decade of software development, I
| don't think I've ever got much use out of commit messages.
|
| > normally the commit message has no relevant information
|
| Maybe that's why you've never got much use of them?
|
| If your colleagues or yourself wrote better commits, they
| could have been of use.
|
| An easily readable history is most important in open source
| projects, when ideally random people should be able to
| verify what changed easily.
|
| But it can also be very useful in proprietary projects, for
| example to quickly find the reason of some code, or to
| facilitate future external security reviews (in the very
| rare case where they're performed).
| ziml77 wrote:
| 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.
| bonzini wrote:
| You confuse "include all the broken work-in-progress
| commits" with "split the independent parts that get you
| from A to B".
| whstl wrote:
| 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.
| g-b-r wrote:
| Merge commits allow you to nest series of commits, they're
| far from always bad
| Supermancho wrote:
| > 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 wrote:
| 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.
| conartist6 wrote:
| 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 wrote:
| 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.
| roguecoder wrote:
| 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 wrote:
| You're making the faulty assumption the whole project won't
| get scrapped
| conartist6 wrote:
| 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 wrote:
| 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
| gjgtcbkj wrote:
| 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.
| sexyman48 wrote:
| 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.
| roguecoder wrote:
| 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.
| sexyman48 wrote:
| Consistently? You don't know that. There are just as many
| successful one-man shows -- with the important caveat that
| they lack redundancy.
| roguecoder wrote:
| 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.
| gjgtcbkj wrote:
| 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-r wrote:
| You only know poor developers then, I guess
| watwut wrote:
| 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.
| osigurdson wrote:
| 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.
| roguecoder wrote:
| 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.
| davedx wrote:
| 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.
| CuriouslyC wrote:
| 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 wrote:
| > 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.
| tcoff91 wrote:
| Just use jj instead of git and cut your rebasing time by 95%.
|
| Suddenly rebasing a stack of branches becomes 1 command.
| sebk wrote:
| 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.
| scuff3d wrote:
| 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.
| roguecoder wrote:
| That's one of the challenges with making changes all at
| once: it is a lot easier for one thing going wrong to
| suddenly result in thousands of lines of changes.
|
| One technique I use when I find that happening is to check
| out a clean branch, and first make whatever structural
| change I need to avoid that rabbit hole. That PR is easy to
| review, because it doesn't change any behavior and there
| are tests that verify none of my shuffling things around
| changed how the software behaves (if those tests don't
| exist, I add them first as their own PR).
|
| Once I've made the change I need to make easy, then the PR
| for the actual change is easy to review and understand.
| Which also means the code will be easy to understand when
| someone reads it down the line. And the test changes in
| that PR capture exactly how the behavior of the system is
| changed by the code change.
|
| This skill of how to take big projects and turn them into a
| series of smaller logical steps is hard. It's not one that
| gets taught in college. But it lets us grow even large,
| complex code bases that do complex tasks without getting
| overwhelmed or lost or tangled up.
| scuff3d wrote:
| That makes sense. Reading your comment got me thinking
| some of the issue might be that I have always worked on
| somewhat immature projects. Either R&D or greenfield
| projects. Which is super nice in a whole lot of ways, but
| a lot of times I don't know what the final shape of the
| changes to the rest of the system are going to be,
| because that part of the system itself isn't well
| established yet. So it evolves throughout whatever I'm
| doing. Which would make it difficult to break them off
| and work them in a different branch.
|
| Maybe there's a partial solution if I can keep those
| commits clean and separate in the tree. And then when I'm
| done reorder things such that those all happen as a block
| of contiguous commits.
| Qerub wrote:
| There's a nice Manning book from 2014 about this way of
| working named The Mikado Method.
| vault_ wrote:
| > - 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.
| kiitos wrote:
| 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 wrote:
| 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.
| tcoff91 wrote:
| A stack of PRs is much better for reviewers than a single
| massive PR.
|
| Use jujutsu and then stacking branches is a breeze
| nerdponx wrote:
| 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.
| scuff3d wrote:
| > 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 wrote:
| 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.
| roguecoder wrote:
| 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.
| roguecoder wrote:
| 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 wrote:
| 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.
| osigurdson wrote:
| 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-r wrote:
| If security or bugs don't matter much for your project, sure
| osigurdson wrote:
| I don't think "LGTM" helps with either of those. Making
| someone personally responsible is the real unlock here.
| g-b-r wrote:
| Personally responsible for the review?
|
| For sure if you can say LGTM without even looking at
| anything it doesn't make much sense
| mtlynch wrote:
| 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 wrote:
| 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-r wrote:
| > 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.
| mtlynch wrote:
| > _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.
| g-b-r wrote:
| > 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.
|
| After a few months it will probably be as useful to you as
| to anyone else; if you only use commits as some sort of
| help while developing, you might as well just squash them
| before making a PR.
|
| > 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?
|
| Yes, except that it doesn't matter if it's their thought
| process or not; it doesn't take a ton of time to reorder
| your commits, if you had some care for them in the first
| place.
|
| It doesn't make much sense to place failed attempts in a
| series of commits (and of their reverts), just go back to
| the last good commit if something was a dead end (and save
| the failed attempt in a branch/tag, if you want to keep it
| around).
|
| > 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.
|
| It's not the individual commits themselves that you need to
| review (although you can do that, if you place a lot of
| value to good histories); going through each commit, if
| they're indeed not just random snapshots but have been made
| thoughtfully, can let you review the PR a lot faster,
| because they'll be small changes described by the commits'
| messages.
| kiitos wrote:
| > if you only use commits as some sort of help while
| developing, you might as well just squash them before
| making a PR.
|
| yeah for sure you want to squash-merge every PR to main,
| right?
|
| commits are just commits, there is no moral value to
| them, there is no "good history" or "bad history" of
| them, whether or not they're "made thoughtfully" isn't
| really interesting or relevant
|
| git is just a tool, and commits are just a means to an
| end
| g-b-r wrote:
| > yeah for sure you want to squash-merge every PR to
| main, right?
|
| Oh god you're serious?
|
| > git is just a tool, and commits are just a means to an
| end
|
| To more ends than you realize, probably, if you put some
| care in making them
| kiitos wrote:
| i just don't use commits like you do, and that doesn't
| mean i'm being less careful or less thoughtful, or that
| my changes are worse than yours
|
| commits are what i say they are, nothing more or less
| stack_framer wrote:
| 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."
| CuriouslyC wrote:
| Don't worry, if you actually increase AI usage your team will
| be forced to automate code review, either explicitly or
| implicitly.
| meesles wrote:
| 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 wrote:
| 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 wrote:
| 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 wrote:
| 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 wrote:
| This is the drawback of teaching some software developers
| social skills.
|
| /s
| bonzini wrote:
| Kinda like "the queen's duck"
| (https://bwiggs.com/notebook/queens-duck/).
| iamleppert wrote:
| 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 wrote:
| 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
| tcoff91 wrote:
| 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 wrote:
| 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 wrote:
| 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 wrote:
| 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 wrote:
| 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.
| ziml77 wrote:
| 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_zhang wrote:
| 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-r wrote:
| I mean, the reviewer is not supposed to know everything, right?
|
| Uhm he kind of is, if you want a proper review...
| roguecoder wrote:
| "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_zhang wrote:
| > 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.
| kiitos wrote:
| more or less the entire point of code review is to ensure
| that the reviewer fully understands the code that they
| review
|
| i know that lots of orgs don't do it this way, but it's
| really important to make this point very clear: those orgs
| are wrong, and pathological, no matter how many of them
| there are
| danielfalbo wrote:
| 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 wrote:
| > 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 wrote:
| 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 wrote:
| 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.
| bob1029 wrote:
| 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?
| roguecoder wrote:
| 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.
| bob1029 wrote:
| > 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 wrote:
| 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 wrote:
| 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 wrote:
| 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 wrote:
| 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.
| roguecoder wrote:
| 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 wrote:
| 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 wrote:
| 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-r wrote:
| PRs only live on GitHub, what happens if it gets shut down or
| it accidentally loses some data?
| kiitos wrote:
| the entire repo lives only on github as well, there is no
| meaningful difference between git commits and PR comments
| in a github-hosted repo
| niko_dex wrote:
| 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 wrote:
| 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.
| gagabity wrote:
| 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.
| WorldMaker wrote:
| 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 wrote:
| Can you describe a little more about why you walk through
| individual commits instead of just reviewing the latest commit
| only?
| XorNot wrote:
| Because the idea is that each commit that you submit is a
| coherent unit of work. That's why we have commit messages.
| WorldMaker wrote:
| 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.)
| kiitos wrote:
| > But yes, not everyone can or will write good commits.
|
| some people treat commits as meaningful units of
| independent review, and some people treat them as
| savepoints and the PR as the only meaningful unit of
| review, it's a distinction of process, not purity -- both
| approaches are totally fine, one is not better than the
| other
|
| git and commits and prs are means, not ends
| jimbobimbo wrote:
| That's exactly it. What happens in a private branch is an
| implementation detail and reflects personal work style.
| Policing that is counterproductive.
| WorldMaker wrote:
| No one said anything about "policing" anything. I'm not
| telling anyone _how_ to write PRs, I 'm just suggesting
| that if we had better tools we'd get "better" PRs for
| values of "what happens in _this_ 'private' branch is
| _more_ than an implementation 'detail' but a useful
| story and a useful documentation of the process". You
| don't need to agree that is "objectively" or
| "universally" a "better" way to make PRs for everyone and
| every project, but I'd hope you could at least respect
| that it's a nice goal that some of us have at least some
| of the time and why we would like PR tools that respect
| _that_ approach as much as they seem to already respect
| your "no one cares how the sausage is made" approach.
| DenisM wrote:
| 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 wrote:
| 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).
| WorldMaker wrote:
| 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.
| KronisLV wrote:
| > 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.
| roguecoder wrote:
| 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.
| KronisLV wrote:
| > 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 wrote:
| 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 wrote:
| 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.
| roblh wrote:
| 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 wrote:
| 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-writescode wrote:
| 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-writescode wrote:
| 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?
| crazygringo wrote:
| 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 wrote:
| you could probably write an extension to accomplish this in a
| couple of days with GPT-5 now
| TiredGuy wrote:
| 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
| conradludgate wrote:
| 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.
| kiitos wrote:
| 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!
| conradludgate wrote:
| Hard disagree. I've found this way of working benefits both
| me and my colleagues.
|
| I'm able to work on long lived branches without getting
| into conflict hell because when I rebase, I'm only dealing
| with small conflicts.
|
| I'm able to slim down initially large changes into smaller
| total diffs because I realised one change wasn't actually
| necessary, but only because I took the time to reflect on
| the code and separate the concerns
|
| Being able to separate your code into smaller units is a
| really great tool, and helps you really understand your own
| code changes in a new light. Amusingly despite me often
| rewriting the same code 3 times, I feel like I've never
| been more productive (and no, I don't use any LLMs)
| mnembrini wrote:
| 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 wrote:
| 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.
| roblh wrote:
| 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.
| herval wrote:
| 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.
| leetrout wrote:
| 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.
| BobbyJo wrote:
| 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 "
| collingreen wrote:
| This is a really good way to put this. The "just because
| the do they same thing right now doesn't mean they _do the
| same thing_" concept is hard to convey!
| MoreQARespect wrote:
| 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.
| herval wrote:
| Sure.
| BobbyJo wrote:
| > 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.
| MoreQARespect wrote:
| Give me one example then. One is all it takes to disprove a
| rule.
|
| Im fairly sure that I could explain how to break up _any_
| long PR in a sensible way. Parent thinks _couldnt_ be done,
| so do you - what is an example?
|
| The only exception i can think of is something where 99.9%
| of the changes are autogenerated (where i wouldnt really be
| reading it carefully anyway, so the length is
| immaterial...).
| 1dom wrote:
| I agreed with you initially.
|
| > I'm fairly sure that I could explain how to break up
| any long PR in a sensible way. Parent thinks couldnt be
| done, so do you - what is an example?
|
| To me, when I meet experts in any field, the quality that
| stands out isn't that they do everything to expert level,
| it's that they get everything done as they said they
| would. Sometimes that means big PRs, because that's the
| environment created, and the expert finds the way to get
| the job done.
|
| I'm not doubting you _could_ break up any PR into a
| shorter one. But that's kind of the point of an expert:
| they recognise what makes sense to do in reality, rather
| than just doing something because it's best practice and
| expecting everyone else to do the same.
|
| They ultimately get the thing done how they said they
| would.
| horsawlarway wrote:
| In my experience - this one is the correct one. Make a
| commitment, keep the commitment, stay responsible for the
| commitment afterwards.
|
| This whole chain is like arguing on how tidy your desk
| should be. Some people like it fastidious to the nth
| degree. Some people prefer a little mess.
|
| In neither case does that preference really matter much
| compared to all the other things a real job entails.
| alganet wrote:
| > they get everything done as they said they would
|
| This. It saves everyone's time.
| MoreQARespect wrote:
| >I'm not doubting you _could_ break up any PR into a
| shorter one. But that's kind of the point of an expert:
| they recognise what makes sense to do in reality
|
| I have seen plenty of huge PRs which were more trouble
| than they were worth to break up after discovery. At some
| point it becomes like unbaking a cake. It's a trade off.
|
| Ive just never thought when I saw any of them that there
| wasnt a _more_ practical way to get there with a bunch of
| smaller PRs.
|
| Unlike dealing with an already existent large PR this
| isnt really a trade off thing - there are basically
| almost no circumstances when it is _preferable_ to review
| one 1000 line code change instead of 4x self contained
| 200 line changes.
| overfeed wrote:
| > Im fairly sure that I could explain how to break up any
| long PR in a sensible way. Parent thinks couldnt be done,
| so do you - what is an example?
|
| Not _couldn 't_ - but _shouldn 't_, such as when there's
| tight coupling across many files/modules. As an example,
| changing the css classes and rules affecting 20+
| components to follow updated branding should be in one
| big PR[1] _for most branching strategies._
|
| Sometimes it's easier to split this into smaller chunks
| and front-load reviews for PRs _into_ the feature branch,
| and then merge the big change with no further reviews,
| which may go against some ham-fisted rule about merging
| to main. Knowing when to break rules and why, ownership,
| and caring for the spirit of the law and not just the
| letter are what separates mid-levels from seniors.
|
| 1. Or changeset, if your version control system allows
| stacking.
| status_quo69 wrote:
| You can and should break that up because I'm probably
| going to want to see screenshots to ensure that the
| branding changes make sense in context and everything
| looks consistent.
|
| How would you do this? You'd either
|
| 1. Create N pull requests then merge all of them together
| into a big PR that would get merged into mainline at once
| 2. Do the same thing but do a bit of octopus merging
| since git merge can take multiple branches as arguments.
| Since most source control strategies are locked down,
| this isn't usually something that I can tell my juniors
| to do
|
| The point of breaking things down like this is to
| minimize reviewer context. With bigger PRs there's a
| human tendency to try and hold the whole thing in your
| head at once, even if parts of the pull request are
| independent from others.
| overfeed wrote:
| > The point of breaking things down like this is to
| minimize reviewer context.
|
| This principle is much more important than some rule that
| says "Merges to main should not be more than 150 lines
| long". Sticklers for hard-and-fast rules usually haven't
| achieved the experience to know that adhering to
| fundamental principles will occasionally direct you to
| break the rules.
| ants_everywhere wrote:
| > Merges to main should not be more than 150 lines long
|
| This can be done by allowing a flag in the commit message
| that bypasses the 150 line long (or whatever example)
| rule in the CI that enforces it. Then the reviewers and
| submitter can agree whether or not it makes sense to
| bypass the rule for this specific case.
|
| In many cases like this, it's okay to override a rule if
| the people in charge of keeping the codebase healthy
| agree it's a special case.
| kiitos wrote:
| minimizing reviewer context is one thing a PR can try to
| do, but it's not like that's any kind of universal most-
| important metric that every PR needs to optimize for, in
| fact very often minimizing reviewer context is in direct
| tension with making changes that are holistic and
| coherent
|
| code review is meant to take time
| MoreQARespect wrote:
| >Not couldn't - but shouldn't, such as when there's tight
| coupling across many files/modules.
|
| No, this is a pretty classic example of where you can
| break up the work by first refactoring out the tightly
| wound coupling in one PR before making the actual (now
| simpler/smaller) change in a second PR.
| thayne wrote:
| A widely used class has a bad API. You refactor it to
| make a cleaner API, but your change isn't backwards
| compatible. Your options are:
|
| - Change everything all at once. This creates a large PR.
| - Split it up into multiple small PRs. Now your
| individual PRs don't compile, and make less sense on
| their own. - Create a new class, and then split up
| multiple PRs that transition code to use the new class,
| and then finally a PR to remove the old class. This is
| more work for both the author and the reviewer and the
| individual PRs are harder to understand on their own.
| mgkimsal wrote:
| API changes breaking BC feel like they should be using
| versioning. I don't see enough people putting versioning
| support in to their API stuff at the outset. I've been
| chastised for doing that with "YAGNI". And then... one
| day, we do need it, and trying to introduce versioning
| support becomes... that much harder.
| degamad wrote:
| The context here was a class API, not a REST API, so
| versioning is not relevant.
| wpollock wrote:
| This is interesting. I believe one way to deal with such
| breaking changes is to have multiple PRs, where the
| breaking change in each is hidden (protected) by a
| feature flag and tested by unit tests. Once all the PRs
| are committed, end to end testing can be done by enabling
| the flag. Any problems in production can be quickly
| reverted by disabling the flag. Eventually, a final PR
| removes the now-useless flag.
|
| Of course, your mileage may vary; this technique is
| certainly not suitable for all breaking changes or all
| workfkows.
| BobbyJo wrote:
| Lets say you have an api bug where you are allowing
| clients to ask for data with a very expensive query, and
| you want to dissallow that behavior. I think it makes
| more sense to change both the api spec (remove the
| endpoint) and the backend (dissallow queries of that kind
| in the future) in one go. That way you can note in the
| one PR exactly why both changes are made, referencing
| whatever bug/postmortem. Making two PRs that separately
| look like fixes to the issue can be confusing later, and
| don't really buy you any clarity in the PR itself.
|
| Is it possible to break them up? Sure. Is it _better_ to
| do so? I don 't think so.
|
| Also, for clarity, neither myself, nor op, every said
| _couldn 't be done_.
| fizzynut wrote:
| A new feature that fundamentally changes the way a lot of
| code is structured.
|
| A group of features that only combined produce a
| measurable output, but each one does not work without the
| others.
|
| A feature that will break a lot of things but needs to be
| merged now so that we have time for everyone to work on
| fixing the actual problems before deadline X as it is
| constantly conflicting every day and we need to spend
| time on fixing the actual issues, not fixing conflicts.
| singpolyma3 wrote:
| I think "doesn't work in every case" is true for basically
| every rule of course. But 99% of people in the industry are
| not qualified to make that call because they will always
| choose "not" out of laziness rather than because it
| actually wasn't a good idea
| throwaway314155 wrote:
| This doesn't match my experience and I assume is deeply
| cultural and subjective.
| mountainriver wrote:
| 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 wrote:
| >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 wrote:
| > "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 wrote:
| > 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 wrote:
| 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.
| sodapopcan wrote:
| Not my experience. Often the single commit is all the
| context I need. If it's not, follow the merge to the ticket
| number to get more context.
| singpolyma3 wrote:
| 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 wrote:
| > 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.
| jonahx wrote:
| > 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.
| IshKebab wrote:
| 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.
| mathstuf wrote:
| This is true _r_ now that `git bisect --first-parent`
| exists. But it didn 't always. And even then, there are
| times you find out that there is "prep work" to land your
| feature. And a PR just to do some deck chair moving that
| makes a follow-up commit easier is kind of useless. I
| _have_ done prep work as a separate PR, but this is usually
| when it is more extensive than the feature _and_ it is
| worthwhile on its own.
|
| Another instance is a build system rewrite. There was a
| (short) story of the new system itself and then a commit
| per module on top of that. It landed as 300+ commits in a
| single PR. And it got rebased 2-3 times a week to try and
| keep up as more bits were migrated (and new infra added for
| things other bits needed). Partial landing would have been
| useless and "rewrite the build system" would have been
| utter hell for both me developing and anyone that tries to
| blame across it if it hadn't been split up at least that
| much.
|
| Basically, as with many things in software development,
| there are no black-and-white answers here.
| keybored wrote:
| > 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 perfect illustration of a backwards mindset. If this made
| sense then the standard or least common denominator PR tool
| would work better with many small PRs, which here also
| means they must be able to depend on each other. (separate
| PRs!!!) So is it?
|
| > Also I really really wish GitHub had proper support for
| stacked PRs.
|
| No. It doesn't even support it.
|
| So how does this make sense? This culture of people wanting
| "one PR" for each change, and then standard PR tool that
| everyone knows of _doesn't even_ support it? What's the
| allegiance even to, here? Phabricator or whatever the
| "stacked" tools are?
|
| It's impressive that Git forge culture has managed to
| obfuscate the actual units of change so much that
| heavyweight PRs have become the obvious--they should be
| separate PRs!--unit of change... when they don't even
| support one-change-then-another-one.
| IshKebab wrote:
| Yeah it's kind of infuriating really. It's not like it's
| an uncommon workflow either. Everywhere I've worked
| people end up with PRs that just say "this depends on
| this other PR; ignore the first commit".
|
| Gitlab _kind of_ supports it - if your second PR 's
| target branch is the first PR then it will only show you
| the code from the second PR and it will automatically
| update the target branch to master when the first one
| gets merged. I wouldn't say it's first class support
| though.
|
| Sapling sort of has support for making it work on GitHub:
| https://sapling-scm.com/docs/addons/reviewstack/
|
| And there was some forge that supports Jujutsu that has
| proper first class support, but I can't find it now.
|
| Anyway it's a very useful workflow that lots of people
| want and kind of insane that it isn't well supported by
| GitHub.
|
| To be fair I can't remember the last time GitHub
| introduced any really new features. It's basically in
| maintenance mode.
| m000 wrote:
| > 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.
|
| I think that whether clean history has a point, _really_
| depends on how deep are you refinement sessions. And
| perhaps a bit on the general health of your codebase.
|
| If you don't do refinement with your editors open and grind
| tickets into dust, there will be side-changes adjacent to
| each PR which are not directly related to the ticket. These
| are better to have their own commit (and commit message).
| fillmore wrote:
| 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.
| sodapopcan wrote:
| Also replying to echo this. Hate these blanket statements.
| sodapopcan wrote:
| > 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.
| kiitos wrote:
| git is a means, not an end
|
| commits mean precisely what their author intend them to
| mean, nothing more
|
| if you squash-merge every PR then history is clean where it
| matters
| sodapopcan wrote:
| To quote my least favourite HN response: "No."
| m000 wrote:
| Such developers should be condemned to work with CVS until
| they repent for their sinful statements.
| William_BB wrote:
| 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.
| ownagefool wrote:
| Yeah, I agree with both you and the GP. There's a mess of
| commits that usually don't matter because mostly only the
| before and after level of an actual viable PR does, ergo I
| squash them.
|
| I'm cool with other reasonble approaches though, but I'm
| pretty over pointless hoops because someone says so.
| singpolyma3 wrote:
| If the commits don't matter why did you make them
| separate to begin with?
| ViewTrick1002 wrote:
| "Updated something tiny and ran CI again until it failed
| on some other step"
|
| Together with backing up your work.
|
| Sure you can keep amending your last commit but whenever
| you detour to another problem in the same PR that turns
| into a mess.
|
| Easier to just treat the PR as the atomic unit of work
| and squash away all that intermediate noise.
|
| It also ensures that CI will pass on every commit on the
| main branch.
| sfn42 wrote:
| I'm not the person you asked, but I often don't. I
| personally don't care about git history - I read code not
| commit messages. I don't really care how it got the way
| it is I care about what it is, maybe once in a blue moon
| there could potentially be some useful information in a
| commit message but it's not enough.
|
| So, I'll often just make lots of changes and then commit
| them all at once with some vague commit message and
| that's that. Nobody cares. If I want to tell people why
| the code is the way it is I'll just add a comment.
|
| This is how I work. I have tried to be more disciplined
| with commits and stuff like that but I find that it just
| slows me down and makes the work feel more difficult. I
| also frequently just forget and find myself having made
| lots of changes without any commits so then I have to
| retroactively split it up into commits which can be
| difficult too. So I'd rather just not worry about it,
| focus on getting good work done and move on rather than
| obsess over a git history that's unlikely to ever be read
| by anyone. I realize that's a self-fulfilling prophecy in
| that it'd be more likely to be read if it was useful and
| well done but it's not just me. If I was in a team where
| everyone did it really well I'd try to keep my own work
| up to par. But usually I'm the one who cares most about
| how we do things and this just doesn't seem important to
| me.
| imiric wrote:
| You should care because if the author cared enough to make
| descriptive atomic commits, they will help you understand
| _why_ a particular change was done. This can often avoid
| unnecessary discussions.
|
| And, no, PRs are not necessarily an atomic level of work.
| While they should contain a single feature, fix, etc.,
| sometimes that work can span multiple commits.
|
| If the PR includes superfluous commits, then they should be
| squashed into the appropriate commit. Squashing the entire
| PR when it includes multiple changes is simply a bad
| practice. It's bad because you lose all the history of how
| the overall change was done, which will be useful in the
| future when you need to do a blame, cherry pick, bisect,
| etc.
|
| It's surprising to me how many developers misunderstand the
| value of atomic commits, or even what they are. And at the
| same time, it's exhausting having this discussion every
| time that happens, especially if there is continued
| pushback.
|
| I am not against people having their preferred way of using
| VCS tools. As long as it works for their team, that's fine.
| But there are certain best practices that simply help
| everyone in the long-term, including the author, that I'm
| baffled whenever they're willfully ignored. I can't help
| but think that it's often done out of laziness, and lack of
| discipline and care into the work they do, which somehow
| becomes part of their persona as they gain more experience.
| mvdtnz wrote:
| > You should care because if the author cared enough to
| make descriptive atomic commits, they will help you
| understand why a particular change was done. This can
| often avoid unnecessary discussions.
|
| That's what the description field is for. I never, ever
| inspect the "commits" tab in a PR unless I see some
| lucicrous number on it. And even then it's just to see
| what the heck happened.
|
| > If the PR includes superfluous commits, then they
| should be squashed into the appropriate commit.
|
| This happens on merge if your Github is set up correctly.
|
| > Squashing the entire PR when it includes multiple
| changes is simply a bad practice.
|
| The bad practice is the PR changing multiple distinct
| things.
|
| > It's bad because you lose all the history of how the
| overall change was done, which will be useful in the
| future when you need to do a blame, cherry pick, bisect,
| etc.
|
| It's not.
| imiric wrote:
| > That's what the description field is for.
|
| No. The PR description is for describing the overall
| change, which, again, may include multiple commits. The
| description can also include testing instructions,
| reviewing suggestions, and other information which is not
| suitable for a commit message.
|
| PR descriptions can be edited and updated during the
| review, which can be helpful. A commit message is
| immutable, and remains as a historical artifact.
|
| Also, when I'm working on a code base, the last thing I
| want is to go hunting for PRs to get context about a
| specific change. The commit message should have all the
| information I need directly in the repo.
|
| > I never, ever inspect the "commits" tab in a PR unless
| I see some lucicrous number on it.
|
| And... you're actually proud of this? Amazing.
|
| Have you ever read a descriptive commit message? Do you
| even know what they look like?
|
| I'm taken aback by the idea that there are developers who
| would take the time and effort to write a detailed commit
| message, only for others to not only never read it, but
| to be proud of that fact. Disgraceful.
|
| > This happens on merge if your Github is set up
| correctly.
|
| No. This is what I mean about developers not
| understanding what atomic commits even are. There are
| commits that will be done during a review, or as ad-hoc
| fixes, which indeed shouldn't exist when the PR is
| merged. _But this doesn 't mean that the entire PR should
| be squashed into a single commit._
|
| Those useless commits should instead be squashed into the
| most relevant commit, which is straightforward if you
| create `--fixup` commits which can then be automatically
| squashed with `rebase --autosquash`.
|
| But the PR may ultimately end up with multiple _atomic_
| commits, and squashing them all into a single commit
| would nullify the hard work the author did to keep them
| atomic in the first place.
|
| If you configure GitHub to always squash PRs, or to
| always create a merge commit, or to always rebase, you're
| doing it wrong. Instead, these are decisions that should
| be made on a case-by-case basis for each PR. There are
| situations when either one of them is the best approach.
|
| > The bad practice is the PR changing multiple distinct
| things.
|
| Great. I'm sure you enjoy the overhead of dealing with a
| flood of small PRs that are all related to a single
| change, when all of it could be done in a single PR with
| multiple commits. This is easier to review, discuss, and
| merge as a single unit, rather than have it spread out
| over multiple PRs because of a strict "one PR-one commit"
| policy.
|
| All that rule does, especially if you have PR squashing
| enabled by default, is create a history of bloated
| commits with thousands of lines of unrelated changes,
| that are practically useless for cherry picking,
| bisecting, and determining why a specific change was
| done, _which is the entire point of commits_. Good luck
| working on that codebase.
|
| > It's not.
|
| k.
| singpolyma3 wrote:
| The PR UI on GitHub definitely leads to treating it as the
| unit of work. I consider this unfortunate for the most
| part, but the basic side effect is that I'll often end up
| submitting every commit as a separate PR so they actually
| get looked at
| William_BB wrote:
| Respectfully, I disagree. I understand people have vastly
| different experiences and preferences. In my ideal world,
| a PR is a unit of work that has an in-state and an out-
| state. I don't have to see an initial commit within a PR
| with a full fledged spec and then wonder if any future
| commits within the same PR overrode those changes. A PR
| will rarely be clean from the start.
|
| The way it appears to me, if there's multiple commits
| submitted as separate PRs, then maybe the PR wasn't so
| atomic to begin with.
| singpolyma3 wrote:
| Indeed, I agree. If a PR has multiple commits and those
| commits are atomic then by definition the PR is not
| atomic.
| DSMan195276 wrote:
| > I'd go read the original PR and the discussion that took
| place.
|
| Until your company switches code repos multiple times and
| all the PR history is gone or hard/impossible to track
| down.
|
| I will say, I don't usually make people clean up their
| commits and also usually recommend squashing PRs for any
| teams that aren't comfortable with `git`. When people do
| take the time to make a sensible commit history (when a PR
| warrants more than one commit) it makes looking back
| through their code history to understand what was going on
| 1000% easier. It also forces people to actually look over
| all of their changes, which is something I find a lot of
| people don't bother to do and their code quality suffers a
| lot as a result.
| agentultra wrote:
| It also enables bisect to work properly.
|
| Bisecting on squashed commits is not usually helpful. You
| can still narrow down to the offending commit that
| introduced the error but... it's somewhere in +1200/-325
| lines of change. Good luck.
| siva7 wrote:
| If your PR is + 1000 code lines long, you already made a
| mistake at the requirements and planning stage (like many
| teams do).
| lovich wrote:
| Or finding what bug was reintroduced in a +13/-14398
| jonahx wrote:
| > PR is the atomic level of work.
|
| The atomic level of work should be a single, logically
| coherent change to the codebase. It's not managerial, it's
| explanatory.
|
| As you work things naturally arise. Over here a reformatted
| file, over there comments to clarify an old function that
| confused you, to help the next developer who encounters it.
| Cleaning, preparatory refactoring that is properly viewed
| as a separate action, and so on. Each of these is a
| distinct "operation" on the codebase, and should be
| reviewed in isolation, as a commit.
|
| Some of these operations have _nothing_ to do with the new
| feature you 're adding. And yet creating separate PRs for
| each of them would be onerous to your reviewers and spammy.
| Clean, atomic history lets you work naturally while still
| telling a clear story about how the code changed, both for
| reviewers and future developers.
| illuminator83 wrote:
| I always tell my engineers to create atomic commits and we
| usually review commit by commit. Obviously commits like
| "fixed review comments" or "removed some left-over
| comments" or "fixed typo" should not be pushed into a PR
| you asked others to review. I expect people to understand
| how to clean their commit history - if they don't I teach
| them. The senior people who are capable of structured work
| - e.g. are used to contribute open source projects - do it
| anyway. Because messiness is usually not tolerated by
| maintainers of important projects.
|
| You find people how aren't able to craft clean commits and
| PRs usually thrive in environments in which people are
| either work mostly alone or in which cooperation is
| enforced by external circumstances (like being in the same
| team in a company). As soon as developers many are free to
| choose whom to associate with and whose code they accept -
| rules are usually made and enforced.
| siva7 wrote:
| 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.
| echelon wrote:
| I had a coworker that split every big change into five or
| six PRs with a directed graph image attached showing the
| review order.
|
| This person's code was a maintain of "clever abstractions"
| that reduced maintainability and meant that the only
| effective maintainer was this person.
|
| I was not a fan.
|
| Other teams do just fine without all of the ceremony.
| mdavid626 wrote:
| 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 wrote:
| > - 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.
| bornfreddy wrote:
| 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 wrote:
| > 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 wrote:
| > "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.)
| epage wrote:
| 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 wrote:
| 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.
| muxator wrote:
| > - 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.
| lamontcg wrote:
| 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 wrote:
| 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 wrote:
| 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-more wrote:
| > 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.
| bob1029 wrote:
| > 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).
| smcameron wrote:
| > I've got an internal alarm that starts to go off
| somewhere around 72 hours.
|
| Nah, in my experience, if you've got good commit hygiene
| you can often merge even ancient commits.
|
| Here's a pretty hefty commit I merged _five years_ after it
| was originally written, converting a ~100k line codebase
| from GTK to SDL2, written in 2015, committed in 2020, with
| tons of development in between, with "10 files changed,
| 777 insertions(+), 804 deletions(-)"
|
| https://github.com/smcameron/space-nerds-in-
| space/commit/4ab...
|
| I was expecting it to be a bit of a nightmare, but it
| really wasn't bad at all.
| zzzeek wrote:
| 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.
| keybored wrote:
| > - 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.
| smcameron wrote:
| > - "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.
| Illniyar wrote:
| 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 wrote:
| 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 wrote:
| 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 wrote:
| 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.
| crazygringo wrote:
| 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 wrote:
| 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.
| lo_zamoyski wrote:
| 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 wrote:
| 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 wrote:
| 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 wrote:
| 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.
|
| or
|
| Hey jane/john did you remember to check x before this? Yes! Ok,
| blindly approve.
| flatline wrote:
| 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 wrote:
| 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 wrote:
| 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 wrote:
| 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 ?
| kiitos wrote:
| 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 wrote:
| 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 wrote:
| This post hits on something crucial - the difference between
| performative code review and substantive collaboration. The
| "theatre" metaphor is spot-on. The core issue Sasa 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 wrote:
| 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 wrote:
| 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 wrote:
| 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.
___________________________________________________________________
(page generated 2025-09-25 23:01 UTC)