Gutenberg: Create a "WordPress/gutenberg-code-review" group?

[danielbachhuber]

To increase the pool of code reviewers, can we create a WordPress/gutenberg-code-review group? It seems WordPress/gutenberg is too broad:

The WordPress/gutenberg-code-review group would be people who have self-identified as being comfortable to make merge decisions on pull requests.

From https://wordpress.slack.com/archives/C02QB2JS7/p1525871102000146

cc @WordPress/gutenberg-core

[tofumatt]

Works for me, and the name would help new members of the team understand who to flag for review (even now the difference between gutenberg-core and gutenberg is not always clear to me when requesting a review).

[melchoyce]

This would be great for clearing up my email 😆 I'm on that list, but I can't do code reviews.

[tofumatt]

I promise I won't bike shed more than this (😅) but gutenberg-developers might work as well, if the issue is that there are a lot of non-developers who don't want to do code reviews in the main team. If anything it makes clearer than one group is for code, another is for bigger picture things.

Heck, come to think of it we should have a gutenberg-design team for reviews on design issues rather than having folks know who on the design to individually request for review (or the thing where we have a label for design feedback but we request reviews from developers).

[karmatosed]

To check, the initial idea was to solve an issue some felt of reviews taking too long. If that's the case I am unsure how another list solves that. I don't deny it stops needless emails for some (always a good thing), but it potentially means some could miss out on reviews and even a smaller pool of reviewers.

@tofumatt splitting into design/developers I feel is potentially a step not needed, many of us in design do some code (CSS side at least) which needs reviews. Some even go beyond that and I think that's totally ok. I also don't think size wise it makes sense to split that. I can also see people getting confused who asking and it could get complicated.

If the above points aren't an issue I won't step in way of it, I just think we should explore ways of enabling everyone to feel comfortable reviewing over create another list.

[danielbachhuber]

@karmatosed Do you have an alternative to suggest for consideration?

[karmatosed]

@danielbachhuber I think shifting focus onto enabling reviews and encouraging is probably the route that solves the issues discussed. Let's see what others think as I very much want this to work for everyone here.

(even now the difference between gutenberg-core and gutenberg is not always clear to me when requesting a review).

