code review 6815048: misc/dashboard/codereview: only accept "NOT LGTM" on th...

misc/dashboard/codereview: only accept "NOT LGTM" on the first line of a message.

Too many people quote entire emails and put their reply at the top ("top posting"),
so we shouldn't recognise review commands anywhere in the review text.

[dsymonds, 2012-10-29 10:57:06]

Hello golang-dev@googlegroups.com,

I'd like you to review this change to
https://code.google.com/p/go

[bradfitz, 2012-10-29 11:02:11]

LGTM

or /(?m)^NOT LGTM/ would also be acceptable and maybe less of a behavior
change?

But I only see top-line LGTM statements anyway.

On Mon, Oct 29, 2012 at 11:57 AM, <dsymonds@golang.org> wrote:

> Reviewers: golang-dev_googlegroups.com,
>
> Message:
> Hello golang-dev@googlegroups.com,
>
> I'd like you to review this change to
> https://code.google.com/p/go
>
>
> Description:
> misc/dashboard/codereview: only accept "NOT LGTM" on the first line of a
> message.
>
> Too many people quote entire emails and put their reply at the top ("top
> posting"),
> so we shouldn't recognise review commands anywhere in the review text.
>
> Please review this at
http://codereview.appspot.com/**6815048/<http://codereview.appspot.com/6815048/>
>
> Affected files:
>   M misc/dashboard/codereview/**dashboard/cl.go
>
>
> Index: misc/dashboard/codereview/**dashboard/cl.go
> ==============================**==============================**=======
> --- a/misc/dashboard/codereview/**dashboard/cl.go
> +++ b/misc/dashboard/codereview/**dashboard/cl.go
> @@ -389,7 +389,7 @@
>                         lgtm[s] = true
>                         delete(notLGTM, s) // "LGTM" overrules previous
> "NOT LGTM"
>                 }
> -               if strings.Contains(msg.Text, "NOT LGTM") {
> +               if strings.Contains(line, "NOT LGTM") {
>                         notLGTM[s] = true
>                         delete(lgtm, s) // "NOT LGTM" overrules previous
> "LGTM"
>                 }
>
>
>

[dsymonds, 2012-10-29 11:03:36]

I believe this matches what Rietveld does.

[dsymonds, 2012-10-29 11:04:01]

*** Submitted as http://code.google.com/p/go/source/detail?r=e6aee4c1804a ***

misc/dashboard/codereview: only accept "NOT LGTM" on the first line of a
message.

Too many people quote entire emails and put their reply at the top ("top
posting"),
so we shouldn't recognise review commands anywhere in the review text.

R=golang-dev, bradfitz
CC=golang-dev
http://codereview.appspot.com/6815048

[minux1, 2012-10-29 11:20:22]

rietveld seems to search for "not lgtm" in the whole message.
(it regards brad's last comment as disapproval)

http://code.google.com/p/rietveld/source/detail?r=756dc683ac1060be54e23e6f913...

[dsymonds, 2012-10-29 11:35:15]

On Mon, Oct 29, 2012 at 10:20 PM,  <minux.ma@gmail.com> wrote:

> rietveld seems to search for "not lgtm" in the whole message.
> (it regards brad's last comment as disapproval)

Sounds like we should fix it, then.