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
Branch
package1
Lint
Lint OK
Unit
Unit Tests OK
Build Status
Buildable 18588
Build 25037: Run Core Tests
Build 25036: arc lint + arc unit

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.