This feels like a side problem but one we should investigate into fixing. I don't know the answer there, just poking around I can't see a way to limit or declare that easier. Perhaps this leads to documentation being a good starting point. PR's don't have the same awesome templates (unless I'm missing that option) as issues do, otherwise we could utilise that.

[noisysocks]

I had it in my head that gutenberg-core was for people who were comfortable with reviewing PRs. It's probably worth revisiting our groups and making sure we're all on the same page 🙂

Some mostly off-the-cuff ideas for encouraging reviews:

• Write some guidelines about what makes a review a good review. If there were e.g. a checklist that new reviewers could go through then that might encourage them
• Encourage a culture of iteration rather than getting things right on the first try. Part of the scariness of reviewing a PR is a fear that what you're signing off on isn't perfect
• Encourage tagging individuals rather than groups where it makes sense, e.g. for PRs that don't need a lot of consensus. If a group is tagged, it's easy for everyone in the group to say _someone else will review this_
  
  
  
  • I personally find that review requests I receive because GitHub suggested me are more approachable because I understand the code
  
  
  • We could maybe experiment with code ownership to suggest individuals to tag
  
  
  • Maybe we can designate a release manager to, every so often, go through PRs that are important for the next release and assign them to contributors in a round-robin fashion
  
  

[gziolo]

I would start with making sure that we have proper labels assigned to all PRs to make triaging easier rather than making it easier to ping a larger group of potential reviewers. The thing is that we should have more people doing reviews in the first place. It's perfectly fine to have people only testing the changes or reviewing only parts of the code, or doing copy or a11y audit only.

[tofumatt]

@tofumatt splitting into design/developers I feel is potentially a step not needed, many of us in design do some code (CSS side at least) which needs reviews. Some even go beyond that and I think that's totally ok. I also don't think size wise it makes sense to split that. I can also see people getting confused who asking and it could get complicated.

Folks could be part of both groups! 🙂

PR's don't have the same awesome templates (unless I'm missing that option) as issues do, otherwise we could utilise that.

We have a pull request template so we could add who to flag for what if that's what you meant. That'd be a cool idea actually.

I would start with making sure that we have proper labels assigned to all PRs to make triaging easier rather than making it easier to ping a larger group of potential reviewers.

The nice thing about code reviews is you can see them in a queue on GitHub (without using other tools) and you are notified about them. You can ignore a review easily if you don't have time/aren't the right person to review it. Having to actively look through the queue is, at least for me, less preferable to simply being pinged when my attention is requested for a review.

Maybe the process should be: for simple reviews that request an entire team, the first person to review re-assigns the review to just them. 🤷‍♂️

Also GitHub's suggested reviewer feature works pretty well.

---

The real question is: how many folks are actually pinged too often for review? Are there many cases where a group of people are requested for review with no feedback? I haven't noticed that yet but hey I'm New Here™. I notice plenty of people just ignore reviews if someone else is handling it which is fine. Maybe everything is already fine. 😁

[danielbachhuber]

Maybe everything is already fine.

The review cycle is quite slow (at least for non-Automatticians). My expectation is that new code would see an initial review _at least_ within 24 hours, preferably within 12, during the work week.

On the topic, this is a good article to read:

I believe that the single most important thing a team can do collectively to improve its throughput is to make code review a (or better yet, the) top priority for all team members. This is the most compelling and concise reason I could come up with, so I’ll emphasize it as strongly as I can:

Pending code reviews represent blocked threads of execution.

The principle applies across the board ("Needs Design Feedback", etc.). A contributor is much more likely to churn if feedback is delayed by an extended period of time. One of the greatest things the Gutenberg project can do to improve contributions is shorten the feedback cycle.

[karmatosed]

@tofumatt just to reply:

We have a pull request template so we could add who to flag for what if that's what you meant. That'd be a cool idea actually.

Issues now have the ability to split into types, it's amazing just checking out by clicking 'new issue now'. What I was meaning was it's a shame we can't say 'hey this needs a code review' or 'hey this needs a design review' at that level and give templates just for those types. One PR template for all cases is great we have that but it doesn't adapt.

[aduth]

The review cycle is quite slow (at least for non-Automatticians). My expectation is that new code would see an initial review at least within 24 hours, preferably within 12, during the work week.

If there's a subconscious favoritism, I certainly don't benefit from it. I'll frequently go a week or longer without feedback on a pull request. But I spend the downtime reviewing others' pull requests or working on other issues, as this is the best that I can do individually to tackle the broader systemic problem of lack of reviewer support.

It'd be great if a review could occur within 24 hours. Is creating a new group going to help to get there? Speaking for myself, I'm well aware of where to look when I have the availability to do a review, and promoting the idea that a group of self-identified experts is responsible for reviews is not going to allow us to achieve this 24 hour goal. It's exclusionary and perpetuates an us-vs-them mentality, where the "us" — those proposing changes — are separate from the "them" — those qualified to submit feedback. This can lead to discontentment, manifesting itself in various forms (such as a implications of favoritism).

What I'd like to see instead is breaking down this idea that one must be in a certain position of expertise or comfort to be qualified for submitting reviews, and further that there's a balance of karma in expectations of receiving a review to contributing back in kind. Providing links to resources on performing reviews, and being okay to be wrong, at least so far as highlighting potential confusion and/or not being wrong at all and iterating toward the best solution.

Gamification of contributions could be an interesting venture, with a (non-monetary) bounty system of sorts. An obvious problem is it requires time investment to set-up, so it isn't an immediate remedy (though my interest is certainly piqued to take on). You might also be interested in this talk from WordCamp US 2015 by @timmyc where a similar system was employed to encourage contribution in the development of WordPress.com's editor. Also StackOverflow's bounty system.

[pento]

I agree that there's a problem here in need of addressing, and having designated reviewers (either a group, or individuals responsible for different areas) is probably one part of the solution.

That said, I don't believe that 12-24 hours for a review turnaround is an achievable goal in the short term, so I wouldn't want to get hung up on that. Sometimes, it can take a few days or a week for something to get reviewed. I don't mind that for now, as it gives folks a chance let the issue tumble around in their head for a bit before they need to start actively participating in it. I would certainly like to see us pushing towards shorter review turnarounds, though.

Anyway, now seems like a good a time as any to lay out my current thinking for review processes, project structure, that kind of thing. If this doesn't interest you, feel free to jump to the tl;dr at the bottom.

A Treatise on Encouraging Reviews Through Cooperation (draft) 🙃

A disclaimer:

_This idea is still evolving, it is by no means the thing that will come to pass for Core. If you think it needs improvement, then a) you'd be right; and b) the best way to improve it to is to help it evolve further. Let's figure out the shortcomings in our processes together._

When considering Gutenberg's processes, one of main the things I'm looking for is how it's going to translate to all of WordPress Core. Ultimately, Core will adopt many of the practices we're modelling here. With that in mind, having a group of people responsible for all reviews across all of Gutenberg isn't a particularly scalable option: as @noisysocks mentioned, tagging the entire group encourages a _someone else will review this_ mindset.

At the other end of the scale, tagging individual contributors is possibly an option for when you have a really good idea of what suits each individual, but that doesn't work for new contributors, or even folks who are passingly familiar with who's working on what.

Ultimately, what I'd like to aim for is a process fairly similar to Core's, where:

Folks who've contributed a little bit (whether it be in the form of PRs, testing, triage, or generally being helpful) are quickly added to a group where they're able to help with tagging, assigning, triage, that kind of thing. Comparing this to Core, this is effectively adding a small barrier between "opening a ticket" and "milestoning a ticket": I think this barrier is necessary, due to the significant lower barrier to creating an issue, far more people have GitHub accounts than have WP.org accounts.

Folks who've been contributing PRs for a while, and have shown themselves to have good judgement, can become owners, who can review PRs in areas that they're familiar with. This is approximately equivalent to Core's component maintainers, and I suspect would be part of the technical solution to address this issue.

The next step, merging, is similar to Core's committers at first glance. I'm not sure that's the right way to go, however. I'd like to see it flatten a bit, giving owners the authority to say "this PR should be merged as is", and mergers is more like an administrative role: ensuring the right owner groups have reviewed the PR, it's tagged appropriately, that the merge commit contains all the right Co-authored-by tags, etc. To take an arbitrary example (that probably doesn't reflect actual owner groups that would exist), an "Editor" owner group might be tagged if anything in the /editor directory is modified in a PR. It's the job of a merger to ensure the PR has also been reviewed by someone in the "Data" group, if the PR also includes changes to the /editor/store directory, or the "Documentation" group when there are significant documentation changes. It's also their role to know when this is not really necessary, as introducing artificial roadblocks doesn't benefit anyone.

