Repository navigation
Write guidelines about closing PR/issue by triagers #527
Description
Activity
Those are my suggestions for what I think reasonable. But I'm open for other suggestions, for example if core devs prefer that triagers should not close issue/pr ever, then let's document that.
My vote is for: "triagers should not close issue/pr ever".
I consider my own experience when filing issues on other projects (mypy for example). If my suggestion is shut-down, it needs to have been seen by one of the project principals. I would feel dismissed if a recent addition to the team marked by issue as closed based on a misunderstanding.
Instead of closing, the new triagers should post recommendations to close or move forward and include their reasons. If their "batting average" of correct recommendations is high, that will become a basis for proposing them to be full core devs (along with actually fixing some of the issues). FWIW, on tough or potentially controversial calls, this is what I do as well (I post a recommendation to go forward or close, but wait for a consensus to emerge).
This policy will also help protect the new triagers. In my long experience dealing with the issue tracker almost daily, some members of the general public aren't fun to deal with. They are insistent that they are right and even when we are gentle and compassionate, they can be very aggressive and occasionally angry when their issue is closed.
BTW, one of new triagers had written to me offline and indicated a desire to close many issues rapidly and aggressively. I cautioned against it, but understood the sentiment. I've seen this on StackOverflow where aspiring moderators ambitiously mark many questions as duplicates in an effort to "clean-up" the site. Unfortunately, many of the closed questions are active and are in some essential way different from the possible duplicate.
Addenda: Perhaps we've been lucky but almost never get spam. Once in a while, we do get a post the wrong repo for PyPy, Jython, or an extension module, but this only comes up once every few months and is easily dealt with.
Hi everybody!
I think the triagers team may can close some issues/PR, but like propose Mariatta not
whole Issues/PR (e.g spam o invalid.) I think that could help us have a more "clean"
repository. IMHO have that possibility, will give to the triagers team member (that maybe
they are not core dev yet) more experience and motivation.But is true what rhettinger says, closing a PR could be necessary have lot of experience
and core dev have it.I've seen this on StackOverflow where aspiring moderators ambitiously mark many questions as duplicates in an effort to "clean-up" the site. Unfortunately, many of the closed questions are active and are in some essential way different from the possible duplicate.
And that is true.
So, I propose create new labels like "should be closed" or "incorrect issue/pr" to note that
the core dev can made a quickly review to close it or not.I would agree that triagers shouldn't close any potentially controversial PRs with current/recent activity, but for much older PRs that have been stagnant for long period of time, I don't think it's an issue.
Here's an example of one that I closed earlier today: python/cpython#117. That one was inactive for well over a year (when Brett added an
awaiting changeslabel on Feb 20, 2018), with the last response from the author being on Feb 19, 2017.Any PR which has recent activity from the author and is not completely invalid (such as attempting to merge Python 2.7 into the master branch or duplicates) should only be closed by core developers.
So, I propose create new labels like "should be closed" or "incorrect issue/pr" to note that
the core dev can made a quickly review to close it or not.There's already an
invalidandDO-NOT-MERGElabel, but it might be beneficial to add astaleorinactivelabel. Theinactivelabel could potentially cause the bots to close the PR automatically if there is no activity from in the PR from the author in over a month from when the label was added. However, this additional functionality would likely be something to be considered in the future, as it would add an additional complexity to the responsibilities of the team.I think that allowing triagers to close PRs that have been inactive for a long period of time frees up more core developer time. If the author comes back, the PR can easily be reopened.
[Kyle]
Here's an example of one that I closed earlier today: python/cpython#117. That one was inactive for well over a year (when Brett added an
awaiting changeslabel on Feb 20, 2018), with the last response from the author being on Feb 19, 2017.Arguably, that shouldn't have been closed. Age is an insufficient reason for being closed. The author had posted as recently as May of this year asking for feedback (which he never got).
IMO, what was needed was a review of the patch and a recommendation of whether it correctly solved the open problem in bpo-29553. Also, what should have been done is to ping Steven Bethard who is typically the final arbiter on all things argparse.
As a result of this closure, we may have lost a potentially correct patch and may have dissuaded someone from making future contributions.
[Kyle]
I think that allowing triagers to close PRs that have been inactive for a long
period of time frees up more core developer time.That doesn't really help us at all. Closing only takes seconds. The real value comes from reading the report, reading the PR, trying it out, thinking about its implications, engaging with the OP, and making a recommendation on what to do next.
Arguably, that shouldn't been closed. Age is an insufficient reason for being closed. The author had posted as recently as May of this year asking for feedback (which he never got).
I think you may have misread the date in the bpo issue, the author of the PR (andrewnester) last commented on the issue on 2017-05-12, which was May of two years ago. Also, another contributor attempted to create an updated version of it at python/cpython#14976, so age wasn't the only factor.
Also, I would argue that age shouldn't be a factor for issues, but I think that it can be somewhat of factor for PRs, particularly if the author has not responded to or fully addressed feedback. If another contributor wishes to pick up where the previous one left off, they can find the PR through the still open issue, and attempt to create another PR.
That doesn't really help us at all. Closing only takes seconds. The real value comes from reading the report, reading the PR, trying it out, thinking about its implications, engaging with the OP, and making a recommendation on what to do next.
That's primarily what I've been trying to prioritize, particularly for PRs that have not yet received meaningful feedback. But my idea to close inactive/stale PRs was based on what @brettcannon said in the mailing list discussion:
... I'm not prepared to say we can't have triagers help with labels and closing stale PRs that have not received a timely response over a potential worry.
Was I misunderstanding what he was saying? From my perspective, python/cpython#117 seemed like a perfect example of a stale PR.
Also, what should have been done is to ping Steven Bethard who is typically the final arbiter on all things argparse.
Although it's is not related to this particular discussion, as far as I can tell, Steven Bethard has not been active in any Python repository on either bpo or GitHub for the last 3 years at least, if not longer. I briefly searched through every nosy list he was included, up to 43 months ago with no response from him on bpo. Is there something I'm missing?
Steven Bethard has not been active in any Python repository on either bpo or GitHub for the last 3 years at least
I don't know about that. Perhaps we need a new principal maintainer for that module. We're always getting requests where someone who really knows the module needs to decide what is in scope and consistent with the design vision for the module.
Back to a more central topic for this issue: In general, staleness isn't sufficient reason to close an issue or PR. If so, we would just automatically close everything after a certain date. If a bug is valid, it doesn't stop being valid over time. Perhaps, we can decide it is something we can live with, but the age itself doesn't invalidate it. Likewise, PRs that were never reviewed or that have been inactive may still be good even after many years.
FWIW, I made a mistake closing an issue yesterday. It had been inactive a 1 1/2 years. And there were significant unanswered objections by key core devs. What I missed was the Guido approved the concept on python-dev a year ago. The OP had just been waiting for someone to put the ball over the goal line. The OP rightfullly reopened this issue right away but was somewhat irritated by the issue being closed.
I agree with @aeros167 the very old inactive issues/pr should be close, because probably that PR will be very outdated and will be necessary a refactor.
The procedure with old inactive issues/PR could be: first ping to core dev asking for a review, and then if then the core dev could be say "ok close it" or directly close it.
Let's start small and not burden triagers with trying to close PRs that look like they have not had any activity recently as that's a judgment call of what's reasonable which we have never standardized on. We could add a label for triagers to mark a PR as potentially stale so it gets flagged up to core devs to have a look.
Reacted by Kyle Stanley and Emmanuel AriasI created a PR to address this issue, based on the feedback in here and in the python-dev mailing list thread: #530.
Reacted by Emmanuel AriasFWIW, when I recommend that an issue be closed but don't immediately close it myself, it means that I'm waiting for other core-devs to assent.
FWIW, when I recommend that an issue be closed but don't immediately close it myself, it means that I'm waiting for other core-devs to assent.
Mariatta recently added a
stalelabel for GitHub to suggest that a PR should be closed on the basis of it no longer being relevant, or if the author has not responded to feedback in a long period of time. This will be primarily applied by triagers, but it might also be helpful for core devs to use it as way of suggesting that a PR should be closed while waiting for a consensus from additional core devs.The already existing
invalidlabel can be used for PRs that attempt to merge rejected or incorrect changes. I've used it once so far when an author attempted to apply a change after it was explicitly rejected by an expert of the module in the issue comments.Will this address closing/reopening PRs in order to re-trigger bad CI builds? I think it should be fine, but that having it explicitly mentioned in the guide is important given the nuances of this issue.
Will this address closing/reopening PRs in order to re-trigger bad CI builds? I think it should be fine, but that having it explicitly mentioned in the guide is important given the nuances of this issue.
Usually, my suggestion has been to apply an empty comment and remove it to rerun the CI builds, but this seems to be another viable alternative. Based on a comment from @vstinner he was suggesting that any CI failure should be thoroughly investigated, even if it seems unrelated to the PR. If it's determined to be unrelated and hasn't already been reported, an issue should be opened for it.
It would be a good idea though to outline the best practices for investigating, reporting, and rerunning CI tests. That would probably be substantial enough to warrant having its own issue and dedicated devguide section though.
I agree about "never close PRs" but I'm gonna have to push back on the "never close issues". I've triaged many-an-invalid bug on bpo. These are cases where it's just a beginner mistake and they just need quick little pointer in the right direction. I don't think having a core dev look at it would add any value. For example:
- https://bugs.python.org/issue34721
- https://bugs.python.org/issue27697
- https://bugs.python.org/issue27947
- https://bugs.python.org/issue34772
I completely agree when it comes to a proposal/PR though, in that case closing sends a very negative signal and can definitely dissuade people from contributing and using the bug tracker.
Reacted by Zachary Ware- On 8/29/2019 11:51 PM, Ammar Askar wrote: I agree about "never close PRs" but I'm gonna have to push back on the "never close issues". I've triaged many-an-invalid bug on bpo. These are cases where it's just a beginner mistake and they just need quick little pointer in the right direction. I don't think having a core dev look at it would add any value. For example: * https://bugs.python.org/issue34721 * https://bugs.python.org/issue27697 * https://bugs.python.org/issue27947 * https://bugs.python.org/issue34772 I completely agree when it comes to a proposal/PR though, in that case closing sends a very negative signal and can definitely dissuade people from contributing and using the bug tracker.Those are nicely done, as well as I would have. Would you be willing to help newer triagers? Both as to the close decision and just as importantly, the close message. Terry
Would you be willing to help newer triagers? Both as to the close decision and just as
importantly, the close message.Sure thing. To any of the new triagers: please feel free to reach out to me by email, irc, zulip or add me to the nosy if you want me to look over or give feedback on an issue that seems like it should be closed.
Reacted by Brandt BucherI agree about "never close PRs" but I'm gonna have to push back on the
"never close issues". I've triaged many-an-invalid bug on bpoThe new rules for the triager role primarily are targeted at GitHub and specifically with PRs, since pretty much all of the non-dev issues come from bpo still (which we're gradually moving away from). I'll update update the section accordingly though so it doesn't come across as suggesting that bpo issues shouldn't be closed, thanks for the feedback!
I updated the PR to the devguide to only restrict triagers from closing PRs. Since there wasn't explicit consensus on whether or not triagers should be able to close issues, I'll leave that out of the devguide for now.
Hi everybody! I think this issue could be close, isn't?
Not sure if I should... 😉
Reacted by Ammar Askar@eamanu @brandtbucher I think this issue can be safely closed at this point since #530 was merged. I'll close it. If there's a need to, it can always be reopened.
Closing an issue/PR means making decision and rejecting the proposed idea.
Perhaps this ability needs to be leveraged more carefully. It would be great if we have some guidelines of when a PR/issue can be closed. Example:
Whenever in doubt, always ask first.