Repository navigation
New label: "pending closure" #13859
Description
Type: other
@brainwane @gloomy-ghost @Hainish @J0WI @wonderchook and anyone else: I've created a new label called pending closure.
Sometimes a pull request is created and for whatever reason, it has no activity from the PR contributor for a long time. Maybe there was some discussion earlier, but not now. Maybe the contributor has lost interest in it, or lost interest in HTTPS Everywhere, or they don't use GitHub regularly. In any case, they have essentially abandoned the PR. If a maintainer reviews the PR and has feedback, the contributor won't be around to update the PR. So it's unclear to the maintainer whether it's worth spending time reviewing the PR, and the end result is the PR just sits around.
A way around this problem is for the maintainer to "ping" the contributor in a comment in the PR, by directly saying "ping" or by asking the contributor directly if they are still interested in working on the PR. If the contributor doesn't reply in some reasonable amount of time, we can assume the PR is abandoned and close it and, optionally, recreate it or note the domains in issue #3069 or whatever else.
My intent with the pending closure label is to apply it to PRs that have a ping, and remove it if either the contributor says they are still interested in working on the PR or if the PR is closed. This will make it easy to search for these pinged PRs and close them if it's been a while since the ping, meaning the PR has probably been abandoned.
This is all at the discretion of the maintainers and I don't want to make this a formal, or worse, automated, process. The ping and label probably shouldn't apply until a PR has been around without contributor activity for at least a month, and the PR probably shouldn't be closed until the ping has been unanswered for at least a week. These are rough estimates. Contributors who are still active with HTTPS Everywhere should probably be given more leeway than contributors who aren't, and similarly a contributor with any recent GitHub activity -- meaning, contributions shown in their profile -- should probably be given more leeway than a contributor without any recent activity.
In some cases a contributor has many open, possibly abandoned PRs and it may be convenient for a maintainer to ping them in one PR but have that ping apply to other open PRs too (and make that clear to the contributor in the ping). In that case the label should only be applied to the PR with the ping, but the other PRs can still be closed if the ping is unanswered.
Sometimes a contributor deletes their GitHub account and the PR is assigned to @ghost. In that case the PR has definitely been abandoned and can get the label right away without a ping. The PR can be kept open for a little while in case another maintainer wants to look at it for some reason, but eventually the PR can be closed.
I'm going to start using this label right away, but other than the label itself, this is not really a new policy, for me at least. We've been pinging-and-deleting for a while now.
Please feel free to share your thoughts, criticisms etc.
Activity
wonderchook commentedon Dec 4, 2017
Seem reasonable to me. Should there be some sort of process if what is submitted is high priority to make sure it doesn't get closed. Thinking something like a site in the top 1k or 100k?
jeremyn commentedon Dec 4, 2017
If a PR has been abandoned and changes are needed, then the PR should be closed even if it's
top-100, because the contributor isn't there to implement the changes. The maintainer or someone else can make a new PR, or make an issue for the domains or note them in #3069, if appropriate.[-]New label: "pending closure"[/-][+]Discussion: new label: "pending closure"[/+]https-everywhere-bot commentedon Dec 5, 2017
Thanks! Your edit helped me out. I'll take it from here now.
[-]Discussion: new label: "pending closure"[/-][+]New label: "pending closure"[/+]jeremyn commentedon Dec 5, 2017
Unrelated but I'd like to have a
discussionlabel instead of, or in addition to, the existingquestionlabel. In fact I initially renamedquestiontodiscussion, but then changed it back in case anyone feels strongly about it, and so we don't lose information about which issues originally hadquestion.So how do we feel about renaming
questiontodiscussion, or about adding a newdiscussionlabel?J0WI commentedon Dec 8, 2017
Renaming may result in some confusion and should be avoided. But feel free to create a new one. :)
I'm still a bit worried making discussions and decisions in GitHub issues. But to discuss this I have to open an issue... ;)
jeremyn commentedon Dec 9, 2017
I'd rather just use
questionfor everything than have bothquestionanddiscussionlabels, especially since some pastquestions are reallydiscussions, so anyone looking at past examples to decide which label to use for a new issue might be confused.J0WI commentedon Dec 22, 2017
From the recently referenced ruleset:
I would not close PRs that didn't receive any feedback. Just asking if they are still interested and then closing without any cause is not nice and we may loose patches that were ready for merge.
If the patch is still fine it shouldn't matter whether the author is responding or not.
jeremyn commentedon Dec 22, 2017
PR #11153 for
nofap.comwas missing domains such asemergencyandtracker.I agree it's appropriate to take a quick look before closing a PR to make sure that it's not ready to go as it is. However if there is anything wrong then we should be free to close it without describing the problems.
jeremyn commentedon Dec 22, 2017
In my previous comment, by "missing domains" I mean that these domains show up in Google. I don't know if they work with HTTPS or not, but either way they should be mentioned in the ruleset.
Bisaloo commentedon Dec 29, 2017
@jeremyn, could you keep the
pending closurelabel even after you closed the PR? This way, contributors can go throughis:pr label:"pending closure" is:closedand see if they are interested in taking over some of them.jeremyn commentedon Dec 29, 2017
@Bisaloo
pending closureisn't appropriate for a PR that has been closed, since it is no longer "pending" being closed.Anyway in most of these
pending closurecases I've done something with the PR besides just closing it, for example I've re-created the PR myself, or added the domain(s) to issue #3069. I prefer #3069 to be the primary source that interested contributors use if they are looking for something to do, though there is some tension about this (see the discussion in issue #13890).This sort of follow-up activity from a maintainer isn't guaranteed though, so if you're very interested in these specific PRs for some reason, you can regularly search for and subscribe to them while they are still open.
Hainish commentedon Jan 30, 2018
Generally, a
pending closurelabel makes sense to me.One complication. After a cursory view, a PR abandoned by its contributor may seem useful and warrant a longer lifespan if it can't be reviewed immediately. If a
pending closurelabel is applied, another maintainer may feel free to close it at some point. This will not be the desired outcome.Additionally, a maintainer may search for the
pending closurelabel and have many PRs that are not being shepherded by themselves show up in the results.This is why I'd like to offer a friendly amendment to this proposal: that any PR which has been applied the
peding closurelabel also be given an assignee. It will be assigned to the maintainer who has sent the ping. This will make it clear who is responsible for shepherding it along, and facilitate better searches (e.g.label:pending-closure assignee:Hainish).jeremyn commentedon Jan 31, 2018
As I said in my previous comment #13859 (comment), in most cases I have not just closed
pending closurePRs but have also done something with them.There's no standard for what assigned PRs mean. @J0WI never assigns PRs to themselves. I do, and it means I'm signaling that I have some responsibility for responding to comments, probably because I've given a contributor changes to make and want to signal an obligation to review their changes.
I keep an eye on my assigned PRs and try to keep their number down. The worst situation with assigned PRs is with contributors who never respond. The PR sits in my queue forever until, usually, I end up recreating the PR myself so I can unassign myself from the original. The point of
pending closureis to identify PRs that have been abandoned, so they are actually ones I would particularly prefer not to assign to myself.The real value in a PR is knowing that some subdomain should be covered, and having a second person to review the changes. The actual content of the PR is secondary and has low value, since it is easy to create given the domain. If the contributor has abandoned the PR then the value left is knowing the domain, which can be added to a list. There is no need to carefully shepherd the actual XML file.
Hainish commentedon Jan 31, 2018
In that case, what is your preferred method for finding
pending-closureissues that have been pinged specifically by you? A functional equivalent to using theassigneewill be to issue feedback (or ping, if feedback had not previously been made) via the formal review process, and thus use something likeis:open is:pr reviewed-by:Hainish label:pending-closure. This will keep your assigned PRs down in fact, and will allow you to do an easy filtering of which are pending closure by you.Hainish commentedon Jan 31, 2018
This is what most of us do anyway, but seems a good criteria to use when searching.
J0WI commentedon Jan 31, 2018
GitHub is a bit weird with assigning bugs/PRs. Only Collaborators (even from different EFF projects) can be selected. Other bug trackers allow to assignee any user, who is actually working on it.
I prefer the review field, which is also automatically updated when you add review comments.
jeremyn commentedon Jan 31, 2018
@Hainish
pending closureis not a status that should be associated with a specific maintainer. It just means the PR has met some criteria for age and inactivity and we want to know if the contributor is still interested in it. A bot could assign the label.In practice, when a
pending closurecontributor has expressed interest in getting their PR merged, I generally work with them to do that, so I have been "soft assigned" to these PRs. This is not ideal for me, though.@J0WI The PR system is intended to handle patches, not bug reports. The person who is working on the technical problem should be the person who created the PR. I agree (?) that there is some confusion between the review and the assigned fields. Also this project is unusual because most PRs have low complexity and often come from inexperienced contributors, so the line between "patch" and "bug report/support issue" is sometimes blurred.
Hainish commentedon Jan 31, 2018
@jeremyn Certainly, if you are in communication and involved in the review process with an absentee contributor, you should be the one to follow up on it if they either (a) reappear or (b) don't return. In the latter case, the responsibility should fall on you to close the issue. If I was in the feedback process with a contributor, it would be inappropriate for someone else to come and close that issue from under me. Therefore, clear demarcation of responsibility (via the
reviewfield, for instance) makes perfect sense to me.In searches, I would want only those PRs which I was in the review process for showing up. I wouldn't want your results - that would be confusing and inefficient to wade through. I would want to show up only those PRs that I have taken the responsibility of helping get merged. I can see this applying to both ruleset PRs and core code PRs.
jeremyn commentedon Feb 1, 2018
@Hainish As I said,
pending closurejust means the PR has met some age and inactivity criteria, and we want to see if the contributor is still interested in working on it. It is not part of the feedback process, and it should not imply ownership, similar to how you are not taking ownership of a PR just because you have applied an Alexa label to it.Hainish commentedon Feb 1, 2018
@jeremyn Clearly it is part of a feedback process if you're pinging a contributor to solicit feedback.
jeremyn commentedon Feb 1, 2018
@Hainish I don't think asking a contributor if they would respond to feedback is itself feedback that obliges the maintainer to see that PR through to its resolution.
Also this whole idea of requiring a maintainer to do something is pretty new. So far being a maintainer has been a very low obligation role, other than obligations we impose on ourselves.
jeremyn commentedon Feb 8, 2018
I'm closing this issue with these comments:
Since I created the
pending closurelabel two months ago, I think I'm the only person who used it (or was by far its most frequent user), either to apply the label initially or to close a PR after the label was applied.For many of the PRs that I closed but did not resolve under this approach, I added a comment with the domain(s) involved to issue #3069. These comments start at about #3069 (comment) if any wants to review them.
There are unresolved concerns about important parts of this idea, such as @J0WI 's comment at #13859 (comment) regarding how much review/feedback PRs should get before they are closed, and @Hainish 's comment at #13859 (comment) about how much ownership/obligation a maintainer has toward a PR they've marked with the label.
(As a side note, regarding @Hainish 's concerns, I don't accept any continuing obligation to the PRs that I've closed but not resolved under this approach. For example if one of those contributors adds a comment to their PR sometime in the future asking to continue work on it, I'm not going to feel required to see it through.)
I've deleted the label since I was the only one using it, and since it is not currently applied to anything (earlier today I closed the remaining items that had it applied). Given the unresolved concerns mentioned above, it should probably not be applied to anything else.