P5.js: PR Process

[mcturner1995]

Hi Everyone,

My name is Moira and I am the new project lead for p5. As part of my role, I tend to the PRs of the p5 project and p5 website. However, I wanted to get some community input about the PR review process and empower you all to take more ownership of the process. Specifically, my questions are:
How many reviewers should take a look at a PR before it’s ready to merge?
Who should have access to merging the PR? Should the PR assignee be given access to merge?

I wanted to bring these questions up because I don’t want to create a bottleneck when it comes to the PR process. What do you all think?

[welcome[bot]]

Welcome! 👋 Thanks for opening your first issue here! And to ensure the community is able to respond to your issue, be sure to follow the issue template if you haven't already.

[montoyamoraga]

hi @mcturner1995 :)
my guess is at least 1 person reviewing each PR,
i try to take care of the issues i know how to handle,
and sometimes ask for review of people who i know can take care of them

maybe we can formalize the process of knowing who can help with what,
we could ask the most active contributors on the repos to this section
https://github.com/processing/p5.js#stewards

[limzykenneth]

There was some discussion in #3893 and #4230 around this which led to the ideas of stewards that @montoyamoraga linked to.

I think it is good to formalize stewards for different types of contributions to the repos (including the website) and these stewards, apart from being familiar with said area, will also be able to approve and merge the PRs for the most part. A steward will also be responsible for reviewing the changes and ask for additional changes if required.

However I also want to seperate out the PRs themselves in to maybe three categories:

• simple fix: such as typos in documentation, these usually don't require an issue filed beforehand
• bug fix: an incorrect behaviour or behaviour that doesn't match with the documentation, these should have issues filed first
• new features: new features to the library, must have issue filed and a case for accesibility be made first before PR is filed

Of these categories, simple fix and bug fix can probably be reviewed and merged by one person (for simple fix anyone with push access, for bug fix the steward). For new features it should be agreed upon by more than one maintainer in the issues to include that feature before a PR is filed and PR approved by more than one maintainer before merging.

For assigning stewards, I think we can be relatively loose here, ie. multiple stewards for any one section and assigning the same person to a few areas.

[outofambit]

Hi @mcturner1995! Thanks for starting this conversation :)

@limzykenneth I love that breakdown of different types of PRs and the review requirements you proposed there. That sounds great to me. :)

For assigning stewards, I think we can be relatively loose here, ie. multiple stewards for any one section and assigning the same person to a few areas.

Yeah (I think?) that was the initial intention for the stewards concept. (I've been slow to add myself there, but going to do that soon. @limzykenneth @montoyamoraga no pressure, but I think you'd be good candidates too!)

Who should have access to merging the PR? Should the PR assignee be given access to merge?

I propose that we start with the reviewer merging the PR once it's all approved and see how that goes. I don't think it has to be a hard rule (especially if the PR author already has merge privileges), but maybe as a rule of thumb? This seems simpler than having the extra back and forth between the reviewer and the author, but also I can see the educational value of asking the author to merge the PR themselves. Perhaps we could say the reviewer is responsible for merging _or_ explicitly asking the author to do so?

[montoyamoraga]

hi @outofambit i just saw your PR request, it looks great to me, and yes i would love to be added as steward, i will do my own PR soon about it too, and i agree with the breakdown of @limzykenneth of PRs :)

in my case i try to merge PRs that i am comfortable with, and also close issues that are either stale or need no further discussion, and also i try to add people to the all-contributors list, and answer beginner doubts and direct people to the forum when it's applicable, i will think of a wording to summaerize my my steward role :)