doc: simplify pull request wait time to 48 hours #23082

Trott · 2018-09-25T13:38:18Z

Not a change I'd want to sneak in unnoticed, so: @nodejs/collaborators

Currently, we have a 48/72 rule for how many hours a pull request should
be left open at a minimum. Unfortunately, whether a pull request should
be left open for 48 or 72 hours is often unclear. The 72 hours is
required if it is a weekend. If I open a pull request on a Friday
morning, does it need to stay open 48 hours or 72 or something in
between? Does it matter if I'm in one time zone or another?

Pull requests used to require a single approval but now require two
approvals unless they've been open for a week. Given this, it seems like
we can simplify the wait time rule to be just 48 hours.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
documentation is changed or added
commit message follows commit guidelines

nodejs-github-bot · 2018-09-25T13:38:19Z

@Trott build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/1009/pipeline

refack

sorry for the 🔧 in the 🎡 .

refack · 2018-09-25T15:00:49Z

I strongly believe that if simplification is sought, then the timespan should be 72h.
Requiring that volunteers be constantly responsive in 48h windows IMHO is tub-optimal. (And I write "require" since I strongly believe is mutual accountability, i.e. any change is our mutual responsibility, so I wish to at least be able to be made aware of it ahead of time).

Pull requests used to require a single approval but now require two
approvals unless they've been open for a week. Given this, it seems like
we can simplify the wait time rule to be just 48 hours.

It is my opinion that approvals are just part of the picture. Review and notification is the other.

richardlau · 2018-09-25T15:12:58Z

It is my opinion that approvals are just part of the picture. Review and notification is the other.

Well that raises another point -- I've recently seen a PR that pinged a group and then landed (with no responses either way from the group pinged) an hour later (the PR itself had been opened for long enough). If a group is pinged on a PR presumably we want to give sufficient time for that group to notice the ping and respond.

mscdex · 2018-09-25T15:22:01Z

I'd prefer to opt for the longer of the two.

refack · 2018-09-25T15:26:02Z

An idea for possible simplification without changes in policy. Have the bot flag issues when enough time passed.

vsemozhetbyt · 2018-09-25T15:29:15Z

@richardlau FWIW, the group may be pinged if there are no sufficient approvals for the relevant PR for some time (we need 2 now). PRs may be considered ready by a lander once a second reviewer appears. Otherwise, we may need to add a rule about sufficient time after a ping.

BridgeAR · 2018-09-25T15:35:43Z

@refack

Requiring that volunteers be constantly responsive in 48h windows IMHO is tub-optimal.

I am somewhat surprised that this is the conclusion when thinking about this time frame. I personally definitely do not review each PR and I would not want to either even though I definitely look at a lot of them. I rely on all fellow collaborators that everyone does a good job reviewing code. If a PR got two LGs I would not expect mine to be required at all. So why does everyone have to be responsive in 48 hours? Another point is that even if something lands and it turns out that it was a bad idea or what so ever we can still revert it.

mscdex · 2018-09-25T15:36:48Z

To echo what was said in the older, 72 hour alternative PR, I think standardizing on 72 hours is better because it works especially well for the weekend scenario, since 72 hours covers an entire weekend better when considering different people from different timezones.

mcollina

LGTM

refack · 2018-09-25T16:49:27Z

I am somewhat surprised that this is the conclusion when thinking about this time frame. I personally definitely do not review each PR and I would not want to either even though I definitely look at a lot of them.

I'm not referring to reviewing every PR, but I would like the possibility to review every PR that interests me, or that may impact my contribution in the future.

I rely on all fellow collaborators that everyone does a good job reviewing code. If a PR got two LGs I would not expect mine to be required at all. So why does everyone have to be responsive in 48 hours?

I want to decouple Approvals from Review. IMHO review is a way for me to minimize future surprise, and also as a way for the community to acknowledge that my opinion matters.

Another point is that even if something lands and it turns out that it was a bad idea or what so ever we can still revert it.

Revert is IMHO a more aggressive act then discussion ahead of time. IMHO revert should happen only if a change hurts the project. Review and discussion are opportunities to make things optimal in the first hand.

tl;dr what is the rush? When ⚖️ code quality vs velocity, I opt for quality every time.

