Page MenuHomePhabricator

All reviewers should be allowed to mark a comment as done
Closed, ResolvedPublic

Description

All reviewers should be allowed to mark a comment as done.

Event Timeline

angie raised the priority of this task from to Needs Triage.
angie updated the task description. (Show Details)
angie added a project: Restricted Project.
angie added subscribers: jhurwitz, angie, aj.

Can you describe the problem you're facing, not just your desired solution? See discussion here for why we consider this to be important:

https://secure.phabricator.com/book/phabcontrib/article/feature_requests/#describe-problems

First, thanks so much for adding 'done' comments, it's definitely helps. I can speak to why we asked for this a little bit.

  • Each comment thread is really a way to have a discussion on a change. Sometimes I might post a comment on a diff asking a question about a line of code. The author of the change might reply but not be sure if that is an acceptable answer or if it sufficiently answers my question, so they might not mark the comment as 'done'. In cases like these, I'm sorta responsible for that comment thread, so it's my responsibility to mark it as done.
  • Sometimes people just forget to mark comments as done.
  • Sometimes a comment gets out of date, possibly because of a rebase/merge or a change in another part of the diff.

Related is that it would be really awesome to have a view of just the non-done comment "threads" in a diff. It's surely not always sufficient, but I personally found a ton of value from it when using Google's critique.

@epriestley, what do you think of the problems/use cases @aj described?

+1 to this.

I'm not sure I see the use case for anyone to mark a comment done, but at least the author of the diff *and* the commentor. Though I could also imagine one reviewer taking over for another and needing the power to mark the comments as done.

Some discussion in T9367.

The current design of "Done" is trying to make it an optional feature authors can use to help manage large amounts of feedback, not a required feature. We don't teach users to check the box, we don't prompt them if they don't, we don't suggest they do, etc. So stuff like this:

Sometimes people just forget to mark comments as done.

...is, under the current design, expected and encouraged. For example, I very rarely use this checkbox myself.

Philosophically, I want to avoid building features which emphasize or teach granular, line-by-line code review, because I believe this review is of very little value compared to high-level, big-picture review. The lack of "check all the boxes" cues in the UI is an intentional decision to avoid emphasizing a mechanical "look at each line, check all the boxes, you're done" approach to review.

Each comment thread is really a way to have a discussion on a change. Sometimes I might post a comment on a diff asking a question about a line of code. The author of the change might reply but not be sure if that is an acceptable answer or if it sufficiently answers my question, so they might not mark the comment as 'done'. In cases like these, I'm sorta responsible for that comment thread, so it's my responsibility to mark it as done.

I'd expect the acceptability of the answer to be signaled by you accepting or rejecting the review, or responding to the question.

Sometimes people just forget to mark comments as done.

The current design of the feature intentionally makes "Done" an optional feature that the author uses at their discretion.

Sometimes a comment gets out of date, possibly because of a rebase/merge or a change in another part of the diff.

I'd expect this to usually be self-evident, but you could reply "Never mind, this is obsoleted by..." and that gives the author a chance to correct you if you made a mistake in your assessment.


Generally, the current design of this feature is very understated, and this is intentional. T1460 is not yet marked resolved because many behaviors here remain murky; we're likely to resolve that before pursuing other changes to inlines. The current direction for this feature isn't entirely clear, but I don't expect to push the workflow much in the direction of making "Done" a progress/completeness metric.

eadler added a project: Restricted Project.Jul 7 2016, 5:13 PM
eadler moved this task from Restricted Project Column to Restricted Project Column on the Restricted Project board.Jul 7 2016, 10:58 PM

Per T1460:

  • I plan to pursue T5654 and T8250 before revisiting this, since I think they'll impact whatever happens here.
  • I don't currently plan to let any reviewer mark an inline is done.

My current thinking is that I may let the user writing the inline mark it as done before they submit it (e.g., "just a note"). But I'd generally like to find solutions here which keep the focus away from mechanical item-by-item check-all-the-boxes review.

  • D19634 allows reviewers to mark their own inlines as "Done" before they submit them. This addresses the case where you want to leave a non-actionable inline (like "This is much-improved.") but feel like leaving an inline like this is imposing on the author because they "must" check it off.
  • D19635 marks the author's own inlines as "Done" by default. They may un-mark them if they prefer (e.g., actually want to leave themselves a TODO). This addresses the case where authors feel like they "must" check off every comment, so they either reply in two passes (one to submit; one to check off all their own replies) or avoid replying.

D19635 marks this task as complete; I don't currently plan to let other reviewers interact with this checkbox in the general case.