nodejs / node-core-utils

CLI tools for Node.js Core collaborators
https://nodejs.github.io/node-core-utils/
MIT License
234 stars 106 forks source link

Added todo warn `Warn new commits after reviews` #56

Closed priyank-p closed 6 years ago

priyank-p commented 6 years ago

It warns if there has been new commits after the last review if so it warns and then outputs two links to one of the commit link and other link for all the changes that the PR has.

priyank-p commented 6 years ago

The tests are coming soon probably early tomorrow. Just wanted to get it in so you guys could review it. Definitely request changes on naming things differently.

Sample output below. screen shot 2017-11-02 at 9 38 56 pm

codecov-io commented 6 years ago

Codecov Report

Merging #56 into master will not change coverage. The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #56   +/-   ##
=======================================
  Coverage   96.54%   96.54%           
=======================================
  Files          13       13           
  Lines         434      434           
=======================================
  Hits          419      419           
  Misses         15       15

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 e21cf5a...12c3372. Read the comment docs.

joyeecheung commented 6 years ago

Also can you add some tests in test/unit/pr_checker.test.js? Thanks!

priyank-p commented 6 years ago

I will probably close this PR and make a new one with test and all the suggestions and fixes.

priyank-p commented 6 years ago

@apapirovski / anybody what about this output compared to before?

[WARN] There has been new commits pushed to Pull Request since the last review
[WARN] [New Commit] src: improve module loader readability
[WARN] New commit url:  https://github.com/nodejs/node/pull/16536/commits/b619e41d8b2066fcab6459cea0f49f6924200a37
apapirovski commented 6 years ago

I think a combination of what @tniessen proposed and something like

[WARN] Changes were pushed since the last review: [WARN] src: improve module loader readability [WARN] nit: fix error [WARN] See https://github.com/nodejs/node/pull/16536/commits for more info

I'm not a big fan of adding [commit] or something similar to each line because once you're familiar with the tool it just means you have to look that far further right to start reading. But maybe others have different thoughts on that.

Adding link to each commit seems noisy but I'm open to it if others prefer it. (Maybe if it's just one commit, it links to it and if it's multiple it links to the aggregate.)

tniessen commented 6 years ago

[WARN] There has been new commits pushed to Pull Request since the last review

See https://github.com/joyeecheung/node-core-utils/pull/56#discussion_r148713805. You don't need to use that suggestion but it should conform to English grammar.

[WARN] [New Commit] src: improve module loader readability

Why is New Commit enclosed in brackets while New commit url is not? Why is Commit capitalized in New Commit but not in New commit url?

[WARN] New commit url: https://github.com/nodejs/node/pull/16536/commits/b619e41d8b2066fcab6459cea0f49f6924200a37

Why are there two spaces in front of the URL? (which are not visible here, thanks to GitHub's formatting)

priyank-p commented 6 years ago

@tniessen, I like format by @apapirovski.

apapirovski commented 6 years ago

I just tried to ninja edit but I'll post it separately, in case people miss it:

Maybe if it's just one commit, it links to it and if it's multiple it links to the aggregate commits page.

priyank-p commented 6 years ago

added new pr