mscdex

Making it explicit for the reason previously stated.

Trott · 2018-09-25T16:56:03Z

The 72-hour alternative is at #22275.

refack · 2018-09-25T17:02:31Z

The 72-hour alternative is at #22275.

I think you hit an important point that deserves further unpacking (maybe at CollabSummit?)

#22275 - Simple 72h - 1 review requesting changes and 10 approving reviews
#23082 - Simple 48h - 2 reviews requesting changes and 7 approving reviews

joaocgreis

If there's a need to simplify the current rule, I'd rather go with #22275

Trott · 2018-09-25T22:22:27Z

Since there seems to be consensus that simplification is desirable, but no consensus on whether it should be simplified to 48 or 72 hours, here's a compromise of 60 hours: #23093

fhinkel · 2018-09-26T14:36:56Z

LGTM, we can always open a PR amending or reverting the change if needed.

BridgeAR · 2018-09-27T08:52:36Z

One thing to also keep in mind: most PRs that are not absolutely trivial do not land after 48 hours. The PR has to receive enough LGs, all comments have to be addressed and the CI has to be green. All of these factors make it pretty difficult to land anything in that time frame that is not a simple change.

Trott · 2018-10-06T22:28:58Z

Landed in 124a8e2

vsemozhetbyt · 2018-10-07T00:16:52Z

@Trott There are 2 more mentions to fix:

https://github.com/nodejs/node/blob/master/doc/guides/contributing/pull-requests.md#respect-the-minimum-wait-time-for-comments

For non-trivial changes, Pull Requests must be left open for at least 48 hours during the week, and 72 hours on a weekend.

https://github.com/nodejs/node/blob/master/doc/guides/contributing/pull-requests.md#waiting-until-the-pull-request-gets-landed

A Pull Request needs to stay open for at least 48 hours (72 hours on a weekend) from when it is submitted, even after it gets approved and passes the CI.

Trott · 2018-10-07T04:37:02Z

@vsemozhetbyt Thanks for catching that! I've now opened #23309 to update pull-requests.md. If you've already opened a PR or were planning on it, I'm happy to close mine.

Currently, we have a 72 rule for how many hours a pull request should be left open at a minimum. Reduce that time to 48 hours. PR-URL: #23082 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Denys Otrishko <shishugi@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>

Refs: nodejs/node#23082

Currently, we have a 72 rule for how many hours a pull request should be left open at a minimum. Reduce that time to 48 hours. PR-URL: nodejs#23082 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Denys Otrishko <shishugi@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>

Currently, we have a 72 rule for how many hours a pull request should be left open at a minimum. Reduce that time to 48 hours. PR-URL: #23082 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Denys Otrishko <shishugi@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>

refack · 2018-11-19T22:52:36Z

I want to say I was wrong. It seems to me that simplifying the wait time has made the review process, at least for me, indeed simpler.
As for "keeping on top of thing" - I don't feel like things are moving too fast, or that I've been loosing track of areas that interest me. 🎉
_{speaking only for myself}

Refs: nodejs/node#23082

nodejs-github-bot added the doc Issues and PRs related to the documentations. label Sep 25, 2018

Trott mentioned this pull request Sep 25, 2018

doc: leave pull requests open for 72 hours #22275

Closed

3 tasks

targos approved these changes Sep 25, 2018

View reviewed changes

cjihrig approved these changes Sep 25, 2018

View reviewed changes

lundibundi approved these changes Sep 25, 2018

View reviewed changes

BridgeAR approved these changes Sep 25, 2018

View reviewed changes

jasnell approved these changes Sep 25, 2018

View reviewed changes

lpinca approved these changes Sep 25, 2018

View reviewed changes

refack suggested changes Sep 25, 2018

View reviewed changes

refack added the meta Issues and PRs related to the general management of the project. label Sep 25, 2018

mcollina approved these changes Sep 25, 2018

View reviewed changes

mscdex suggested changes Sep 25, 2018

View reviewed changes

joaocgreis suggested changes Sep 25, 2018

View reviewed changes

Trott mentioned this pull request Sep 25, 2018

doc: simplify pull request wait time to 60 hours #23093

Closed

3 tasks

