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

Policy for stale PRs #11583

Closed
JelleZijlstra opened this issue Mar 13, 2024 · 4 comments · Fixed by #11596
Closed

Policy for stale PRs #11583

JelleZijlstra opened this issue Mar 13, 2024 · 4 comments · Fixed by #11596
Assignees
Labels
project: policy Organization of the typeshed project

Comments

@JelleZijlstra
Copy link
Member

We currently have 71 open PRs. That's not terrible (mypy and CPython have a lot more), but it's more than I'd like. Many of the PRs are not in a mergeable state, so it's hard to get the number down.

I'd like to propose a few policies to help keep the number under control:

  • "Deferred" PRs (https://github.com/python/typeshed/pulls?q=is%3Aopen+is%3Apr+label%3Adeferred) must have a clearly stated blocker. That can be either an unambiguous bug in a type checker (the type checker is not following the spec) or a dependency on an active PEP.
  • It is the responsibility of the PR author to ensure that all of our tests pass. If a PR has been open for three months and CI is still failing, we can close it. This includes draft PRs.

If other maintainers agree, I can add these guidelines to our CONTRIBUTING documentation.

@srittau srittau added the project: policy Organization of the typeshed project label Mar 13, 2024
@srittau
Copy link
Collaborator

srittau commented Mar 13, 2024

A few random thoughts:

  • I'd add unaddressed PR feedback to the reasons to close a PR.
  • For deferred, I'd formulate it a bit more open ended. While a PEP and a type checker bug are good reasons for a deferred label and a vague idea about a potential feature is not, I can imagine other reasons for the label. For example, an ongoing discussion related to it. But I agree that the condition for merging or rejecting the PR should be clearly stated.
  • Make it clear that the maintainer adding the "deferred" label is responsible for stating the condition.
  • It would also be great if we had a standard text for closing old PRs. Something that is well written and polite and mentions the option to reopen a PR if the author finds time to work on it.

@JelleZijlstra JelleZijlstra self-assigned this Mar 13, 2024
@Avasam
Copy link
Collaborator

Avasam commented Mar 13, 2024

I'd like to at least make an attempt to contact the PR author when we consider a PR stale (a message tagging their handle is enough). I have often forgotten about work I started but didn't bring to completion, or didn't see it was blocked by a merge conflict. Less-so in typeshed nowadays, but putting myself in the shoes of an external contributor, I've done it on other projects.

If we ever go the automation route, I have my concerns about autoclosing, as it can cause a lot of friction (especially if that system is ever brought over to issues). I have no problem automatically marking them as stale though.

I'd also want to consider whether it's still being actively worked on, although I have no suggestion for a hard rule on this. Case in point, the currently oldest still open PR is one of mine I've been chipping away at for over a year: #9511
Closing then immediately re-opening a PR works, but is a bit weird.

@srittau
Copy link
Collaborator

srittau commented Mar 13, 2024

If we ever go the automation route, I have my concerns about autoclosing, as it can cause a lot of friction (especially if that system is ever brought over to issues). I have no problem automatically marking them as stale though.

Yeah, autoclosing is a big no from me, but an automated reminder after two months of build failures or unaddressed reviews could be a useful reminder.

@AlexWaygood
Copy link
Member

AlexWaygood commented Mar 13, 2024

Agreed that we definitely shouldn't automate closing stale PRs.

I'd like to at least make an attempt to contact the PR author when we consider a PR stale (a message tagging their handle is enough)

I worry that adding too many rules like this makes triaging harder, and makes it harder for us to cope with our PR backlog. Keeping typeshed maintainable is important; if it becomes unmaintainable, that's also a bad outcome for contributors.

If a PR has failing CI and/or nontrivial merge conflicts, and hasn't seen any activity in >=3 months, I think it's fine to close the PR as long as we are extremely polite in the message we give, thank them for their time, and make it abundantly clear that we're happy to reopen the PR at a moment's notice if they're still interested in working on it

JelleZijlstra added a commit that referenced this issue Mar 14, 2024
JelleZijlstra added a commit that referenced this issue Mar 15, 2024
Closes #11583

Co-authored-by: Alex Waygood <[email protected]>
Co-authored-by: Akuli <[email protected]>
Co-authored-by: Avasam <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
project: policy Organization of the typeshed project
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants