Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 1 commit into from
Closed

Conversation

Trott
Copy link
Member

@Trott Trott commented Sep 25, 2018

Trying to see if we can get consensus on a compromise on the rule simplification to 60 hours.

Prior art:

48 hour PR: #23082
72 hour PR: #22275

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 60 hours.

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

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 60 hours.
@nodejs-github-bot nodejs-github-bot added the doc Issues and PRs related to the documentations. label Sep 25, 2018
@Trott
Copy link
Member Author

Trott commented Sep 25, 2018

As @addaleax pointed out elsewhere, one advantage of 60 hours is that it is the threshold at which point the GitHub interface indicates that a PR was opened "3 days ago" instead of "2 days ago".

Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be worth mentioning in the docs that this is where the GitHub UI draws the 2 days / 3 days line (if I’m right about that, but I’m fairly certain)?

(edit: jinx. 😄)

Copy link
Contributor

@mscdex mscdex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-1 60 hours is harder to reason about because it's not at a day boundary. I also think it's not a good idea to align this with Github's relative date expressions because those could change at any time, making things worse or at least more confusing.

@Trott
Copy link
Member Author

Trott commented Sep 26, 2018

Talking with folks, there seems to be little appetite for this. People tend to fall into the 72- or 48-hour camp, and not 60-hour. To simplify the decision-making process, I'm going to close this one. Thanks/sorry.

@Trott Trott closed this Sep 26, 2018
@Fishrock123
Copy link
Contributor

My preference is 72 hours - anything that really needs to be landed in less than that is usually going to be sub 48 as well, and will be fast-tracked. As in, release blockers, mostly.

Maybe I'm a bit out of touch but that's the feel I get after watching things in Node for a long time.

@Trott Trott deleted the 60 branch January 13, 2022 22:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants