Silverstripe-framework: RFC: Clarify our expectations around draft PRs in core repositories

[ScopeyNZ]

Hey team, as more of a "part-time" contributor now, I'm looking for ways to vet my notifications. I'm sure we're all in the camp that the stream of notifications from GitHub is pretty overwhelming, and I personally don't think draft PRs are helping that.

Our contribution docs have a step-by-step guide that indicate that raising a PR happens once you have completed work on an issue, which I think is a great idea. PRs that aren't complete are just a waste of time to review. We don't have any clarifications around draft PRs though

With that, I'll give my opinion on what _I think_ is the ideal stance on draft PRs in a large open source community:

Raising any PR based on incomplete work is discouraged. Because of this, usage of GitHubs draft PR feature is also discouraged. However, there are some cases where draft PRs are acceptable:

• If you are collaborating with a core contributor, and that contributor has asked you raise a draft PR with your WIP. (Note that I think we should ask to collaborate on a fork over this though)
• If you are working on an open issue, and are close to a solution, but you need some specific assistance on something you are stuck on. This should be accompanied with a direct request in the PR description

---

Would anyone like to weigh in their own opinions? Once we reach a consensus after a vote (if we need it), I'm happy to update the docs on this 🙂

[robbieaverill]

I have disliked draft PRs since they were introduced, so I'm all for voting for them not to be used on our repos. It would be nice if you could disable them from repo settings.

I suspect that the majority of the core team have probably turned off their GitHub notifications already due to the volume of incoming traffic - you're right that draft PRs don't make that any better.

I dislike seeing pull requests in my Silverstripe PRs bookmark list because I know that they aren't ready to merge, and take up screen real estate from the other PRs that are (252 open PRs at time of writing). I can filter draft PRs out of this view though, so this point doesn't hold much weight in this issue's context.

I think that this RFC is going to have much more of an impact on the Silverstripe Ltd team than the rest of the core team, so I'm hesitant to vote on it, but I think your suggested policies are sensible.

[dhensby]

I don't have much of an issue with draft PRs, I'd rather people use them than open PRs that are just prefaced "WIP".

I think there probably is a place for draft PRs as it can be useful to get feedback on incomplete implementations of work. For example, before a complete set of tests has been written. I don't think turning off draft PRs at a repo level (if it could be done) would stop people opening PRs that weren't complete.

There needs to be a balance for contributors that that they don't feel overwhelmed that their work is perfect before they can open a PR for feedback.

I think if there's a body of work that you know is nowhere near complete and is going to need a lot of continuous work, then it probably shouldn't be a PR yet. It's worth remembering that every push takes up a travis build which can mean PRs in need of more imminent review are stalled.

---

I guess in summary what I'm saying is, I think they have a purpose, but that's not just to be a placeholder for a long-running piece of work.

[chillu]

@steve-silverstripe @Cheddam @dnsl48 and the wider CMS Squad is focusing on getting triage processes and expectations a bit more streamlined, which includes closing inactive PRs. I'm with Dan on this in general. I care more about stale PRs, and less about whether they're explicitly called "draft" or they're just implicitly unfinished. PRs should have a clear and active owner, and a pathway to completion.

The benefit of PRs is that you can review code in a way that "survives" force pushes. That's harder to do on a stream of commits in a fork. A good example is this long-lived PR for GraphQL 4. It has a clear owner and momentum, but also isn't finished. But I can comment on code while Aaron is developing it. You could argue that the same can be achieved through sending a pseudo-PR against the fork base though.

[sminnee]

It would be good to understand how much of an issue this is now that we're a bit tighter about closing stale PRs.

From my perspective WIP PRs a common part of the flow inasmuch as anything with changes requested effectively goes back to being WIP. So where never going to be free of WIP, although we might reduce it.

I've got no strong views on the use of draft PRs vs WIP in the title. Is one of these easier to filter out of an otherwise noisy list? If so, maybe we should use that one? We could also immediately self-review with "changes requested" if that were helpful.

I know I'm a notable culprit on this. One reason is that I will start a PR when I have made something that is suitable for use in 1 project. I do this to reduce the chance of this needing to be reimplemented for a 2nd project - to make the work in progress more visible. I kind of feel like pressure not to let these gets stale (the new process) increases the chance of these PRs being finished, whereas if they remained on private branches they would likely die.

@unclecheese's GraphQL4 PR is a useful means of collaboration. The only comment I'd make there is that after 100 comments or so it might be worth merging to master and staring a new PR (at a suitable juncture) - the lack of granularity is a bit awkward. Under such a scheme you outline where you got to in the first PR and what your goals are for the 2nd. So WIP is fine, massive PRs less so.

In some of my recent work I've been trying to bite off less controversial pieces of big initiatives and get them merged earlier. There's a bit of busy work in this but it does reduce branch rot.

[chillu]

OK, sounds like there's no strong feelings against draft PRs on here, it's just a matter of good management. I think this can be summarised in a pretty simple guidance for contributors:

Raise PRs if you think they are ready to review and merge. Draft PRs are only acceptable when you are actively collaborating with a maintainer on this module, and take ownership of getting the PR merged in the next couple of weeks