AprilSylph
ci: mark pull requests as stale after 1 year
Description
Some good reading:
- https://www.hanselman.com/blog/always-be-closingpull-requests
- https://www.jeffgeerling.com/blog/2016/why-i-close-prs-oss-project-maintainer-notes
I don't want to be rejecting pull requests on a whim, but if there is an extended lack interest on either side of a pull request to actually get it merged, I do think it should be rejected by default.
A closed pull request's code can still be viewed regardless of what then happens to its branch, so this doesn't destroy any information—it just better reflects the state of the repository.
What are your thoughts on this?
Testing steps
N/A
I think I would start this discussion by going over what information we're trying to communicate with a PR's existence/last-updated time/draft state/review request state then picking a policy that facilitates that (and, if applicable, noting any psychological effects we think might result from those choices).
For example: I don't know whether, in general, you've seen all of the April-works-at-Tumblr-era PRs. I thus don't currently conclude anything about your interest level or lack thereof in a PR based on whether you've interacted with it! On the other hand, maybe you generally have glanced at them all and have placed things in your review queue based on interest.
In the absence of knowing about your process, and with most open PRs authored by me, I see choosing a heuristic like this as pretty much entirely about reflecting what your process is, so it's hard for me to have much of an opinion about it!
On more practical matters, a) I of course can have an opinion re: PRs authored by neither of us (in which case, yeah, I think this makes sense, and maybe this is what you intended this for) and b) that is actually all of the PRs it would currently affect because I have branch-updated mine; I'm not sure whether it's intended that this means the frequency with which I branch-update mine affects whether they go stale—not an unreasonable situation by any means, but not necessarily one whoch communicates much, re: the first paragraph, if that's the goal (which maybe it's not).
(If it is, I can do another pass of moving drafts to my fork and that sort of thing. I always figured we would get to a point of doing cleanup if and when you became more active after the Tumblr era. But that could also consist of me explicitly telling you that you have my blessing to, as you said, reject pull requests on a whim; there's no point in keeping a PR open if you're definitively not interested in it, but I'll continue branch updating it if I have no idea.)
I think I would start this discussion by going over what information we're trying to communicate with a PR's existence/last-updated time/draft state/review request state
Philosophical discussions? In my repository? It's exactly as likely as you think.
I suppose I can start with what I personally see a pull request as being, when I myself open one...
...well, it varies. The vast majority of the time (see: my recent PRs) it's a signal that I intend, with certainty, to make a given set of changes to the repository. I want to add a feature, or I want to fix a bug, or I want to do some refactoring because I started out trying to add a feature or fix a bug and found the existing code in need of renovation. The common factor is "I want to"—if there was nobody around to review these changes, I would likely just push them straight to the main branch.
For that type of PR, the state simply reflects how far it is from the finish line of being merged, which I as the author want to get it there as soon as possible:
- A draft needs more work done before it is ready for review
- An open PR with no requested reviewers is waiting[for what?] to be put in someone's review queue
- An open PR with changes requested needs to be updated before it is ready for re-review
- An open PR where the last review was neutral needs its discussions resolved
- An approved PR should be ready to merge, although the reviewer can also leave comments that are optional to address prior to merging (e.g. code nitpicks)
The other and much rarer type of PR for me to open is this type—a change that requires a lengthy conversation to figure out if it has a good premise or not. Hypocrite (or maybe just past-me hater) that I am, I don't think I want this type of PR to exist... conversations on whether or not to do things are probably best left to conversational features like discussions and issues.
Now that this PR exists and has a conversation attached, I think the conversation should continue here, but I should at least put it into draft state to signal the level of intent to merge. This PR needs more work before it's ready for code review—we need to discuss what its intentions are and what its effects would be if merged, and a discussion is a form of work.
This brings me nicely to the other PR philosophy I've subscribed to in the time since I created this repository—a pull request is a conversation. It's a plan of action to be reviewed, revised, and then finally executed. It may feel like the work is already done before you've opened a pull request, and a pull request is just the final hurdle... but really, the work is only done when the PR is merged, and that requires both you and your reviewer to be on the same page that yes, this is a good change to make; yes, this is a good way to do it.
A stale PR is a dead conversation; a PR is a conversation; a stale PR is a dead PR.
However, dead conversations on the internet can be revived! That's my goal with adding stale PR comments—it's a call to action for the author (which, yes, is chiefly just you, you fantastic crazy person) to re-assess if the PR is worth reviving.
Which should bring us to what criteria to use in that assessment, but this is a very long comment already and I'm tired of typing it. I'm sure I've said something here which will spawn its own tangent of conversation anyway.
Referencing the conversation we're having contemporaneously in another PR (for future reference: https://github.com/AprilSylph/XKit-Rewritten/pull/1997#discussion_r2549150599)...
In that case, you to some extent correctly identified an action I could take, but was not taking, to make the Github interface reflect what we want. Similar point here. If we're to have conversations on whether or not to do things primarily in conversational features like discussions and issues, which is a preference you noted, attaching code (which is naturally, at least often, how I do it) would be done via a link to a branch compare. Slightly clunkier than a PR—no automatic updates when you push—but I think that's a reasonable tradeoff for organization; we can always make a PR if the answer to whether or not to do things is yes.
I will note that re:
You're a collaborator on this repository, so please try to think of yourself more as a co-owner! Ideally, even if you are still approving code where you somewhat disagree with
Code (and feature decisions) aside, I do primarily defer to you—we could, obviously, change this—on matters of organization and time prioritization. This isn't because of your authority, though, or even necessarily because I trust you to have the perfect answer: it's because I think you're the one affected, so your preferences are the ones that best facilitate the project and so should get most of the weight! (At least as it currently stands. If the situation arises where I am the one making the final decision on which of your 75 PRs to review, you'd better believe I'll feel free to express organizational preferences :D)
Anyway, I like this framing: yes, this is a good change to make; yes, this is a good way to do it.
Trying to use that framing in answering what I personally see a pull request as being, I currently would categorize it like:
- Substantial code written, but incomplete.
- Reviewable; has not been seen by reviewer. Open question: is this a good change to make?
- Reviewable; in review queue (because reviewer saw it and decided to do so, or because author hit request to signal intent). Open question: is this a good change to make?
- Out of review queue because of discussion. This is a good change to make. Open question: is this a good way to do it? /// Out of review queue because of requested changes. This is a good change to make, but a bad way to do it; go find a better way to do it, author!
- Reviewable; in review queue (after changes). This is a good change to make; author thinks it's a good way to do it. Open questions: does reviewer agree; does code pass tests/inspire confidence?
- Approved; waiting on merge. This is a good change to make; this is a good way to d o it; code passes tests and inspires confidence.
My assumptions are that:
- If it's not done, we want to agree not to PR it. (If so, I can convert my incomplete PRs to PRs within my fork. Happy to do this.)
- If it's not yet agreed upon as a good change to make, as per
conversations on whether or not to do things are probably best left to conversational features like discussions and issues, we could agree not to PR it until we talk about it. (We could implement this starting now and do something a little different with outstanding PRs, or I could convert my outstanding, non-interacted PRs to e.g. discussions, or something vaguely like that.)
Not sure what "draft" and "open, but intentionally not review requested" should represent, if anything. I kind of thought this part of the post would go somewhere when I started writing it. Um,
Okay, I think I've got them:
- Like the PR we are currently in, should PRs be draft-converted if (4) they have been agreed upon to be worth investigating, but are change-requested and the changes have not been resolved? As you said,
This PR needs more work before it's ready for code review. Should a draft PR, correctly used, have been a non-draft PR earlier in its lifecycle? An open PR with no requested reviewers is waiting[for what?]- To use a concrete example, I did not review request #1912, as I do consider it reviewable, but also consider it unimportant, and prior to XKit activity ramping up as of quite recently I figured that distinction would be helpful. I, of course, never asked you about this. What would you find useful here?
This isn't because of your authority, though, or even necessarily because I trust you to have the perfect answer: it's because I think you're the one affected, so your preferences are the ones that best facilitate the project and so should get most of the weight!
This is a good way of thinking of things, I like it!
1. If it's not done, we want to agree not to PR it.
If it's not done and there is no immediate intention to complete it, I would prefer it not be PR'd, yes. Ideally, a draft PR represents intention to open it, the same way an open PR should represent intention to merge it.
Personally, I only open a draft PR when I think my code changes are complete, but the pull request description and testing steps are not fully written yet. And, in a lot of cases, I end up making additional code changes as I write the PR description, and realise that my internal rationale for certain things makes no sense when explained in real language.
Going back to intention, though—I think the biggest utility a draft PR has is as a signal to others that this particular thing is already being handled, and so there's no need for them to do any work to the same end. In this sense, a draft PR with no intention to be opened feels a little bit like a broken promise. (And yes, I know it's largely just the two of us doing anything here, but operating as if there are no other contributors feels like a good way to ensure that fact.)
2. If it's not yet agreed upon as a good change to make, as per
conversations on whether or not to do things are probably best left to conversational features like discussions and issues, we could agree not to PR it until we talk about it. (We could implement this starting now and do something a little different with outstanding PRs, or I could convert my outstanding, non-interacted PRs to e.g. discussions, or something vaguely like that.)
Yes, yes please.
The ideal I would like to strive for—and this is something I would call part of my concept of "development heaven"—is for every open fix/feature PR to be linked to a confirmed open bug/enhancement issue. We already have the concept of confirmed issues being practiced in this repository; collaborators can confirm bugs by reproducing them, but only the owner can "confirm" (perhaps more accurately "accept") enhancement issues. If we can start treating issue confirmation as a necessary step in the PR creation process, I think that drastically improves expectations about which PRs should be looked at and which ones should be closed.
This does, of course, technically make me a hypocrite for opening #1999 without first opening a bug issue for the problems it fixes—but I think there's nuance here. A PR can double as an issue by describing the issue it resolves in its description, and I think that's fine as long as it doesn't delay acknowledgement of the issue. If you discover a bug and open a fix PR the same day, that's fine; if you discover a bug and only disclose it in a fix PR a week or a month later, that's not okay.
- Like the PR we are currently in, should PRs be draft-converted if (4) they have been agreed upon to be worth investigating, but are change-requested and the changes have not been resolved?
Nah, "draft" is a decidedly different state from "changes requested". "Not ready for review" ≠"Not ready for re-review".
I've seen review→draft conversions happen at work, but only when the PR author decides the best way to address the feedback is to entirely redo the PR. I'm not sure what value that really has over just leaving the PR in a changes-requested state for a longer-than-usual period of time, though.
An open PR with no requested reviewers is waiting[for what?]- To use a concrete example, I did not review request #​1912 as I do consider it reviewable, but also consider it unimportant, and prior to XKit activity ramping up as of quite recently I figured that distinction would be helpful. I, of course, never asked you about this. What would you find useful here?
I will put it out there that I will almost never go looking for PRs to put in my own review queue. They need to be review-requested at some point, or they will be open forever.
However, I do think it's good to be mindful of a maintainer's existing review queue. Requesting my review on 20 PRs at once is more likely to overwhelm me than to immediately get anything done; requesting my review on just one or two PRs doesn't have that risk. Feel free to steer my attention with selective review requests this way.
I think what might work best is to ensure that on any given day, what you consider to be your most important PR and what you believe is your easiest-to-review PR are both review-requested. In time, if I do my job as a maintainer, you won't have enough PRs open to have to be selective about review requests at all.
Development heaven can be real, we just need to figure out what to put in CONTRIBUTING.md to make it real.
I think the biggest utility a draft PR has is as a signal to others that this particular thing is already being handled, and so there's no need for them to do any work to the same end.
I think the only thing I differed from you on in this case is that I have a state in my model for "this particular thing is being handled to some degree; I want to signal to other that there is no need for them to duplicate the work which I have done, but my work is insufficient to complete the PR (and I have either no immediate intention, or insufficient ability, to complete it.)
Filling the PR list with these definitely doesn't feel like a good organizational strategy, I fully agree assuming that's also your take. I also feel like that's absolutely what, literally speaking, a "draft PR" would refer to; not sure how I would make changes to the github interface based on this fact given hypothetical god mode over the github UI, but that's neither here nor there.
Anyway: any thoughts on what a good policy would be to do in this case? (I will grant that my mental model includes e.g. "hey it might save you some time to either look at or cherry-pick-and-unstage this diff, if you want to try to solve this yourself"—example: https://github.com/AprilSylph/XKit-Rewritten/tree/marcustyphoon/use-new-dom-util—and that may be almost entirely pointless if your brain does not work in such a way where you would ever do that. Well, do the latter thing, I guess.)
I think there is some value to publishing incomplete code for the reason you put forward, but there are ways to do it that are not draft PRs. You can link a branch diff, you can save the diff to a .patch file and upload it in an issue/comment, or smaller patches just paste directly into an issue comment... you can even open a draft PR and then immediately close it! I think I would prefer any of these to a perpetually-open draft PR.
Yeah, I was hoping you might show a preference toward one of those options in particular :D But that may be getting ahead of ourselves; I haven't put a lot of thought into what my alternative is going to be. Maybe I'll convert my current draft PR set to comments in a single discussion thread. Those can be replied to individually. (Annoying that comments here cannot.)
A PR can double as an issue by describing the issue it resolves in its description, and I think that's fine as long as it doesn't delay acknowledgement of the issue.
Yep, I've been following this. I assume we're relaxing "collaborators confirm bugs by reproducing them" when a collaborator found the bug (assuming confidence).
Personally, I only open a draft PR when I think my code changes are complete, but the pull request description and testing steps are not fully written yet.
So treating "draft" as referring to the literal PR material, not the branch. Seems reasonable to me.
I will put it out there that I will almost never go looking for PRs to put in my own review queue.
I certainly assumed this was the case, but was not sure whether you intended to do so if your review queue and assigned issues list both became empty (and I had no important PRs to highlight). In that hypothetical scenario, does the ability to, as you put it, steer your own attention sound at all appealing? I have no preference, really; I could see an argument that it's more efficient for me to do what you said above, as I'm the one who already knows what my PRs do.
(Well... it is kind of weird to try to pick the most "important" of a bunch of refactor or feature PRs as not the person deciding whether to do them, but—whatever: most likely to be accepted, the one I like the most, a random one, doesn't really matter at that point; like you said, in a perfect world we go through them all at some point so the order is inconsequential.
The thing I actually find difficult about picking one for the non-"easiest-to-review" category is that some of them are really quite hard, and potentially not worth—right, right, doesn't matter: you get to judge that; the request is to call your attention to one so you can make that judgement.)
I assume we're relaxing "collaborators confirm bugs by reproducing them" when a collaborator found the bug (assuming confidence).
You assume correctly! :o:
I certainly assumed this was the case, but was not sure whether you intended to do so if your review queue and assigned issues list both became empty (and I had no important PRs to highlight). In that hypothetical scenario, does the ability to, as you put it, steer your own attention sound at all appealing? I have no preference, really; I could see an argument that it's more efficient for me to do what you said above, as I'm the one who already knows what my PRs do.
I agree with this argument, and would have made it myself if you had not just made it for me. If I think something needs doing, and there's no PR for that in my review queue, it's incredibly likely that I will completely ignore the open PRs list and just do it myself (e.g. #1996).
The thing I actually find difficult about picking one for the non-"easiest-to-review" category is that some of them are really quite hard, and potentially not worth—right, right, doesn't matter: you get to judge that; the request is to call your attention to one so you can make that judgement.
I think I would start this discussion by going over what information we're trying to communicate with a PR's existence/last-updated time/draft state/review request state
Having done so, back to the matter actually at hand: sure, this seems reasonable.