Inferring the disposition of "BTW ..." comments

[jwnimmer-tri]

(See https://docs.reviewable.io/discussions.html#checking-and-changing-dispositions for background.)

In my experience with both my core review team and external first-time review collaborators, when people open a comment with "BTW" the discussion semantics that are most appropriate for the point they are making would be "Discussing", not "Informing".

Generally the topic is either an almost-trivial problem (where "minor ..." could have been used instead), or else a suggestion for a different way to write the code, which the author might find to be a better alternative. In either case, the action required should be on the author -- to either make a change, or ask a follow-up question, or dismiss.

The problems with using "Informing" for this case are (1) every other reviewer receives a red bubble to respond to, and (2) the discussion is not blocking, so the PR might signal that review is complete even though there is an unfinished discussion topic.

The red-bubble of (1) is especially odd. When two people are reviewing a PR, discussions that are "Blocking" or "Discussing" only red-bubble the author-reviewer pair who are involved in the back-and-forth. But when someone posts a BTW it red-bubbles everyone, even though it's usually one of the most trivial discussions.

Some of my more expert users have learned to manually toggle the disposition back to "Discussing..." when they post BTWs, but it's an uphill battle to teach everyone.

My suggestion is to swap the meaning to BTW over to "Discussing". (I suspect that making it per-org configurable would be even more confusing.) To me, that best matches what users expect in terms of a suggestion-response workflow.

I suppose maybe I could work-around this on my own? Possibly the review completion condition has enough information for me to solve (2) but I think (1) would still be a problem.

I'm happy to leave FYI as informational only. I don't think most people expect FYIs to block the review.

[pkaminski]

Hey Jeremy, thanks for bringing this up. I agree that BTW is a bit ambiguous: it could either mean "BTW, I just spotted this minor issue" or "BTW, your code made me think of this unrelated thing". I also agree that making the semantics configurable is problematic (and working around them in the completion condition is pretty much equivalent) but I'm not convinced that changing them at this late date is much better. I'm sure there are lots of people used to its current behavior that would be very confused (or worse, not notice) that it now does something else. Perhaps we could simply remove the magic word altogether, though that runs a very similar risk with dropping backwards compatibility.

Brainstorming other options:

• We could attract more attention to the disposition change when the magic word is detected, perhaps by flashing some color on the new disposition (in the usual spot) or popping up a short-lived tooltip over the word.
• We could give self-hosted Enterprise installs the ability to redefine magic words and mappings. While I don't think it's a good idea at the repo or organization level, it makes some sense as a customization at the installation level. This would not be entirely trivial, though, as it would need to feed into both client and server, as well as update the UI tips...
• ...?

The problems with using "Informing" for this case are (1) every other reviewer receives a red bubble to respond to

Could you expand on this? While there is some special-casing for the Informing disposition, it's not clear to me what you mean by "red bubble". The (legacy, no longer used) red disc on an avatar that indicates they're blocking the review? The red unreplied counter? Something else? Instructions for a repro would be most useful here.

(2) the discussion is not blocking, so the PR might signal that review is complete even though there is an unfinished discussion topic.

By default, Reviewable will automatically select Approve or Block when publishing based on the user's dispositions and review marks. This shows up on the publish button and should (might?) give a user pause if the default value is unexpected. Do you have things configured differently, or is the signal too subtle perhaps?

[pkaminski]

Ping -- thoughts @jwnimmer-tri?

[jwnimmer-tri]

I'm not convinced that changing them at this late date is much better.

Understood.

We could give self-hosted Enterprise installs the ability to redefine magic words and mappings.

Possibly useful for others, but not for us. Our reviewers split their time between our open-source code (using reviewable.io) and corporate code (using enterprise), and having different semantics in each would probably be frustrating / confusing.

The problems with using "Informing" for this case are (1) every other reviewer receives a red bubble to respond to

Could you expand on this?

I meant the red unreplied counter, i.e., this glyph on the dashboard page (or similarly in the review itself):

I will see what I can do about a repro, but from memory it's pretty easy to see:

• Author creates a PR, and assigns two people for review.
• One reviewer posts their review, e.g., with 3 nit comments (=> "discussing"), 2 comments with no magic keyword (=> "blocking"), and 1 BTW comment (=> "informing").
• The second reviewer will see an "unreplied" indicator for only the BTW comment (i.e., set to "1"). The 5 other discussions show up as grey; the second reviewer can browse through them but no action is required.

Do you have things configured differently ...

Yes, we have GitHub reviews (and review approvals) completely disabled in our completion condition, because their workflow semantics (on the GitHub side) are nonsense. We still use the original "all discussions resolved + lgtm emoji" as our approval workflow.

[pkaminski]

As noted in #953, we'll likely go ahead with rescinding the BTW keyword. I'm just waiting for a bit more feedback.

The problems with using "Informing" for this case are (1) every other reviewer receives a red bubble to respond to

So I'd completely forgotten, but this was an intentional feature! See the discussion starting here. I guess it might be useful to have a resolved discussion that still only pings the author but I don't think we want to add any more disposition options. Hmm...

[schreter]

Just my 2c: Wouldn't it make more sense to introduce a config file (e.g., in JSON format), which would reside either in the repo (taking precedence) or in the Enterprise case on the self-hosted Revieable instance? Then, it would be trivial to tweak it to the liking of the repo owner, also adding the possibility to match based on any regex to any default disposition.

Such a config file can be in the future used for many other purposes for various other configurations, when they become necessary.

Aside from that, what I'd also like to see is to be able to set the default dispositions for self-comments and comments for occasional reviewers. Especially the latter is problematic, since the default disposition is set to "blocking", thus blocking the review even when all comments are resolved, resulting in the need to hunt down the unexperienced occasional reviewer to finally turn his blocking comments to something else (yes, I know one can dismiss the dissenter from a discussion, but that's not the proper way to go about things).

