[devsnek]

Rejected isn't really very accurate and it has a negative connotation when in reality its more likely that the reviewer wants the PR to land, but wants to make it better (hence "requested changes")

[priyank-p]

LGTM

[codecov]

Codecov Report

Merging #109 into master will not change coverage.
The diff coverage is 100%.

@@           Coverage Diff           @@
##           master     #109   +/-   ##
=======================================
  Coverage   91.01%   91.01%           
=======================================
  Files          14       14           
  Lines         534      534           
=======================================
  Hits          486      486           
  Misses         48       48

Impacted Files	Coverage Δ
lib/pr_checker.js	96.89% <100%> (ø)	⬆️
lib/reviews.js	98.41% <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 96c517e...a41bd06. Read the comment docs.

[Tiriel]

I don't quite agree on this. I'll leave as "request changes" for now, and I'll got with the majority, I'll explain why.

Although there isn't any button "Reject PR" every time, I think there are people 👎 on PRs, not requesting changes. So in effect, they are completely rejecting the PR, and that needs to be logged.

If we could triage between actual "changes requested" and real rejection that would be cool though, but for now, I think it's better to count them as "rejected" and not just "change requested".

[devsnek]

how about we consider empty request changes as rejection

[Tiriel]

Don't know if it's this simple. We could also parse comments for :-1: but I don't know if it's a viable condition.

But that's just my two cents anyway, as said, if the majority thinks we should make the change, then go.

[joyeecheung]

CI looks green:

https://travis-ci.org/nodejs/node-core-utils/builds/301478072
https://ci.appveyor.com/project/joyeecheung/node-core-utils/build/170.$%7Bpatch%7D

[Tiriel]

Just to give specific example:
nodejs/node#15579 (review)

The workflow isn't that simple, and sometimes "changes requested" reviews are indeed rejections. Empty "changes requested" reviews or reviews containing -1 should indeed be treated as rejection I'd say, even if technically they are "changes requested" for GitHub

[refack]

@Tiriel I'm not sure that is completely correct, as the status of a fully "rejected" PR will be Closed. As long as the PR is open it is either still in negotiations, or blocked by some other issue.
The semantic of the GitHub ❌ has been debated is several issues (mainly nodejs/CTC#128) but AFAICT it wasn't used to indicate a "reject" status. I'm not even sure node core needs a "reject" status...

If at some point we develop an abandoned-issue-closing-bot, then we might consider a PR with a ❌ for closure...

[Tiriel]

@refack Well that's precisely my point, it's not used solely for requesting change, nore purely for rejection.
Of course it doesn't mean the issue has been definitely rejected, but you can't deny some people (and quite often from what I've seen) use it to say they feel the feature/change isn't a good one and should be abandonned.

So, while I think the current workflow can be perfected, I don't think we should completely strip away the "rejection" part.

Anyway, the PR is to merged since I'm alone on this, but I do think we'll lose a bit where we could've added a new feature instead of replacing one

[refack]

So, while I think the current workflow can be perfected, I don't think we should completely strip away the "rejection" part.

I think we should. Mainly for "optics".
As far as I remember the term "rejected" is never used in node's governance model, but rather the term "blocked", as in any Collaborator can block, but that can be overridden either for lacking explanation, or by the TSC.

you can't deny some people (and quite often from what I've seen) use it to say they feel the feature/change isn't a good one and should be abandonned

I don't deny it, but I do think it's the minority, and it's sort of a bug in the process. Optimally the ❌ review should be used exclusively to request changes, while full-on rejection should be done in words.

Again this is non-trivial issue, but I do feel this tool should be non-opinionated, and hance should not assume intent. A ❌ review is called "Request Changes" by GitHub, and we should follow suit:

Consensus

[Tiriel]

@refack Thanks for taking the time in any case.

It does make sense indeed, and for consistency I agree there's a case in changing the workflow. This does let me with the impression that there's a point to consider about full-on rejections and that they should be handled though, even in minority.

Just dismissed my review so the PR can be merged clean :)

[priyank-p]

Merging it now since minor changes and approved.