fhinkel approved these changes Sep 26, 2018

View reviewed changes

apapirovski approved these changes Sep 27, 2018

View reviewed changes

Trott closed this Oct 6, 2018

trivikr mentioned this pull request Oct 7, 2018

git-node: simplify PR wait time to 48 hours nodejs/node-core-utils#288

Merged

targos mentioned this pull request Oct 7, 2018

Release proposal: v10.12.0 #23313

Merged

priyank-p pushed a commit to nodejs/node-core-utils that referenced this pull request Oct 7, 2018

git-node: simplify PR wait time to 48 hours

57c7f04

Refs: nodejs/node#23082

Trott deleted the nick-nolte-and-eddie-murphy-in branch January 13, 2022 22:50

johnfrench3 pushed a commit to johnfrench3/core-utils-node that referenced this pull request Nov 2, 2022

git-node: simplify PR wait time to 48 hours

ae34926

Refs: nodejs/node#23082

renawolford6 added a commit to renawolford6/node-dev-build-core-utils that referenced this pull request Nov 10, 2022

git-node: simplify PR wait time to 48 hours

cb4a410

Refs: nodejs/node#23082

Developerarif2 pushed a commit to Developerarif2/node-core-utils that referenced this pull request Jan 27, 2023

git-node: simplify PR wait time to 48 hours

cfec04f

Refs: nodejs/node#23082

gerkai added a commit to gerkai/node-core-utils-project-build that referenced this pull request Jan 27, 2023

git-node: simplify PR wait time to 48 hours

3cc44e3

Refs: nodejs/node#23082

patrickm68 added a commit to patrickm68/NodeJS-core-utils that referenced this pull request Sep 14, 2023

git-node: simplify PR wait time to 48 hours

364a3d9

Refs: nodejs/node#23082

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

doc: simplify pull request wait time to 48 hours #23082

doc: simplify pull request wait time to 48 hours #23082

Trott commented Sep 25, 2018

nodejs-github-bot commented Sep 25, 2018

refack left a comment

refack commented Sep 25, 2018 •

edited

Loading

richardlau commented Sep 25, 2018

mscdex commented Sep 25, 2018

refack commented Sep 25, 2018

vsemozhetbyt commented Sep 25, 2018 •

edited

Loading

BridgeAR commented Sep 25, 2018

mscdex commented Sep 25, 2018

mcollina left a comment

refack commented Sep 25, 2018

mscdex left a comment •

edited

Loading

Trott commented Sep 25, 2018

refack commented Sep 25, 2018

joaocgreis left a comment

Trott commented Sep 25, 2018

fhinkel commented Sep 26, 2018

BridgeAR commented Sep 27, 2018

Trott commented Oct 6, 2018

vsemozhetbyt commented Oct 7, 2018

Trott commented Oct 7, 2018

refack commented Nov 19, 2018

doc: simplify pull request wait time to 48 hours #23082

doc: simplify pull request wait time to 48 hours #23082

Conversation

Trott commented Sep 25, 2018

Checklist

nodejs-github-bot commented Sep 25, 2018

refack left a comment

Choose a reason for hiding this comment

refack commented Sep 25, 2018 • edited Loading

richardlau commented Sep 25, 2018

mscdex commented Sep 25, 2018

refack commented Sep 25, 2018

vsemozhetbyt commented Sep 25, 2018 • edited Loading

BridgeAR commented Sep 25, 2018

mscdex commented Sep 25, 2018

mcollina left a comment

Choose a reason for hiding this comment

refack commented Sep 25, 2018

mscdex left a comment • edited Loading

Choose a reason for hiding this comment

Trott commented Sep 25, 2018

refack commented Sep 25, 2018

joaocgreis left a comment

Choose a reason for hiding this comment

Trott commented Sep 25, 2018

fhinkel commented Sep 26, 2018

BridgeAR commented Sep 27, 2018

Trott commented Oct 6, 2018

vsemozhetbyt commented Oct 7, 2018

Trott commented Oct 7, 2018

refack commented Nov 19, 2018

refack commented Sep 25, 2018 •

edited

Loading

vsemozhetbyt commented Sep 25, 2018 •

edited

Loading

mscdex left a comment •

edited

Loading