[mkow]

Just my 2c: Wouldn't it make more sense to introduce a config file (e.g., in JSON format), which would reside either in the repo (taking precedence) or in the Enterprise case on the self-hosted Revieable instance?

I don't like this idea, such settings are usually user personal preferences, not project-specific style.

since the default disposition is set to "blocking", thus blocking the review even when all comments are resolved

I think the default disposition for project members should be "blocking", although maybe for people without some special rights in the repo it shouldn't be? Anyways, @schreter, I think you can quite easily customize this in the status check script when configuring the repo on Reviewable (to ignore blocking dispositions from non-whitelisted members, or something similar).

[schreter]

I don't like this idea, such settings are usually user personal preferences, not project-specific style.

True, but only after it's initialized with project defaults. The default being "blocking" without the possibility to configure it on the project level is IMHO wrong.

I think the default disposition for project members should be "blocking"

Not really. Since default completion criterium is to have comments resolved, it's fully sufficient to have it in "discussing" mode. Blocking is blocking, period - I don't discuss, I'm against submitting the code as-is. And the problem is, we have several people who are on the team, but very seldom do code reviews (e.g., junior members). Yes, sometimes we also have external reviewers coming in via temp licenses (one-time users), but that's an exception - mostly we are talking about people in the team. And even though I keep reminding them not to set everything to blocking, they ignore good advice, causing unnecessary stalls especially on behalf of the most experienced team members. So I'd like to set a different default to get rid of the problem.

customize this in the status check script

On the GitHub side, it's already configured that way (in fact, to ignore blocking comments from all members - see above). But on Reviewable side, it still shows the review as incomplete, even though the author resolved all issues. The occasional reviewer clicks on "Approve", no issue there, but doesn't close his comments (which he has to do manually). Thus, the Reviewable still says the condition is not met. And, tools working on top of GitHub (such as spr) verify all the checks and don't ignore Reviewable, so it's impossible to merge a stack of changes because of such incomplete review.

Therefore, I'd like to configure these defaults (not personal settings) on the project level. Also other stuff, like association of keywords with comment disposition can be defaulted in project settings, but who says the user can't upload his own JSON config file with different settings?

I just would like to have it as flexible as possible with as little config as possible and reserve the "blocking" disposition for problematic code only, not for "regular" review comments (and then also block submitting to GH when something is set to blocking).

[mkow]

True, but only after it's initialized with project defaults. The default being "blocking" without the possibility to configure it on the project level is IMHO wrong.

Oh, sorry, I thought this part was about the disposition of BTW and similar comments (see the original discussion in this issue).

Blocking is blocking, period - I don't discuss, I'm against submitting the code as-is.

I don't agree here :) Half of my blocking comments are "please explain why you did that thing this way". It's blocking because I want to get the answer and understand the PR before merging it, and often the code will be submitted as-is. I really rarely open discussions which I don't want to get an answer on before merging.

And the problem is, we have several people who are on the team, but very seldom do code reviews (e.g., junior members).

So, you could probably just add some role to the project which default to blocking? (for proper reviewers) Of course I can't talk for your project, but for me it seems pretty bad and risky to change the default disposition for everyone in the project :)

[pkaminski]

This issue has diverged a bit from the original request. :) That's fine, but let me close that out first: we decided to go ahead and remove "BTW" and "OK" as disposition keywords. This should ship in the next few days.

Configuring custom per-user disposition keywords would be cool, but as explained in #953 it's a lot of work and I don't think the ROI is there. Never say never, but we have no plans to implement it at this point.

@jwnimmer-tri, if you think the notify-all behavior of Informing discussions is a problem worth raising then let's open a new issue for that so it doesn't get lost here.

Now, the issue of default dispositions is a timely and interesting one. There are actually four such defaults, set up as a 2x2 matrix: reviewer / PR author on one axis, and new discussion / reply on the other. Each user can set a per-cell default though I suspect this functionality is not often used and certainly not by novice users.

If no per-user default is set, the fallback is Blocking for new discussions and Discussing for replies, for both reviewers and PR authors. The reasoning behind these defaults was that Blocking is the safest choice for reviewers as it ensures that discussions won't get resolved by authors and inexplicably disappear; for authors, it's best to be consistent and have the same default for new discussions, as it's more predictable. For replies, Discussing is uncontroversial AFAIK.

