Comment by rr808
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.
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.
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.
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.