Page MenuHomePhabricator

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

Authored by epriestley on Sep 29 2017, 8:38 PM.
Tags
None
Referenced Files
F19183446: D18663.id44809.diff
Tue, Dec 16, 4:15 AM
F18876457: D18663.diff
Nov 6 2025, 1:00 AM
F18866612: D18663.id.diff
Nov 3 2025, 6:13 PM
F18782953: D18663.id44812.diff
Oct 13 2025, 3:06 AM
F18745529: D18663.id44809.diff
Oct 3 2025, 7:46 AM
F18732088: D18663.diff
Sep 30 2025, 4:14 PM
F18697564: D18663.diff
Sep 27 2025, 9:48 AM
F18646685: D18663.id.diff
Sep 19 2025, 10:44 AM
Subscribers

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

Screen Shot 2017-09-29 at 1.35.51 PM.png (394×670 px, 53 KB)

Diff Detail

Repository
rP Phabricator
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

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.

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.