Any top-level comment on pull request ought be tagged with one of four emojis:
-
❓ for a non-blocking comment that asks for clarification. The pull request author must answer the question before the pull request is merged, but does not have to wait for the comment author to re-review before merging.
-
🎨 for a non-blocking comment that proposes a refactor or cleanup. The pull request author does not have to address the comment for the pull request to merge.
-
⚠️ for a blocking comment that must be addressed before the pull request can merge. The comment's author should leave aRequest Changesreview, and is responsible for re-reviewing once the pull request author has addressed the issue. -
😻 for a comment that compliments the author for their work.
Note, this policy does not apply to follow-up comments in threads.
Your comment likely falls into one of these categories. If you feel that it does not, default to :art:, as such a comment requires the least amount of work from the pull request author.
Your comment would probably be better served with a single focus. For instance:
-
❓ + 🎨 -- you likely cannot confidently propose a refactor when you have a related, unanswered question. You should likely just ask a question, and propose a refactor later.
-
❓ +
⚠️ -- if you are not confident that the pull request can merge, you should always mark your comment as:warning:. -
🎨 +
⚠️ -- this is impossible, as:art:comments are definitionally non-blocking, and:warning:comments are definitionally blocking. It is your responsibility to decide which it is. -
😻 + something else --
:heart_eyes_cat:is reserved for exclusively positive comments, that do not propose a change, ask a question, nor identify a problem. If you do not leave any:heart_eyes_cat:comments, you are likely not accurately representing your opinions of the pull request.
Someone left a :warning: comment on my pull request that I do not believe is blocking. What do I do?
Your first instinct should be to trust your teammates -- if they believe an issue is blocking, they are likely making that determination intentionally, based on their unique experience. You should respect their feelings.
If you feel like they have misunderstood the issue, ask for clarification, and answer any questions they may have.
It is the pull request author's prerogative to ignore :art: comments. If you feel like your comments require attention before merging, mark them as :warning:.
If you feel like your comments are non-blocking but should still be addressed, you should introspect about why you feel like that is the case. Perhaps the comments are indeed blocking, or your sense of what is blocking has drifted from the team's.
I want to mark my comment as :warning:, but I do not want to be responsible re-reviewing the PR. What do I do?
If you believe that code is dangerous to merge into master, preventing it from doing so is one of the most important things you can do as an engineer. You should try to make time in your schedule to be a part of that process.
In rare cases, you may need to prevent a pull request from merging, but do not have time to re-review the pull request. (i.e. reviewing a pull request the day before you go on vacation). In such cases, you should explicitly delegate that responsibility to someone else. i.e.
⚠️ I'm pretty sure this line of code introduces an n plus one! we need to fix that before merging. @raorao can re-review the PR once you've pushed your change.
I want the pull request author to take my feedback seriously, but I do not want to mark it as :warning:. What do I do?
One of the great advantages of this approach is that pull request authors do not have to guess as to what you think is important or not. You are not insulting their work by leaving a :warning: comment, you are simply communicating how important you feel the issue is.
If you feel like your feedback is not being taken seriously, this may be a symptom or larger issues on the team, and should be addressed outside of the pull request process.