Hacker Newsnew | past | comments | ask | show | jobs | submitlogin

It's not exactly the case you might be pointing at, but there will definitely be times where I don't accept but would want someone else to do so. IMHO it should happen explicitely.

For instance sometimes the translation isn't consistent with other screens, but that's not an issue I'm willing to follow to the bitter end. If that's the only thing I have concerns about, leaving a comment to check with the copy writing team and let that team approve or not is a decent course of action.

Same with security issues, queries that will cost decent money at each execution, design inconsistencies, etc.

In these cases, not approving is also less ambiguous than approving but requesting extra action that don't require additional review from us (assuming we're very explicit in the comment about being ok with the rest of the code and not needing a re-review)



Approving with comments like "please fix X before you merge" is a footgun I've decided to avoid.

I totally agree with you on being explicit about why approval isn't given.

I'll say that there are lots of things that make any/some of us suck at PR reviews that I don't think are made worse or better by this "always approve or request changes" vs "comment without approval or requesting changes is okay" difference.




Guidelines | FAQ | Lists | API | Security | Legal | Apply to YC | Contact

Search: