3 ms·
You bring up a good point about changing the code. I wouldn't want the reviewer changing the code himself and not getting reviewed again. But even if it was rev
by mnm1 8y ago
You bring up a good point about changing the code. I wouldn't want the reviewer changing the code himself and not getting reviewed again. But even if it was reviewed again, I would still find it problematic as the reviewer might make changes that are unnecessary and detrimental to the code base based solely on his opinions. For example, he prefers to use front end templates vs. backend or prefers to break up classes that should stay together, etc. Most code review issues that come up are a matter of opinion and it should be up to the author to change or not to change the code. If the reviewer feels so strongly about a change, he should submit a different PR in addition and it should be evaluated separately. I can't think of anything more enraging than coming in and seeing your reviewer rewriting your perfectly good PR simply because he wants to do it his way. At that point, I wonder why I even wrote a PR in the first place. Why not have this asshole write all the code then?
- skybrian 8y agoYeah, so in the end it's all about the relationship between the programmers, and as I've said, this can be tricky. If you neglect that it's not going to work. For example, if you're sending a patch to the owner of the code and they are known to be very picky, maybe it won't be considered an insult to rewrite it, since in the end it's their project and it saves a lot of back and forth? Or maybe it's a team where they have gotten used to it? Believe it or not, starting from a patch that actually works is actually pretty helpful and does save time compared to starting from scratch.
- mnm1 8y agoI'm OK with sending a patch or pull request back to the author in any circumstance and using the author's work as a base for said patch/PR makes complete sense to me. The problem I've seen is when the reviewer takes it upon himself to make these changes directly in the current PR without giving the original author a chance to change or defend his code. This is exacerbated when said changes take an entire day or two of the reviewer's time and are so extensive that the original work submitted becomes irrelevant. This wastes the reviewer's time which could be spent on other things instead of rewriting something that already works and has been done and it wastes the author's time because it essentially discards his work. You're right, it is definitely about the relationship between programmers, but I think some ground rules would help in cases where the relationship is unclear or the programmers work async remotely.