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

231–240 of 431 posts

Re: The Theatre of Pull Requests and Code Review

#231

Post by BobbyJo »

MoreQARespect wrote: post #180
BobbyJo wrote: post #171

Earlier quoted context omitted.

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

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

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.

Re: The Theatre of Pull Requests and Code Review

#232

Post by scottgg »

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 ?

Re: The Theatre of Pull Requests and Code Review

#233

Post by criemen »

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

Re: The Theatre of Pull Requests and Code Review

#234

Post by fizzynut »

MoreQARespect wrote: post #180
BobbyJo wrote: post #171

Earlier quoted context omitted.

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

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

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.

Re: The Theatre of Pull Requests and Code Review

#235

Post by mathstuf »

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…

This is truer 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.

Re: The Theatre of Pull Requests and Code Review

#236

Post by degamad »

mgkimsal wrote: post #227
thayne wrote: post #225

Earlier quoted context omitted.

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…

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.

The context here was a class API, not a REST API, so versioning is not relevant.

Re: The Theatre of Pull Requests and Code Review

#237

Post by osigurdson »

g-b-r wrote: post #108
osigurdson wrote: post #45

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…

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

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

Re: The Theatre of Pull Requests and Code Review

#238

Post by skirmish »

StilesCrisis wrote: post #35
SOLAR_FIELDS wrote: post #25

Earlier quoted context omitted.

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

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

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

Re: The Theatre of Pull Requests and Code Review

#239

Post by markus_zhang »

roguecoder wrote: post #205
markus_zhang wrote: post #71

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

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

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

Re: The Theatre of Pull Requests and Code Review

#240

Post by lijok »

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.

Post reply on HN