Fixing review state to APPROVED whe 'LGTM' in COMMENTED review

[Tiriel]

From #67 Tested with nodejs/node#16248 as given in issue #67

Ran lint and tests.

Comments and reviews deeply welcomed!

[codecov-io]

Codecov Report

Merging #72 into master will increase coverage by 0.03%. The diff coverage is 100%.

@@            Coverage Diff             @@
##           master      #72      +/-   ##
==========================================
+ Coverage   98.09%   98.13%   +0.03%     
==========================================
  Files          13       13              
  Lines         473      482       +9     
==========================================
+ Hits          464      473       +9     
  Misses          9        9

Impacted Files	Coverage Δ
lib/pr_checker.js	96.47% <100%> (+0.02%)	:arrow_up:
lib/reviews.js	98.41% <100%> (+0.23%)	:arrow_up:
lib/args.js	100% <100%> (ø)	:arrow_up:

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 744a34d...f3f4afb. Read the comment docs.

[priyank-p]

@Tiriel Great Work! can you add tests in test/unit/reviews.test.js

[Tiriel]

Oh yeah sorry, that's indeed a good idea. I'm on it!

[Tiriel]

Original tests were in fact all that was needed. I just had to add a test-case in fixtures for my first draft to make some tests fail. (pr_data tests in fact). Adding the test case was indeed a great idea, thanks @cPhost !

Corrected work, now tests still pass with proper test case, and command still responds properly.

[refack]

BTW: people can also dismiss other's reviews, so need to keep track of the author attribution

[joyeecheung]

@refack Yes, we probably need to dig a bit more into the state machine later as well..

[Tiriel]

Okay, I've read all the comments, bit confusing xD

I think I'll go for both things you suggested if possible @joyeecheung , new source and ?in report.

@joyeecheung @refack Otherwise, what about changing the RegEx to this: ^(L|S)GTM!?$ ? This would prevent most cases of matches when people are discussing "LGTM" without actually wanting to validate a PR and still allowing some leeway, don't you think?

[Tiriel]

By the way, I see that the info telling one PR was approved in comments or commented review is on console.info. Wouldn't it be better to pass it to console.warn?

[Tiriel]

For now, that's what I got:

Not committed yet, Just want to see if it's ok for you ;)

[apapirovski]

I know I'm going to be a minority on this but I would scrape the whole LGTM checking (including the existing). Everyone uses the green button. If they're not using and they say 'LGTM' or something that extent, it usually means they scanned it but don't have complete confidence to give it firm approval.

I don't think the few times we'll find a legitimate approval that wasn't logged as such will be more frequent than the comments with LGTM where it's a part of a broader statement that is not necessarily an approval.

[apapirovski]

For example, what does this count as? (This is from a recent PR I landed.)

Basically LGTM, just note that automatic fixing can introduce bugs if another variable of the same name was declared within the function.

I don't think the author would expect to be listed in Reviewed-By even though they gave a LGTM.

Or the more common:

LGTM but I'm not an expert on this domain.

[Tiriel]

That's a good point, but actually that's why I would put the notice in warning.

In a way, just to say "Some people seem to approve, but be careful".

And that's also why I'd say we only register "LGTM" when it's the only thing in the comment (my RegEx proposal)

[joyeecheung]

Or....maybe we should skip those people in the actual metadata, just log them as "potential reviewers"?

[apapirovski]

@joyeecheung I think that's my favourite suggestion so far.

[apapirovski]

Fat fingered the close button. Sorry.

[Tiriel]

Maybe then keeping the line "XXX reviewed in comment" while removing the review from review count?

[joyeecheung]

@Tiriel Yes that's what I had in mind.

[joyeecheung]

@Tiriel Or maybe..

The following collaborators are not listed in the metadata,
please add them manually if you think their comments count as approvals:
Foo <foo@example.org>: I am not sure about if I can give a proper LGTM but this is a great idea
Bar <bar@example.org>: LGTM modulo style nits

(we can even make this interactive and let the user decide!...although for the moment I think this PR is good with just removing them in the metadata)

[Tiriel]

For now that's what I got: Is that okay?

[Tiriel]

I can do like you said in your last comment, but I feel that would need a more important rework of the workflow that would justify another PR

[Tiriel]

Actually this way other tests fail.

Going to rework already :)

[Tiriel]

Hey!

Finally reworked a bit. I needed to adapt the tests, since the LGTM in comments don't count as approvals now. The result looks like this:

Hope it's ok now!

[joyeecheung]

@Tiriel Sorry for so much back-and-forth, but I actually kinda see the point of https://github.com/joyeecheung/node-core-utils/pull/72#discussion_r148944765 now, we might as well just ignore the reviews that are not just ^\s*lgtm\s*$/i, in that case we don't really need to warn about anything because it's pretty clear that comments with only LGTM are legitimate sign-offs. As for the rest with additional texts we can live with ignoring those (personally I usually ignore those if I had to generate the metadata manually, because when there are additional texts it's seldom the case that the author is actually saying that they approve).

[Tiriel]

No prob! I'll try and rollback to the previous version then. What do you prefer? r.bodyText.toLowerCase() === 'lgtm', or changing the regex to /^lgtm\W?$/gmi?

Personnally I prefer modifying the regex, I've seen people adding punctuation to LGTMs and making a strict equal comparison wouldn't count them.

[joyeecheung]

@Tiriel Let's go with regexp then. Also we might want to trim the text before testing them as well.

[Tiriel]

Done! Rebased, linted and tested as usual (now) :D

[joyeecheung]

Also with this we can probably drop the check for FROM_COMMENT in the pr checker, or put it behind a command line flag and warn about both FROM_COMMENT and FROM_REVIEW_COMMENT (but I don't think we really need a warning for that any more..)

[Tiriel]

There, this should do it. I added the --comments flag and some tests cases. I'll let you review, since I had to adapt some existing tests cases

[Tiriel]

Fixed!

[priyank-p]

@Tiriel can you add this option in readme.

[Tiriel]

Very good idea! Incoming!

[Tiriel]

Going to merge then, unless someone objects!