Comment by sarchertech
Comment by sarchertech 5 days ago
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.