Page MenuHomePhabricator

Ability to differentiate differential comments
Closed, WontfixPublic

Description

In some cases a reviewer requests changes on a revision and in addition to the comments on the code where changes are desired, the reviewer also leaves general comments such as questions or suggestions.
It would be useful to be able to differentiate between comments that require a change and those that don't. That way the author won't confuse suggestions or questions with change request comments.

Event Timeline

starkej2 raised the priority of this task from to Normal.
starkej2 updated the task description. (Show Details)
starkej2 added a project: Differential.
starkej2 added a subscriber: starkej2.

Can you provide some more context, or examples of the kinds of comments that are ambiguous?

My experience as an author is that I distinguish between suggestions/questions vs change requests by reading the comments. I haven't personally experienced much difficulty with this.

As a reviewer, I try to phrase comments so they're clear. For example, I use language like "Consider..." or "Another approach..." to present soft suggestions, "Prefer ..." or "By convention..." to present firmer suggestions, and "This is a severe security concern." or "As written, this won't work." to raise more serious issues. I haven't experienced much in the way of miscommunication by doing this, and my feedback doesn't necessarily break down into 2-3 severity buckets.

I'd worry that trying to formalize this would solve a non-problem while making the workflow heavier for reviewers. If this is a problem specific to your environment (for example, maybe you employ engineers from diverse backgrounds and there's sometimes a language barrier), could you resolve in your environment by conventionally prefacing comments with text like "Suggestion:" or "Change:"?

If you want something somewhat-formal, you could potentially use the "Important", "Warning" and "Note" styles in Remarkup to do this clearly:

This is a security vulnerability.
Consider documenting this in greater detail.

Beyond being more flexible, this allows reviewers to leave feedback with a mixture of suggestions and requests in the same block of text.

I have adopted the following general practice, which works well (for me at least) for both GitHub and Phabricator. This text is taken from the CONTRIBUTING.md file for Code Contracts.

GitHub provides several emoji which can be included in many areas of the site, especially including comments on issues
and pull requests. To reduce confusion during evaluation of a pull request, the following emoji convey special intent.

  • ❓ (:question:) Indicates a question. This emoji comes with no strings attached, which means it's fine to simply answer the question in another comment without making any changes to code.

    In general, pull requests with an unanswered question will not be merged until *someone* addresses it. This could be the original creator of the issue or pull request, another contributor to the project, or even the person who asked the question.
  • ❗ (:exclamation:) Indicates a problem with the code which will need to be addressed before changes can be merged into master. This *may* be accompanied by a specific suggestion for how to change the code. Any contributor is completely welcome to open a discussion regarding alternative solutions, even if the originally proposed change has already been made.

    Every active contributor to a project is likely to use the ❗ emoji (or similar) due to a simple misunderstanding or oversight in their evaluation. This is absolutely fine. If you notice a case like this, simply add a comment to clarify the intent and the exclamation can be considered resolved.

    Pull requests with an unresolved exclamation will not be merged until they are addressed by either a change in the code or a detailed explanation. While it is possible for code with an exclamation to be merged, this will usually only occur if *all* of the following conditions hold:
    • The rationale is fully and clearly explained
    • The code corrects an issue that is likely to affect users
    • All other proposed solutions are either too time-consuming to implement or are equally (or more) problematic
  • 💡 (:bulb:) Indicates an idea or suggestion regarding a potential change in the code. These items will not necessarily block the inclusion of code into the master branch, especially in cases where suggestions are trivial and no other issues were found during code review.

    In certain trivial cases (e.g. a simple formatting change), a core contributor on the project may make the suggested change themselves in the process of merging a pull request. Like the ❗ emoji, 💡 suggestions invite open discussion about alternative solutions from any contributor.

In addition to the above, I have started using the following symbols:

  • 💭 (:thought_balloon:) Indicates I'm just "thinking out loud".
  • 📝 (:memo:) Indicates a note. I typically use this to add context to a review request which may otherwise have been overlooked.
  • ➡️ (:arrow_right:) Indicates the outcome of a discussion in a review. Used to mark my rationale for decisions to not change the code. Also used for "definitive" answers to questions which are asked in review.

In Phabricator, I also mark comments done based on the following criteria: A comment is marked done when the author does not intend to make any more changed as a result of the comment. A "change" can be as simple as adding a comment answering a question, creating a new bug/task/issue for addressing later, or as complex as uploading a new diff. If a comment is marked done and you still want to see something that you don't currently see, it means it's time to add a new comment.

Yeah, something generally in that vein seems like a good approach to me. It's much more flexible in terms of conveying intent (you can pick as many different emoji as your team needs/wants) and in terms of process (you don't need to explicitly fiddle with anything if your comment is unambiguous, like maybe pointing out a typo on a documentation change submitted by someone you know well and regularly work closely with).

You could also use {icon ...} or {icon ..., color=...} syntax if you want even more options (, , ) although these won't work as well in email right now

(And using only color to convey intent may create difficulty for users with low vision or colorblindness.)

starkej2 claimed this task.

Good points...never thought to use emoji in that manner. Thanks for the suggestions!