search

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

change rejected to requested changes #109

Closed devsnek closed 6 years ago

devsnek commented 6 years ago

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")

codecov[bot] commented 6 years ago

Codecov Report

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

Impacted file tree graph

@@           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%> (ø) :arrow_up:
lib/reviews.js 98.41% <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 96c517e...a41bd06. Read the comment docs.

devsnek commented 6 years ago

how about we consider empty request changes as rejection

Tiriel commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

Just to give specific example: https://github.com/nodejs/node/pull/15579#pullrequestreview-66057696

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 commented 6 years ago

@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 https://github.com/nodejs/CTC/issues/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 commented 6 years ago

@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 commented 6 years ago

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: image

Tiriel commented 6 years ago

@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 commented 6 years ago

Merging it now since minor changes and approved.