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)
Feb 24 2024, 2:55 PM
Unknown Object (File)
Feb 10 2024, 2:47 AM
Unknown Object (File)
Feb 3 2024, 3:27 PM
Unknown Object (File)
Jan 28 2024, 9:55 AM
Unknown Object (File)
Jan 25 2024, 3:00 AM
Unknown Object (File)
Jan 24 2024, 7:15 PM
Unknown Object (File)
Jan 16 2024, 12:54 AM
Unknown Object (File)
Jan 6 2024, 5:15 PM
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
Branch
review1
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 16334
Build 21721: Run Core Tests
Build 21720: arc lint + arc unit

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!