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