Add 'disapprovals'

[nicolasbock]

Use Scenario

A reviewer is unhappy with the current state of the PR. They express they disapproval with a -1.

New Behavior

The lgtm approver counts all approvals as +1 and all disapprovals as -1, tallies them up, and only if their sum is greater than a configurable threshold is the PR approved.

Requirements

Configurable set of regexpressions for recognized disapprovals.

[bradrydzewski]

just wanted to note that we should probably make the disapproval pattern configurable:

approvals = 1
pattern = "(?i)LGTM"
+disapprovals = "(?i):\\-1:"

[nicolasbock]

The option naming is inconsistent at this point (approvals means something completely different than disapprovals). A more consistent approach would be:

approval_threshold = 2
approval_pattern = "(?i)LGTM"
disapproval_pattern = "(?i):\\-1:"

[tboerger]

Naming it approval_pattern and disapproval_pattern makes sense for me, but than we need to fallback to the old behavior to avoid breaking installs.

[ljharb]

Probably disapproval_threshold would make sense too? That way people could set it to 0 to disable, or 2 to require 2 -1s before something's blocked, etc

[bradrydzewski]

The option naming is inconsistent at this point (approvals means something completely different than disapprovals). A more consistent approach would be:

while I agree, unfortunately this would be a breaking change for projects using hosted lgtm.co. I would probably prefer a non-uniform solution for the initial implementation, and we can address naming consistency / file format changes in a separate pull request.

Probably disapproval_threshold would make sense too? That way people could set it to 0 to disable, or 2 to require 2 -1s before something's blocked, etc

@ljharb I hadn't thought of this, but seems to make sense. If too many people dislike the pull request drone refuses to unlock it. This should probably be an optional feature. In fact, disapprovals in general should probably be opt-in since I could see people not expecting that behavior and opening issues telling me everything is broken :)

[ljharb]

Right - disapproval_threshold would default to 0 which means it can't be disapproved.

[ljharb]

Also you could totally do approval_threshold || approvals and approval_pattern || pattern for now to make it semver-minor, and in the next semver-major, drop the || fallbacks.

[bradrydzewski]

@ljharb we can definitely update the naming conventions and map the old file values. If we adjust the file properties we could also consider nesting. The file (with all options) would then look something like this:

[approval]
threshold = 2
pattern = "(?i)LGTM"
disable_self = true

[disapproval]
enabled = true # if false, won't look for the disapproval pattern
threshold = 2 # if exceeded the pr cannot be unlocked by lgtm. if zero, the pr can be unlocked if lgtm approvals - disapprovals > threshold
pattern = "(?i):\\-1:"
disable_self = true

[maintainers]
path = "MAINTAINERS" # customize file path for teams that place in .github/MAINTAINERS
team = "maintainers" # customize team name container approver list

[ljharb]

That sounds like a great idea! Should enabled and disable_self both be on both approval and disapproval? I'd think so.

Separately, what happens if the approval threshold is set to 0? I'd assume setting either threshold to 0 would also set that category's "enabled" to false.

[ljharb]

It would also be nice if "maintainers" defaulted to, or had the option to be set to, "whoever github has set up with write privileges"

[bradrydzewski]

That sounds like a great idea! Should enabled and disable_self both be on disapproval? I'd think so.

yep makes sense. I updated the spec above

Separately, what happens if the approval threshold is set to 0? I'd assume setting either threshold to 0 would also set that category's "enabled" to false.

unfortunately the zero value of an integer is 0 in Go, which means we can't easily differentiate between 0 and unset. So if something is 0 we assume it is just unset

[ljharb]

Too bad, that sounds like a serious flaw in Go :-/

[bradrydzewski]

Too bad, that sounds like a serious flaw in Go :-/

the workaround in Go is usually to create a custom type (called NullInt for example) that wraps an integer and implements a custom unmarshaler for that type. So it is possible but somewhat annoying to implement https://godoc.org/github.com/BurntSushi/toml#Unmarshaler

It would also be nice if "maintainers" defaulted to

yep, we do this today. When we encounter a configuration option that has a zero value (empty string, 0 integer) we use the defaults which are configurable by environment variable (at the server level)

or had the option to be set to, "whoever github has set up with write privileges:

we don't provide this option today but it certainly sounds like a good idea. I'm thinking something like collaborators = true although I'm not sure that is explicit enough. Open to ideas

[maintainers]
path = "MAINTAINERS"
team = "maintainers"
collaborators = true

[ljharb]

hmm, that's not been my experience - when enabling LGTM on a new repo that lacks a config file, nobody's able to approve anything :-/