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
F12839873: D18663.diff
Thu, Mar 28, 7:58 PM
Unknown Object (File)
Fri, Mar 15, 5:21 PM
Unknown Object (File)
Feb 5 2024, 10:05 PM
Unknown Object (File)
Feb 3 2024, 5:00 PM
Unknown Object (File)
Feb 3 2024, 5:39 AM
Unknown Object (File)
Jan 3 2024, 4:34 AM
Unknown Object (File)
Jan 2 2024, 2:00 AM
Unknown Object (File)
Dec 27 2023, 12:14 PM
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.