3 ms·
I think it depends on your teams' expectations for code review. It seems like perhaps your team doesn't define what is acceptable/constructive/valued, etc from
by agentultra 4y ago
I think it depends on your teams' expectations for code review. It seems like perhaps your team doesn't define what is acceptable/constructive/valued, etc from reviewers.
Personally I prefer the first one on this list: it gives the author the opportunity to refuse the suggestion. Perhaps its a frivolous request and the PR is already been open for far too long. Maybe the author would prefer to leave the duplication there until there's more justification for extracting it to a function. Who can say?
Not all feedback is necessarily good or useful.
Personally I don't bother with suggestions like this where it's more of a style preference unless there is a style guide to adhere to where "extracting this to a function" would clearly connect to a guideline.
I tend to focus on verifying the that the author did their homework: what evidence/proof did they submit that ensures me the change is correct wrt. specs/requirements/tasks? If that evidence/proof is sufficient that's all I care about: it makes it easier to manage a higher volume of review requests.
I have a pet peeve for suggestions that are "trivial" and based on, "well I would have written it this way," as I don't find them terribly useful most of the time. They rarely affect the specification of the program and their claims to readability/maintainability are often dubious and superficial. Bike shedding is a huge waste of time in the review process. At least when worded with, "Should we..." there's a chance for the author to explain whether they had already considered that or accept the suggestion as helpful and make the change.