I said very rarely not never. I classify suggested changes in 2 flavors. The first is minor changes where someone suggests "hey this would be easier to read if you used syntax A vs syntax B here".

I get those frequently, and they are usually reasonable suggestions, and I usually graciously accept them. But I say it's not worth bringing up in a PR because 99% of the time it doesn't actually matter in the long run. Both forms will have been used in the code base previously, and which one you think is better really comes down to which one you use more. It's the kind of thing you could change with a sufficiently opinionated linter. And the kind of thing that isn't worth the cost of mandatory PR reviews.

The 2nd is where someone suggests changes to overall architecture or changes to the way the feature or code works large scale.

These are much rarer because of several reasons

1. PR reviews are almost always surface level and so are more likely to catch category 1 than category 2. The incentives at nearly every tech company push for this. 2. Very frequently one person isn't available to review all of the PRs that go into a feature, so the reviewers lack context. 3. It's very unlikely that even if someone wants to dig deeper, that they have the free time to spend even 1% of the time the person who wrote the PR spent on it.

But the biggest reason I personally don't get many of the 2nd category is because I talk through non-trivial problems with other experienced engineers before I get to the PR stage.

You're missing a third category which is "here's another way you could do this..." which isn't just about legibility but more tangible tradeoffs.

Certainly I agree architectural decisions shouldn't be made in every PR and ideally should be discussed before that point.

I'd consider that to fall under my category 2

"changes to overall architecture or changes to the way the feature or code works large scale"

I'm not saying there's never a reason to go back and redesign something after a PR review, but in my mind getting a design to that point and then actually needing to change it is a huge failure.

Far more common the case where someone wants a big design change with no tangible benefits just different tradeoffs.

I just don't think the ocassional benefit is worth the cost of the process.

You're lumping big changes with small changes. If you really won't go back and change one function or one class because someone shared a better idea at the PR stage that's unhealthy and you will improve by letting go of that.

I never said I won't go back and change a single function. And I never said I wouldn't change things larger than that if asked. I would lump small requests like that in with category 1 (of which a syntax change wasn't meant to an exhaustive example).

What I said was those kind of requests usually aren't meaningful or impactful long term. They very very rarely make or save anyone a single dollar. Let alone enough money to justify the time spent on the review process.

If you've already spent the time to suggest the change, if it's slightly better, sure I'll make it. Even if it's not any better, as long as it's not worse, if you feel strongly about it, there's a good chance I'll go along. I just don't think the process has a positive ROI, and I've yet to see any data to convince me otherwise.

The problem is the good ideas should come before or while you start coding. By the time the PR is done isn't the right time to rewrite it.

What? This is absolutely incorrect. There's be no point in PRs if it's too late to change anything. Part of the point is to reduce the number of synchronous discussions your team has to have about code before writing it. PRs let you iterate on actual code instead of endlessly discussing hypothetical implementations.

This is a huge pet peeve for me.

You’d kill your teams velocity if you did this for every PR.

I worked on a team once with… Perfectionist Petra. Petra would jump on an 1200 line refactor that was blocking the team and demand changes that would require it to be rewritten. Not changes that would save the code base from grievous error: Petra didn’t like the pattern used or didn’t approve of the design.

Sufficient tests? Check. Linted and formatted? Check. Does Petra approve? Big variable. I often wanted to tell Petra if they could just write the code out for me in a ticket so I could copy it for them. Instead I had to try and read Petra’s mind or hope they wouldn’t jump on my PR.

Sometimes you have to trust your teammate and not let the perfect plan interfere with a good enough one.

I'm not advocating for constant nitpicking or demanding perfection. But somewhere between that and "PRs are too late for changes" is where most good teams operate.

I realize my comment was emotionally charged.

The point was that I'm a planner. I tend to discuss designs long before I begin writing code. By the time I get to a PR I don't expect to get feedback about the design. I'm looking for someone to check that the code is acceptable to merge. If you have problems with the design, it is too late!

However I work with folks who are not planners. They will write a PR as a proposal. Their expectation is that they may need to iterate on the code a bit based on that feedback; even the design and approach itself is up for review. If you have problems with the design, it's fine!

What has worked well for me in those situations in the past is to be up-front about what kind of feedback you're seeking. Mention in the PR description that you're looking for design crit, code review, etc. Whatever lingo works for your team.

Update: Realizing at the last minute that the design itself is wrong, and saying so, is a good thing -- and I do appreciate that. However it should be rare. My pet peeve is more with the folks who nitpick... whose design contributions are superficial and concerned with style more than substance.

I don't disagree some people have bad, overly nitpicky PR habits and they should be trained out of that.

That said, sitting down before hand and planning is useful for the big picture but one of the big benefits of PRs, as I explained earlier, is to eliminate lots of wasted time going back and forth in meetings about how to implement something and instead empowering engineers to go write code and then to iterate on actual implementations instead of endlessly discussing ahead of time. So there is a balance there where you make sure everyone is aligned on what you're building but also being open to suggestions on your PRs. Sometimes that may mean rewriting large chunks in exchange for drastically reducing pre-coding discussions.

I agree that balance is necessary and nitpickers should be steered away from that behaviour.

I think I'm more concerned with nitpickers which show up in different flavours.

I understand a functional requirement to rewrite a piece of code in a PR. Perhaps you spent the last 8 hours coding up a solution that passes all of the tests but if you use that data structure at scale it wouldn't perform acceptably. That's a rewrite and that sucks but it will be for the better.

OPP mentioned that this is happening but in discussion prior to PR. Makes sense to me.

I addressed that in the comment you are responding to...