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

Tiriel · 2017-11-04T21:45:54Z

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

Ran lint and tests.

Comments and reviews deeply welcomed!

codecov-io · 2017-11-04T21:47:47Z

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%)`	⬆️
lib/reviews.js	`98.41% <100%> (+0.23%)`	⬆️
lib/args.js	`100% <100%> (ø)`	⬆️

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 · 2017-11-04T22:51:17Z

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

Tiriel · 2017-11-04T23:02:13Z

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

Tiriel · 2017-11-05T00:10:09Z

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 · 2017-11-05T00:13:30Z

lib/reviews.js

+   * @returns {boolean}
+   */
+  hasLGTM(object, prop) {
+    return LGTM_RE.test(object[prop]);


So this is too loose. It would pass for I'm revoking my LGTM.
IMHO this should be object[prop].toLowerCase() === 'lgtm'.

@refack This is already the case for normal comments, I think we should log an info here, like what we do for those, to remind the landers to double-check. The actual context would be too hard to parse without some kind of natural language processing..

OK maybe not log an info here, but make a new type of REVIEW_SOURCES, maybe FROM_REVIEW_COMMENT, and warn them in checkReviews like what we do for FROM_COMMENT

@refack re: I'm revoking my LGTM. they would've put a red cross there I think? (Although if people could click the wrong button for approvals then they might click the same one for objections..

I'm thinking of something I saw last week - nodejs/node#16612 (review)

That case was not a "Request Changes" , just "Don't count me as approver", same as dismissed review.

Another idea: if we are uncertain about a LGTM, we can log it as ?Reviewed-By: xxx, and then core-validate-commit should complain about it (it doesn't at the moment, we can update that. For now we can just warn about them ourselves).

On a second thought, I am finding the object[prop].toLowerCase() === 'lgtm' approach better (although we can just do that by changing LGTM_RE to /^lgtm$/i and trim the comments before testing them), it's just simpler that way. I think people who don't give LGTM either this way or with a green review would not mind not being listed in the review-by list. Also it saves you the trouble of listing them back manually if they are already in the metadata.

joyeecheung

Most bits LGTM but I think we should log something about the source, like what we do for LGTM in normal comments (hey look at me using LGTM in an CHANGES_REQUESTED XD)

And I'm dismissing my review, so we'll have good data

refack · 2017-11-05T02:07:10Z

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

joyeecheung · 2017-11-05T02:14:44Z

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

Tiriel · 2017-11-05T09:05:54Z

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 · 2017-11-05T09:10:56Z

For now, that's what I got:

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

Tiriel · 2017-11-05T09:54:53Z

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?

apapirovski · 2017-11-05T12:21:03Z

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 · 2017-11-05T12:25:16Z

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 · 2017-11-05T12:28:46Z

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 · 2017-11-05T16:04:05Z

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

apapirovski · 2017-11-05T16:06:12Z

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

apapirovski · 2017-11-05T16:06:41Z

Fat fingered the close button. Sorry.

Tiriel · 2017-11-05T16:23:12Z

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

joyeecheung · 2017-11-05T16:40:14Z

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

joyeecheung · 2017-11-05T16:45:47Z

@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 · 2017-11-05T17:14:11Z

For now that's what I got:

Is that okay?

Tiriel · 2017-11-05T17:16:42Z

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 · 2017-11-05T17:22:31Z

Actually this way other tests fail.

Going to rework already :)

joyeecheung · 2017-11-07T09:02:46Z

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

Tiriel · 2017-11-07T09:25:04Z

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

joyeecheung · 2017-11-07T12:39:01Z

lib/reviews.js

+        case COMMENTED:
+          map.set(
+            login,
+            new Review(r.state, r.publishedAt, r.bodyText, FROM_REVIEW_COMMENT)


I suggest we just pass APPROVED as the state here, like what we do for approval in normal comments. This way we don't need to call isApprovedInComment in getReviewers. We still keep the info about its source in review.source.

Oh thanks, that's what I had done in the first place, forgot to reinstate it when reverting

joyeecheung · 2017-11-07T12:44:56Z

test/fixtures/reviews_approved.json

+  "bodyText": "LGTM",
+  "state": "COMMENTED",
+  "author": {
+    "login": "Baz"


Can you make a new case for this? This and the case above are overlapping each other so one of them would not be visible in the tests. To do this we need to add another collaborator in collaborators.json and the readme in the fixtures as well.

Sure thing!

There are in fact numerous tests using each files. I also had to add an entry in reviewers_approved.json and modify accordingly other tests. Push coming

joyeecheung · 2017-11-07T12:47:20Z

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 · 2017-11-07T14:29:32Z

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

joyeecheung

Very close!

joyeecheung · 2017-11-08T02:54:10Z

lib/args.js

@@ -37,6 +37,12 @@ function buildYargs(args = null) {
      describe: 'File to write the metadata in',
      type: 'string'
    })
+    .option('comments', {


I'd rather let this be --check-comments with no aliases, in case we need the c later. This does not seem to be very in demand.

Your desires are my commands!

joyeecheung · 2017-11-08T02:55:30Z

test/unit/pr_checker.test.js

@@ -80,12 +80,13 @@ describe('PRChecker', () => {

      const expectedLogs = {
        warn: [
+          ['Quux User(Quux)) approved in via LGTM in comments'],


There is a redundant ) here..

Actually this redundant parens is in every test and is in fact in the base template :D
I'll remove it everywhere.

Tiriel · 2017-11-08T09:33:16Z

Fixed!

priyank-p · 2017-11-08T14:09:22Z

@Tiriel can you add this option in readme.

Tiriel · 2017-11-08T14:14:31Z

Very good idea! Incoming!

priyank-p

Very minor nits. I think we are good to merge it.

priyank-p · 2017-11-08T14:17:56Z

lib/args.js

@@ -37,6 +37,11 @@ function buildYargs(args = null) {
      describe: 'File to write the metadata in',
      type: 'string'
    })
+    .option('check-comments', {
+      demandOption: false,
+      describe: 'Check for\'LGTM\' in comments',


Same as above^

priyank-p · 2017-11-08T14:18:43Z

README.md

+  --owner, -o       GitHub owner of the PR repository                   [string]
+  --repo, -r        GitHub repository of the PR                         [string]
+  --file, -f        File to write the metadata in                       [string]
+  --check-comments  Check for'LGTM' in comments                        [boolean]


Minor Nit: add a space between for and LGTM 'Check for \'LGTM\' in comments'

Tiriel · 2017-11-08T15:23:11Z

Going to merge then, unless someone objects!

refack suggested changes Nov 5, 2017

View reviewed changes

refack previously approved these changes Nov 5, 2017

View reviewed changes

joyeecheung requested changes Nov 5, 2017

View reviewed changes

joyeecheung mentioned this pull request Nov 5, 2017

feature request: support new syntax for uncertain metadata nodejs/core-validate-commit#23

Closed

apapirovski closed this Nov 5, 2017

apapirovski reopened this Nov 5, 2017

Tiriel force-pushed the feature/approve-commented branch from bdce824 to 6004aed Compare November 5, 2017 17:17

Tiriel force-pushed the feature/approve-commented branch from 6004aed to 6779760 Compare November 6, 2017 13:53

Tiriel force-pushed the feature/approve-commented branch from 6ab5a4e to 0f48d22 Compare November 7, 2017 09:19

joyeecheung reviewed Nov 7, 2017

View reviewed changes

Tiriel added 8 commits November 7, 2017 14:18

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

ed5b97e

Added test case and reworked accordingly

4c15b16

[SQUASH] added missing comment

0992b79

Rebase and rework, approving comments displayed in warning

37cff9d

Rework worklfow + adapt tests to new workflow

a053af6

Fix bad var name

0868d7a

Remove rework, change RegExp to stricter one

f43df7e

Add tests cases and comments flag

b9b6608

Tiriel force-pushed the feature/approve-commented branch from 0f48d22 to b9b6608 Compare November 7, 2017 14:28

joyeecheung reviewed Nov 8, 2017

View reviewed changes

Changed flag, removed unnecessary parens

42113f1

joyeecheung approved these changes Nov 8, 2017

View reviewed changes

Update README.md for new option

3e5c88a

priyank-p approved these changes Nov 8, 2017

View reviewed changes

alopezsanchez approved these changes Nov 8, 2017

View reviewed changes

Few nits!

f3f4afb

priyank-p approved these changes Nov 8, 2017

View reviewed changes

Tiriel merged commit 1d05665 into nodejs:master Nov 8, 2017

Tiriel deleted the feature/approve-commented branch November 8, 2017 15:37

Tiriel mentioned this pull request Nov 8, 2017

todo: account LGTM in "COMMENTED" reviews #67

Closed

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

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

Conversation

Tiriel commented Nov 4, 2017

codecov-io commented Nov 4, 2017 • edited Loading

Codecov Report

priyank-p commented Nov 4, 2017

Tiriel commented Nov 4, 2017

Tiriel commented Nov 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joyeecheung Nov 5, 2017 • edited Loading

Choose a reason for hiding this comment

joyeecheung Nov 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joyeecheung Nov 5, 2017 • edited Loading

Choose a reason for hiding this comment

joyeecheung Nov 7, 2017 • edited Loading

Choose a reason for hiding this comment

joyeecheung left a comment

Choose a reason for hiding this comment

refack commented Nov 5, 2017

joyeecheung commented Nov 5, 2017

Tiriel commented Nov 5, 2017

Tiriel commented Nov 5, 2017 • edited Loading

Tiriel commented Nov 5, 2017

apapirovski commented Nov 5, 2017 • edited Loading

apapirovski commented Nov 5, 2017 • edited Loading

Tiriel commented Nov 5, 2017 • edited Loading

joyeecheung commented Nov 5, 2017

apapirovski commented Nov 5, 2017 • edited Loading

apapirovski commented Nov 5, 2017

Tiriel commented Nov 5, 2017

joyeecheung commented Nov 5, 2017 • edited Loading

joyeecheung commented Nov 5, 2017 • edited Loading

Tiriel commented Nov 5, 2017 • edited Loading

Tiriel commented Nov 5, 2017

Tiriel commented Nov 5, 2017

joyeecheung commented Nov 7, 2017

Tiriel commented Nov 7, 2017 • edited Loading

Choose a reason for hiding this comment

Tiriel Nov 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joyeecheung commented Nov 7, 2017

Tiriel commented Nov 7, 2017

joyeecheung left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tiriel commented Nov 8, 2017

priyank-p commented Nov 8, 2017

Tiriel commented Nov 8, 2017

priyank-p left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tiriel commented Nov 8, 2017

codecov-io commented Nov 4, 2017 •

edited

Loading

Tiriel commented Nov 5, 2017 •

edited

Loading

joyeecheung Nov 5, 2017 •

edited

Loading

joyeecheung Nov 5, 2017 •

edited

Loading

joyeecheung Nov 5, 2017 •

edited

Loading

joyeecheung Nov 7, 2017 •

edited

Loading

Tiriel commented Nov 5, 2017 •

edited

Loading

apapirovski commented Nov 5, 2017 •

edited

Loading

apapirovski commented Nov 5, 2017 •

edited

Loading

Tiriel commented Nov 5, 2017 •

edited

Loading

apapirovski commented Nov 5, 2017 •

edited

Loading

joyeecheung commented Nov 5, 2017 •

edited

Loading

joyeecheung commented Nov 5, 2017 •

edited

Loading

Tiriel commented Nov 5, 2017 •

edited

Loading

Tiriel commented Nov 7, 2017 •

edited

Loading

Tiriel Nov 7, 2017 •

edited

Loading