Conversation
|
Strong -1 to landing PRs without review. |
mcollina
left a comment
There was a problem hiding this comment.
I don’t think this is a good idea. There are significant risks for crypto and http. They will complicate life for streams.
Essentially all subsystems that are slow in reviewing changes will suffer the most.
|
Is there any way to gather some data on average review time for Pull Requests, how many landed within a week with only one approval, etc.? I had the same feeling on llnode and proposed a similar change, but then I gathered some data (which is easier there, since there are way less pull requests) and most pull requests were reviewed quickly (within 48 hours), with only a couple of outliers. |
|
Absolutely needs some hard data to back changing. At the time of posting everything in https://github.com/nodejs/node/pulls?q=is%3Apr+is%3Aopen+sort%3Aupdated-desc+review%3Anone has been updated within the last five days which suggests they are not dormant. We’ve only just introduced the |
|
I think the fact people are afraid to review unfamiliar code should not be overcome by introducing more unfamiliar un-reviewed code but by making more people familiar with the code. I think we might want to consider "hot" reviews where the reviewer and the author discuss the changes or to discuss un-reviewed-for-a-long-time PRs once a week (in TSC meetings?) |
tniessen
left a comment
There was a problem hiding this comment.
Landing code without reviews sound dangerous. I also like our new policy of requiring two reviews instead of only one, I don't see much of a reason to change that back to one.
GH is being misleading here. #27246 and #27508 for example are on that list but they've had plenty reviews, multiple rounds even in the case of #27246, it's just that no one lgtm'd them yet. There are a handful over a few days old that truly haven't been reviewed yet. That's maybe not good but it's not super terrible either given that most pull requests seem to get reviewed within a day. |
|
I'm -1 as well as I believe reviews are important. |
|
The feedback here seems to be overwhelmingly -1. I'm going to close this out. |
Summery of proposal:
I'm proposing this as a test trail.
I'm not sure if I'm alone in feeling that in the last few months there has been a lull in Collaborator interaction with the project, and it seems like more and more PRs have been left un-reviewed for long periods of time, as well as several significant PRs that had to wait the full week, and landed with only one review.
/CC @nodejs/collaborators
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes