Hacker News Board Hacker News, browsed like a classic message board
Live data from Hacker News

The Theatre of Pull Requests and Code Review

meks.quest

Post reply on HN
Print view View original

301–310 of 431 posts

Re: The Theatre of Pull Requests and Code Review

#301

Post by zzzeek »

herval wrote: post #150

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 necess…

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.

Re: The Theatre of Pull Requests and Code Review

#302

Post by siva7 »

jonahx wrote: post #200
herval wrote: post #150

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 necess…

> 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 s…

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.

Re: The Theatre of Pull Requests and Code Review

#303

Post by jimbobimbo »

kiitos wrote: post #279
WorldMaker wrote: post #224

Earlier quoted context omitted.

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

> 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

That's exactly it. What happens in a private branch is an implementation detail and reflects personal work style. Policing that is counterproductive.

Re: The Theatre of Pull Requests and Code Review

#304

Post by alganet »

1dom wrote: post #192
MoreQARespect wrote: post #180

Earlier quoted context omitted.

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

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 creat…

> they get everything done as they said they would

This. It saves everyone's time.

Re: The Theatre of Pull Requests and Code Review

#305

Post by conartist6 »

roguecoder wrote: post #181
conartist6 wrote: post #29

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

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.

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.

Re: The Theatre of Pull Requests and Code Review

#306

Post by keybored »

herval wrote: post #150

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 necess…

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

Re: The Theatre of Pull Requests and Code Review

#307

Post by [deleted] »

siva7 wrote: post #302
jonahx wrote: post #200

Earlier quoted context omitted.

> 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 s…

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.

[deleted]

Re: The Theatre of Pull Requests and Code Review

#308

Post by [deleted] »

kiitos wrote: post #296
g-b-r wrote: post #216

Earlier quoted context omitted.

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

> 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 m…

[deleted]

Re: The Theatre of Pull Requests and Code Review

#309

Post by keybored »

IshKebab wrote: post #222
jonahx wrote: post #200

Earlier quoted context omitted.

> 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 s…

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…

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

Re: The Theatre of Pull Requests and Code Review

#310

Post by ViewTrick1002 »

singpolyma3 wrote: post #271
ownagefool wrote: post #263

Earlier quoted context omitted.

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.

If the commits don't matter why did you make them separate to begin with?

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

Post reply on HN