Closed Tiriel closed 6 years ago
Merging #92 into master will increase coverage by
0.01%
. The diff coverage is100%
.
@@ Coverage Diff @@
## master #92 +/- ##
=========================================
+ Coverage 98.09% 98.1% +0.01%
=========================================
Files 13 13
Lines 473 476 +3
=========================================
+ Hits 464 467 +3
Misses 9 9
Impacted Files | Coverage Δ | |
---|---|---|
lib/pr_checker.js | 96.52% <100%> (+0.07%) |
: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...2008fb4. Read the comment docs.
Refined ;)
(Please see my 'outdated' reviews too, bad timing on my part but the feedback is still valid.)
By the way, the missing time limit isn’t because a PR is made for Code & Learn, it’s because it’s touching only a specific part of the tests or documentation – maybe we can work towards checking whether a PR does that, as a future goal :)
@addaleax meaning if it has test
or doc
label.
@addaleax is there specific labels to look for then?
@cPhost @apapirovski thanks for the input, gonna implement them asap!
I'm going to end up with the most commented and reviewed PRs each time it seems... Dunno if it's good or bad, but it feels like I have lots to learn yet Thanks everyone for taking the time!
I'm going to end up with the most commented and reviewed PRs each time it seems... Dunno if it's good or bad, but it feels like I have lots to learn yet.
The more comments it has the better the PR Gets. 👍
I think test
and doc
are good labels to check for, but checking whether the commit only affects one or two test/doc files would be ideal (also much harder to implement I guess, so I wouldn’t suggest it for this PR?)
Thanks @addaleax ! I feel it would be weird to ship the new labels condition without the number of files, so I'll just open a new issue so we can agree on the max number of files to check.
@addaleax I would be careful about test-only because some tests are trickier than the others (I am looking at async-wrap-uncaughtexception
, sign), but doc-only changes seem to be safe enough, I think "having only the doc label" is a pretty good indication?
@addaleax Also we can just add a new label like "fast-track"...I think I've put it in the source code as a todo somewhere, lol
I added the parsing for 'doc' label only, just in case. Otherwise, I just wait for your reviews ;)
@joyeecheung I’m fine with anything :) I don’t think we can remove human judgement from these cases anyway
@apapirovski @cPhost If you guys are ok to review again... ^^
@nodejs/automation I'd like at least one more review before merging, just to be sure. Otherwise I'm going to merge it anyway in few hours since there seems to be no objection.
@joyeecheung No problem at all! Sorry, I'm a bit bored at work these days, so I got too much time for this :D
Merging!
Hey team!
Quick addition, title is explicit enough I think. Open to suggestions!