The choice of Blocking for new author discussions was hotly debated at the time and, based on experience since, I believe I made the wrong call: authors keep blocking their own review by accident, and nobody notices or cares about consistency. I'm inclined to change this default to Discussing soon unless somebody has a strong counter-argument to consider. (As before, I'll also explicitly ask some of our larger customers for feedback.)

The question of whether these fallback defaults should be customizable is an interesting one. I think there are two scenarios to consider:

1. Repositories may have different preferred workflows or "house rules" regarding use of the various dispositions, and would like to, e.g., default new reviewer discussions to Discussing to better fit those. (Also see Let admins set a default review overlap strategy at the repo level #955 for a related repo-level customization request.) On the surface this is a reasonable request though we'd want to wait for Load review completion condition script from repository #722 (repository settings files) to be available first to avoid bloating the repo settings UI. I have two concerns, though: (i) for users working in multiple repos, it may be confusing to have the default change somewhat arbitrarily; and (ii) this doesn't help in monorepos, where the customization would make more sense per-path, which would lead to even greater confusion as the default would change even within a single review!
2. Different "kinds" of reviewers should have different defaults, e.g., "core" reviewers should be Blocking, but "casual" reviewers should be Discussing. This is about articulating and refining the "role" that somebody plays in a review, and it's something we're moving towards as part of Review participants panel #929. I think it would be interesting to discuss in more depth what kind of roles would be useful and how they should be assigned -- perhaps only assigned or requested reviewers default to Blocking, and everybody else to Discussing? Or maybe it should be driven by repo permissions, or membership in a team returned from the completion condition? We're not ready to implement any of this yet but I'd like to start getting a feel for the design space and what would be useful here. Of course, there are similar concerns about inconsistency and confusion as above.

Finally, a few responses to specific points:

yes, I know one can dismiss the dissenter from a discussion, but that's not the proper way to go about things

This is exactly the kind of situation that dismissal is meant for, though I agree that it would be better to solve the root problem.

But on Reviewable side, it still shows the review as incomplete, even though the author resolved all issues.

As @mkow mentioned, this is something you have control over with the custom completion condition. Yes, the Reviewable UI will still show the discussions as unresolved, but the review completion (and corresponding code-review/reviewable status check) can be set independently. You could even rewrite the discussion resolution logic in your condition to treat Blocking as Discussing, and determine whether the review is complete based on these alternative semantics.

[mkow]

what kind of roles would be useful and how they should be assigned -- perhaps only assigned or requested reviewers default to Blocking, and everybody else to Discussing? Or maybe it should be driven by repo permissions, or membership in a team returned from the completion condition?

At least for us, the best fitting version would be (I think) that the default-blocking people are team members plus people explicitly assigned to the review.
And there's also one more flavor to this: Should other people be forbidden from using blocking disposition completely? This is mostly relevant to public projects hosted on github.com where anyone can come and place a review. From my side I haven't made my mind yet what I'd prefer here...

[pkaminski]

After digging further into the disposition keyword code I found the situation to be even messier than I thought. FYI and BTW can lead to either Informing or Discussing dispositions, depending on whether they're applied to a new discussion or in a reply! That's exceedingly confusing and I have no idea why I settled on this design. This is a good opportunity to straighten out the mess, and given that BTW was in fact usable to set Discussing before I think we'll go with FYI being dedicated to Informing and BTW being dedicated to Discussing. Basically you were completely right @jwnimmer-tri. 😅

Incidentally, I got nearly universal enthusiastic endorsement for making the default for author-created discussions Discussing, so we'll go ahead with that change as well.

[jwnimmer-tri]

@jwnimmer-tri, if you think the notify-all behavior of Informing discussions is a problem worth raising then let's open a new issue for that so it doesn't get lost here.

I think that the FYI workflow is fine as-is. For FYI comments, in my experience in most cases they should be seen by everyone. We use them for things like "FYI This code ended up being moved into a different file over there" or "FYI This goes against the style guide but the reasons here are well-justified like so...", which should notify the dashboard for all participants but need not involve any action required.

I think there's probably a small subtlety in multi-party reviews in case you want to target just one person you need to know to say @bob FYI this was what I came up with (spams bob only instead of FYI @bob this is what I came up with (spams everyone) but I don't think that's very common.

Incidentally, I got nearly universal enthusiastic endorsement for making the default for author-created discussions Discussing, so we'll go ahead with that change as well.

Late to the party, but that's fine by me too. (As an expert user I'd actually prefer ??? or maybe Working for my author-created discussions, but I'm probably a weirdo. Usually mine are questions for myself rather than questions for my reviewers, but that's perhaps atypical.)

[pkaminski]

As an expert user I'd actually prefer ??? or maybe Working for my author-created discussions, but I'm probably a weirdo. Usually mine are questions for myself rather than questions for my reviewers, but that's perhaps atypical.

You can do that! Next time you create a discussion as an author, click the "Change default disposition" link at the bottom of the disposition dropdown then pick your favorite new-discussion-as-author disposition, and presto!

[pkaminski]

The keyword and default disposition changes are now live, and will be in the next Enterprise release.