Page MenuHomePhabricator

Provide a "Reviewers" attachment to "differential.revision.search"
ClosedPublic

Authored by epriestley on Apr 6 2017, 9:29 PM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Jun 27, 12:52 AM
Unknown Object (File)
Fri, Jun 24, 6:18 AM
Unknown Object (File)
Sat, Jun 18, 3:02 AM
Unknown Object (File)
Fri, Jun 17, 11:58 AM
Unknown Object (File)
Thu, Jun 16, 6:04 PM
Unknown Object (File)
Sun, Jun 12, 4:27 AM
Unknown Object (File)
Sat, Jun 11, 10:29 AM
Unknown Object (File)
Fri, Jun 10, 11:48 AM
Subscribers

Details

Summary

Allow API callers to retrieve reviewer information via a new "reviewers" attachment.

Test Plan

Screen Shot 2017-04-06 at 2.27.47 PM.png (1×2 px, 237 KB)

Diff Detail

Repository
rP Phabricator
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

  • Get rid of isCurrent which is kind of meaningless and confusing.
  • Add an isBlocking to try to future-proof against that being separated out.
lvital added a subscriber: lvital.

noidea

src/applications/differential/engineextension/DifferentialReviewersSearchEngineAttachment.php
43

I know you're stripping this away, but out of curiosity, what would this have meant / why is it too complicated? Does it mean whether they've taken any action on the review? Or whether they've taken the last action?

This revision is now accepted and ready to land.Apr 6 2017, 9:40 PM

The meaning of isCurrent was "is this reviewer relevant for state calculations against the current state of the revision", which is kind of involved.

For example, a revision with one user who has "Rejected" is normally in state "Needs Revision", until the next update, but not if the author does "Request Review". When they do, they internally void the outstanding rejects and make them noncurrent.

But current-ness depends on a lot of internal state stuff and on differential.sticky-accept and I think it's ultimately too wrapped up in state calculation to be of much use. At best, you could use it to render "Accepted Current Diff" vs "Accepted Older Diff" some of the time, but it doesn't always mean that and there's no real human-readable label for what it does mean.

This revision was automatically updated to reflect the committed changes.

The meaning of isCurrent was "is this reviewer relevant for state calculations against the current state of the revision", which is kind of involved.

For example, a revision with one user who has "Rejected" is normally in state "Needs Revision", until the next update, but not if the author does "Request Review". When they do, they internally void the outstanding rejects and make them noncurrent.

But current-ness depends on a lot of internal state stuff and on differential.sticky-accept and I think it's ultimately too wrapped up in state calculation to be of much use. At best, you could use it to render "Accepted Current Diff" vs "Accepted Older Diff" some of the time, but it doesn't always mean that and there's no real human-readable label for what it does mean.

Makes some sense... or rather, makes sense why you removed it. Thanks!