In the past we’ve discussed about closing stale PRs (prs with no activity, no comment or anything after a certain period), however we have never reached a decision on how many days of no activity that the PR can be considered stale.
There is now a GitHub Action for automatically markings PRs and issues as stale, and it can also automatically close the stale issue/PR.
It works as follows: if a the PR has no activity after certain (configurable) period, it will apply the “stale” label, and leave a comment to give a warning that this PR is now stale. If someone them leaves a comment in the PR (they can leave a comment saying “I’m waiting for review” or “still working on it” or anything) , then the stale label gets removed.
However if there continued to be no activity, no further comments after certain number of days, then the bot will close the PR.
Obviously I’m aware of how much time consuming it is do this job and it’s not a critique, but I’m afraid that if they are closed automatically, a lot of work would be lost.
Especially for new contributors that are still learning the process of contributing to Python, if they miss the message their PR will be closed and they need to track them carefully or their work will be forgotten which raises the bar for them.
I think the number of days before sending the message should be greater than the maximum time it can take for a core developer to get to a PR, maybe 6 months?
Yes it makes sense to me to have the “days before closing” to be really long like in months instead of days.
I think having the stale label will also be useful so coredevs and triagers can hopefully check the PRs that are stale periodically and prioritize reviewing such PRs.
Once marked as stale, does it go to the top of the PRs list because it has activity? Will it also go to the top of the list because the PR author will say “waiting for review”. That’s going to be more than 3 PRs brought to the top every day.
Is there any benefit to closing PRs? I can’t look at the code right now, so I’m not sure if it can be passed a null value for the days-until-close parameter
I’m not sure the distribution of age on our PRs. 60 days from stale to close seems reasonable if no activity. As for how long before stale (6 months?).
Just for some quick data, I ran some analysis on our pull requests. If anyone has any requests for any other graphs they’d be interested in or the raw data, feel free to reply here.
My two cents: we should lean a bit on the conservative side for the “time to mark as stale” and “time to close stale PRs”. Like @remilapeyre said, this can really dissuade contributors especially given how long reviews can take.
Based on the non core-dev data, given that 95% of pull requests are merged within 300 days, I would recommend that as the cut-off point for marking PRs as stale.
For time to close, setting it to 40-50 days seems reasonable since it looks like that amount of time is what it takes for authors to take actions on pull requests.
I do think it’s useful to close out really old PRs that haven’t been actioned on. It’s equally as annoying for reviewers when they go to review a patch that no longer merges cleanly. New contributors taking on a bug also have to wait for the stale PR author to respond back.
Here’s the quick and dirty code, it’s easy to generate the PR age distribution graph since you can retrieve 100 pull requests per request from Github (~215 requests in total currently). It’s harder to do ones based on labels since that needs a request per pull request to pull in all the data.
Code
from github import Github
import json
g = Github("GITHUB_TOKEN", per_page=100)
with open('pr_list.json', 'w', encoding='utf-8') as f:
repo = g.get_repo("python/cpython")
for i, pull in enumerate(repo.get_pulls('all')):
print("Retrieved {} pull requests".format(i))
f.write(json.dumps(pull._rawData))
f.write("\n")
Jupyter Notebook
pull_requests = {}
with open('pr_list.json', 'r', encoding='utf-8') as f:
for line in f:
raw_object = json.loads(line)
id = int(raw_object["url"].split("/")[-1])
pull_requests[id] = raw_object
merge_days = []
for pr in prs.values():
if pr['user'] in CORE_DEVS or pr['user'] in BOTS:
continue
time_to_merge = pendulum.parse(pr['merged_at']) - pendulum.parse(pr['created_at'])
merge_days.append(time_to_merge.in_days())
ax = sns.kdeplot(all_merge_times, cumulative=True, shade=True)
ax.set_xlim(left=0)
ax.set(xlabel='Days after PR Creation', ylabel='Pull requests merged % (cumulative)')
I think the more complex analysis can be done through the backups we take of the Github repo if we want to avoid hitting the github api for all the info.
I will repeat here that I am opposed to mindless (mechanical) closing of issues and PRs in general and especially those for IDLE. Earlier this year, I closed about 40 IDLE issues (as other than ‘fixed’) and any linked PRs. But I left some open. I am more or less working now on a few really old issues (as in 10 years).
There was a similar discussion in the matplotlib gitter, with both arguments that:
A large number of PRs can be discouraging for (new) contributors.
vs
Closing an inactive PR can also drive contributor away.
Both of which are good point, and I’m not going to delve into whether the PR is closed with/without manual/automatic warning or not. nor does this address whether the turnout time if fast or not and so whether the Number of PR is a good metric, but arguably that’s one of the most prominent (and default) things visible in a repo and finding out whether the turnaround time is short requires user active interaction with the repo which most people won’t do.
I personally don’t think there is a one answer fit all repository, still this have lead me to run this highly non-scientific poll:
Dear OSS contributors. Which feels the worst A) You started a PR on a project, have not finished/touched it in quite some time. PR get closed with a message “This looks inactive, feel free to reopen”. B) You want to contribute to project X, there are already 200+ opened PRs.
In general both groups have about 2.5 to 3 time more respondent feeling that coming across a repo with more than 200 PR is worse than having a PR closed for inactivity.
I’ll leave the analysis to the reader. Some of the tweets reply also have good points.
I will also point out that closing a PR for inactivity can be discouraging for O(1) user per closed PR, (basically anyone interacting with this PR in particular or coming across it, and realising it was closed)
while having a discouragingly high number of PRs will effect about anyone coming across the repo even before contributing. So my guess is that there are many people (at least N=1, me) that won’t even send a PR if this number is too high.
I also don’t talk about maintainer feeling of having too many PRs, personally I feel stressed, and have difficulty finding what to focus on when the number of PRs on repo I maintain is high.
Thanks for reading my rambling, and everyone for you consideration on this issue.
I have just closed a 3 year old PR about this issue. I think has stalled for too long
It sounds like folks here don’t mind the “stale” label but some folks don’t want the PR to be automatically closed. I have adjusted my PR so that the “stale” label will get added, but it doesn’t ever close the PR, by setting the days-before-closing value to -1.
