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

391–400 of 431 posts

Re: The Theatre of Pull Requests and Code Review

#391

Post by Spivak »

singpolyma3 wrote: post #272
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.

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

Have we stoped celebrating laziness being a virtue in software development? Discipline doesn't and will never scale and the pressures of business mean that processes that processes that put up walls to shipping will always crumble.

Real example, we do PR reviews because they're required for our audit and I'm of the opinion that they're mostly theater. It's vanishingly rare that someone actually wants a review rather than hitting the approve button and will call it out specifically if they do. Cool. This means you can't count on code review to catch problems, discipline doesn't scale after all. So instead we rely on a suite of end to end tests that are developed independently by our QA team.

Re: The Theatre of Pull Requests and Code Review

#392

Post by sigseg1v »

illuminator83 wrote: post #321
William_BB wrote: post #256

Earlier quoted context omitted.

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.

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

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

Could you explain this a bit more? I'm having trouble visualizing the end to end process.

1. Someone has what they feel is a complete change and submits a PR for review.

2. The reviewers read part of it, first half looks good, and halfway through they have concerns and request changes.

3. The submitter now has to fix those concerns. If they are not allowed to push an additional commit to do this, how do you propose they accomplish this? Certainly they should not force push a public branch, right? That would cause pain to any reviewer who fetched their changes, and also break the features on GitHub such as them marking which files they have already read in the UI. But if we cannot push new commits and we cannot edit the existing commits, what is the method you are suggesting?

Re: The Theatre of Pull Requests and Code Review

#393

Post by StilesCrisis »

skirmish wrote: post #238
StilesCrisis wrote: post #35

Earlier quoted context omitted.

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

The adapter breaks down on sufficiently massive repos. Unfortunately, there's a reason Perforce is still in use despite no one liking it. Git scales via sub-repos, but those have their own drawbacks, and no one is enthusiastic enough about this to champion it.

Re: The Theatre of Pull Requests and Code Review

#394

Post by sigseg1v »

siva7 wrote: post #299
agentultra wrote: post #281

Earlier quoted context omitted.

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.

If your PR is + 1000 code lines long, you already made a mistake at the requirements and planning stage (like many teams do).

This sounds unattainable to me. For code bases in the 2 million or more lines range, something as simple as refactoring the name of a poorly named base class can hit 5000 lines. It also was not a mistake with the original name it had, but you'd still like to change it to make it more readable given the evolution of the codebase. You would not split that up into multiple commits because that would be a mess and it would not compile unless done in one commit.

How is this a mistake?

Re: The Theatre of Pull Requests and Code Review

#395

Post by jiveturkey »

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 write intermediate commit messages as notes to self. You don't always work continuously on the same PR. The commit messages are a useful context refresher.

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

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

Re: The Theatre of Pull Requests and Code Review

#396

Post by michaelcampbell »

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…

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

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

Re: The Theatre of Pull Requests and Code Review

#397

Post by jiveturkey »

tcoff91 wrote: post #92
vinnymac wrote: post #70

Earlier quoted context omitted.

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…

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.

I often do this. It's a great way to highlight the areas you want a reviewer to focus on, the areas you are least happy about and want some collab. You can't always get collab pre-review, as you're writing. Plus you want to write it down and move on. Then you can async edit when the feedback comes. Not unlike a prose writing process.

Re: The Theatre of Pull Requests and Code Review

#398

Post by michaelcampbell »

tetha wrote: post #376
BobbyJo wrote: post #175

Earlier quoted context omitted.

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"

I enjoy Sandi Metz' point there as well: Code just looking the same is not enough to call it duplication. Once you have to change two places looking the same to add a new feature or to fix a bug, then you have duplication and should centralize it.

I usually wait till 3; 2 is about the _very general_ point at which changing multiple places is about the same work as changing it to be centralized. 3 is almost always a better place to make that leap.

Re: The Theatre of Pull Requests and Code Review

#399

Post by haradion »

StilesCrisis wrote: post #393
skirmish wrote: post #238

Earlier quoted context omitted.

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

The adapter breaks down on sufficiently massive repos. Unfortunately, there's a reason Perforce is still in use despite no one liking it. Git scales via sub-repos, but those have their own drawbacks, and no one is enthusiastic enough about this to champion it.

With the right workspace mapping, you can pull in just part of the depot, which makes the repo size a lot more manageable.

Re: The Theatre of Pull Requests and Code Review

#400

Post by siva7 »

sigseg1v wrote: post #394
siva7 wrote: post #299

Earlier quoted context omitted.

If your PR is + 1000 code lines long, you already made a mistake at the requirements and planning stage (like many teams do).

This sounds unattainable to me. For code bases in the 2 million or more lines range, something as simple as refactoring the name of a poorly named base class can hit 5000 lines. It also was not a mistake with the original name it had, but you'd still like to change it to make it more readable given the evolution of the codebase. You would not split that up into multiple commits because that would be a mess and it wou…

Such PR's shouldn't be the norm but the exception. What happens way more often is that such refactoring happen in addition to other criteria on the same PR. In high-functioning teams i've worked this is usually done as a separate PR / change, as they are aware of the complexity this operation adds by mixing it with scope-related changes and that refactoring shouldn't be in the scope of the original change.
Post reply on HN