What are the use cases you have for these fields?

If we made fields pluggable like D8784 will make actions pluggable, would that solve this more cleanly (via defining a Is High Risk field, or whatever else your use case involves)?

Generally, I don't really want to turn Herald into a scripting language. While doing value comparisons isn't terrible, it's a step in that direction. Offhand, it's hard for me to imagine these fields being used for a variety of different purposes by different users, and I can't come up with too many other uses for value comparisons.

Oh I didn't see that comment there.

The way the risk review system we have works is that it's a JSON array. When "add risk review" or "remove risk review" is triggered, it modifies the value of the JSON field and adds or removes items as appropriate. Hence risk review involves a developer's estimation (1-3) as well as automatically detected things that increase or decrease risk (i.e. are you changing half the code base?)

Being able to detect when there is no unit test coverage or when the developer has explicitly skipped unit / lint and increasing the risk rating is pretty important. The LOC values are being used as a general guide to ascertain the size of a diff (we increase the risk at various size points so that it scales with the size of the diff rather than just a "is it a big diff or not" toggle).

Do you need to write many different rules with different values for these fields, based on other factors like project/repository/author, and expect users to write these rules in a self-serve way?

Suppose you had this rule instead:

When [Always], take these actions: [Assess Risk]

...where "Assess Risk" is a custom action which simply hard-codes the risk computation. Would that solve things?

It's better if it's self-managed and people can set them up just through the Herald interface. The alternative is they'd need to jump onto some Linux box, mess around with some PHP, push the new extension code, let everyone know there's an upgrade going to happen, run ./update_phabricator.sh and hope things don't break.

Alright. I don't want to pull this into the upstream for now since its use is very narrow (it solves one specific problem for one install in a relatively inflexible way), and is narrow in focus (hard-codes the specific fields and values required for this one narrow use case) and it touches parts of the code which are change-prone in the future (lint/unit integration with Differential), and there are plausible workarounds which accomplish a similar goal, and if we can externalize fields in the way D8784 externalizes actions we don't need this anyway.

I would accept this if some of these things happened:

• it had a stronger, broader, or more common use case behind it;
• it expands support (e.g., detect passing tests, not just skipped/no tests) and/or use cases make a strong case that no expansion is required;
• lint/unit APIs from Differential stabilize; and
• we decide not to externalize fields for whatever reason.

However, my current evaluation is that the utility here is too narrow to justify inclusion. Particularly, we can't ever back out of this without breaking some users, since it creates new public API.