In many ways, I'm thinking of "owners" and "mergers" as simply being two aspects of a similar role: one with a focus and deep knowledge of the thing (or things) they work on, the other trading off having a deep knowledge of particular areas, for a wider view of the entire project. In Core, we've often used commit access as a "this person is trusted to make good decisions" flag, even if they don't really need commit access. It's important to allow those people to continue their good work, without feeling the pressure of "will this thing I commit break the internet".

It's likely that a person would be an owner (probably in several owner groups) before they became a merger: this isn't an indication of seniority, simply a likely outcome of the merger role requiring knowledge of how many of the parts of Core work together, which requires experience working with those parts.

Finally, I think there's a need to formalise something we've been doing on an ad-hoc basis in Core for a while: add a "project manager" group, as a third aspect of the "this person is trusted to make good decisions" role. This has some crossover with mergers, but focusses more on the day-to-day of knowing _who_ works on what parts, rather than how those parts work together. Nudging individual owners to do reviews, ensuring the owner groups are accurate, keeping milestones moving, etc.

The PM group is an important aspect of solving this particular issue: it's easy enough to set up some GitHub teams and commit a CODEOWNERS file, but that'll become stale and useless pretty quickly without it being maintained. Having people who are trusted to ping individual owners when a PR has stalled means we're making it easier for new contributors to gain easier wins.

Of course, this is only a brief overview (he says, like, 1000 words in) of how I think this could work, it doesn't cover many other important aspects, including (but not limited to):

• How do new contributors learn about this process? What information do they need to know about immediately, and what can they learn as they go?
• How do we ensure these roles are seen as being fluid, and not necessarily a "status" title to grip on tightly to? This is most effective when it's only people who are actively doing the work who are in these groups.
• How do we encourage a sense of ownership for people in each role? Prestige or public recognition, mild gamification, or something else? How do we avoid a "sense of ownership" turning into gatekeeping?
• How does a person move into one of these roles, or become an owner for another area? Who decides that they're "a person trusted to make good decisions"?
• How do we handle conflict resolution? In particular, what happens when owners or mergers disagree?

These don't need to be answered now, but would need to be addressed as part of looking at how to change Core's processes. For the purposes of not holding back Gutenberg, I would suggest this tl;dr:

---

• There are probably only a few owner groups that could be automatically pinged based on specific areas, but mostly it would fall back to the general group of people who are actively working on Gutenberg. (Both full time and part time.)
• There are several folks doing PM work on Gutenberg, though no-one is currently focussed on it full time. I don't believe this is a blocker to implementing a process change for Gutenberg, just something for everyone to be aware of: if you see a PR that hasn't been reviewed by a group member, but you know who can do it, you should feel free to ping those specific individuals. Ideally, a person focussed on PMing would be able to easily take over from the ad-hoc efforts.
• We already have existing project leads in @karmatosed and @mtias: for questions of maintaining group members, conflict resolution, and anything else that comes up, we should continue to defer to their decisions. If you have a question, suggestion, concern, or comment, you should ping them directly.

I don't believe these changes would cause roadblocks to the existing progress that Gutenberg is making, but I would like feedback from all y'all who are currently working on Gutenberg: do you see this getting in your way? If not, I think it would be a valuable experiment towards seeing how it could work on a Core-sized project.

[mcsf]

Apologies; this was mistakenly closed by a slippery finger.

[danielbachhuber]

@karmatosed @mtias What do you see as next steps?

[danielbachhuber]

Per lunchtime conversation with @karmatosed today, we're going to proceed as normal until Gutenberg 3.0 is complete (i.e. all major features are done). After that point, we can have a conversation about switching up the workflow to accommodate a higher volume of issues and pull requests.

[karmatosed]

Let's close this up as we don't need to keep open for the phase one focus.