Page MenuHomePhabricator

Mark Owners package reviewers which own nothing in the current diff
ClosedPublic

Authored by epriestley on Sep 29 2017, 8:38 PM.

Details

Summary

Ref PHI91. When Owners (or Herald, or manual user action) adds package reviewers to a revision, later updates to the revision make some of them less relevant or irrelevant.

Provide a hint when a package reviewer doesn't own any of the paths that a diff changes. Humans can then decide if the reviewer is obsolete/irrelevant or not.

This is a rough cut to get the feature working, design could probably use some tweaking if it sticks.

Test Plan

Diff Detail

Repository
rP Phabricator
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

epriestley created this revision.Sep 29 2017, 8:38 PM

I wonder if an "Owns $n Changed Paths" note on currently-relevant packages would be useful, or just distracting.

I want to do another UI pass on this, but I was thinking maybe that the "No Paths" warning is red and has a more visible icon or something, and perhaps we put a little "3 " next to the packages that do own stuff. But I'm not sure if that's actually too useful, and we don't have a great target for clicking it (i.e., there's currently no "filter changesets to just these paths" action), and if we put a tooltip on it it'll be a ridiculous monster for "112 ".

I could imagine perhaps making it so that when you click it all other files are hidden, but that's probably not great if you own multiple packages. I also sort of generally want to gently dissuade "look at tiny pieces of a change through a microscope" sort of review, and this would encourage it, although I don't think this is a particularly dangerous change.

A better interaction in this vein might just be a keystroke like "collapse all files I don't have package authority over", maybe b for Equip Blinders. Not sure if that'd actually be useful, but maybe it would if there are a lot of very cursory review rules like "just make sure none of the Go code uses the old API" or whatever.

amckinley accepted this revision.Sep 29 2017, 9:56 PM

Looks good for a rough cut. I like the idea of a "collapse all files I don't have package authority over" button.

This revision is now accepted and ready to land.Sep 29 2017, 9:56 PM
This revision was automatically updated to reflect the committed changes.