Page MenuHomePhabricator

Allow Herald rules to apply the "exist" and "do not exist" conditions to the "Reviewers" field
Closed, ResolvedPublic


I'm trying to design some Herald rules to support a workflow for when we start actively using Phabricator. I want to be able to support a couple behaviors:

(1. I'd like for my engineers to be able to store "work in progress" differential revisions that aren't yet ready for review (rather than on their laptops when can be eaten by a bear, struck by lightning, or sold to support crippling drug habits). I think that arc diff --plan-changes can work for this, but I think a new status of "Work in Progress" would be apropos (e.g., arc diff --wip)). Dupes T2543

  1. I'd like for my engineers to not worry too much about who is going to review their code, so I'd like them to be able to leave the reviewers field empty when populating their revision. Then, based on herald rules that apply some heuristics (which repository, which directory within the repository) will automatically select some reviewers. (Of course, if they nominate a reviewer, the herald rules should do nothing. However, I don't want to pester potential reviewers until the review exits the "work in progress" status. I see a couple problems a) It's not clear how to create a Hearald rule that matches the empty set. b) Herald doesn't currently have a capability to test against the status of a revision.

(3. It might be nice to have a "pester" button on a differential revision; this would be used by an engineer that they'd like feedback from a particular reviewer that is listed on a revision.) Moved to T4226

Event Timeline

wotte raised the priority of this task from to Needs Triage.
wotte updated the task description. (Show Details)
wotte added projects: Herald, Differential.
wotte updated the task description. (Show Details)
wotte added a subscriber: wotte.

T2543 meanders around but eventually discusses a "Draft" or "In Preparation" phase, which I think would be analogous to your "Work in Progress" phase in (1). This likely remains blocked until T2222.

This phase would not trigger Herald rules, so that should solve the "no triggers until phase exit" part of (2). For the other part of (2), we should allow you to apply the "exist" and "do not exist" conditions to the "Reviewers" field, so you can write [Reviewers] [do not exist] to detect no reviewers (maybe we can find a slightly more natural phrasing).

I'm very hesitant to build (3) into the product. We went down the "nagging" route a bit at Facebook and it seemed to create more annoyance than results. I think this would imply a lot of weird social context, too. I can file something explicit to discuss this in greater detail.

wotte renamed this task from Add herald rule for Differential revisions based on status to Allow Herald rules to apply the "exist" and "do not exist" conditions to the "Reviewers" field.Dec 11 2013, 7:31 PM

I've edited the issue based on your feedback. I think T2543 would address 1. If the outcome if T2543 would either a) not match against Herald rules, or b) a new Herald rule was added to match against the status of a revision, T2543 would also solve most of 2.

I think matching against the empty set for Reviewers would be useful, so I've changed this issue to address that.

I've opened a separate issue T4226 to discuss pester/nag, which I think can be a useful tool.

(T2443 may also be of interest, vaguely.)

(2b) should be fixed at HEAD, let us know if you run into any issues with it. T2543 should happen sooner or later, but T2222 is a bit of work and doesn't have a concrete place on the roadmap yet.