Housekeeping: Removing bottlenecks in the review process.

nerrad commented 5 years ago

This issue is created as a result of the conversation in the recent #core-editor meeting about the need to have more Pull Requests reviewers in the repository.

Currently, in Gutenberg, there is a bit of a bottleneck in that there are a relatively small group of people reviewing and approving pull requests compared to the number of code (and otherwise) contributions made. While this is not a new problem for any project of this size, it is something that is worthwhile tackling to help keep momentum.

This issue is created to track an initial approach to solving this problem which has the following action steps:

[ ] Identify various areas in the project that issues/pulls can be classified as.
[ ] Create Github teams for reviewers and approvers over each of those components. It's okay for people to be in more than one team.
[ ] Use the Github code owners feature to assign the various teams to the different project areas.
[ ] Put out a call for people to self-identify with the identified project areas so they can be assigned to the teams.

Gutenberg Areas

Gutenberg is a large application, composed of different packages. A given group of people might only be interested in a given area of the project. From that perspective, there should be reviewers and approvers assigned to each area. The below headings are just a starter. Let's iterate on them. If more granularity is needed depending on interests, we could create smaller areas.

Project Area	Description	paths
data	comprised of all code interacting with or persisting data	data, redux-routine, api-fetch, core-data
blocks	everything to do with block implementation	block-library,format-library
editor	everything to do with the editor implementation	editor, edit-post,list-reusable-blocks,blocks,rich-text,shortcode,annotations,autop,block-serialization-spec-parser,block-serialization-default-parser
tooling		babel-plugin-import-jsx-pragma, babel-preset-default, browserslist-config, custom-templated-path-webpack-plugin,eslint-plugin,e2e-test-utils,e2e-tests,jest-console,jest-preset-default,jest-puppeteer-axe,library-export-default-webpack-plugin,npm-package-json-lint-config,postcss-themes,scripts
UI components	Reusable React components	components,compose,notices,viewport,nux,element
Utilities	Small JS libraries	blob,deprecated,dom-ready,dom,escape-html,html-entities,is-shallow-equal,keycodes,priority-queue,token-list,url,wordcount,a11y,date,i18n
Extensibility		hooks,plugins
Rest API and persistence	Most of the PHP code of the plugin is about REST API endpoints
Docs

Additional Ideas

There were a number of other ideas that came up in the #core-editor meeting for increasing participation for code reviews. In future meetings these additional ideas could be brought up for future iterations on the goal of increasing code review participation.

How can you become a reviewer / approver?

If you’re interested in helping in one of the areas above (or a smaller set), please comment on this issue and we’ll make sure to add you to the corresponding group.

danielbachhuber commented 5 years ago

Previously #11331 (which didn't ever go anywhere).

nerrad commented 5 years ago

@danielbachhuber I wasn't aware of that issue, thanks for the link. There's a few more concrete actions being taken here and while initially I think we'll put out a call for people to self-identify the components they feel most comfortable with - long term I believe the only way we'll see an increase of reviewers will be through direct invitation. Having a process in place to facilitate will hopefully help.

danielbachhuber commented 5 years ago

Yep. I agree with all of the goals mentioned in the description. Only wanted to point out the past attempt at this, which I'll go ahead and close.

youknowriad commented 5 years ago

I think I added all the packages to a given component, it's not very granular at the moment but maybe we can start high-level and iterate.

youknowriad commented 5 years ago

I suggest the @WordPress/gutenberg-core gets added as owner to all the components for now (to avoid breaking our current workflow which works but just lacks reviewers) and other people can ask to be added to specific packages if needed.

People are also free to volunteer to be a reviewer for any given area here.

karmatosed commented 5 years ago

I would caution having 'project components'. I understand application and whilst I see issues with this I am ok seeing if it works. I don't agree in way they are split. We can learn a lot from the way components work in WP over re-inventing a new way that already works.

gziolo commented 5 years ago

That's a great proposal for application components. Excellent work identifying areas which might be grouped together. I guess this would be enough for start and we could quickly learn if there are more groups to add. There might be also slight tweaks applied. To give an example, for the data team you could also assign packages/*/src/store/*.js file. Using the same pattern matching, you could specify that design team can edit all assets which match *.scss, *.svg, *.gif, etc..

youknowriad commented 5 years ago

I think we should clarify that this is not about being a component maintainer or something, it can create confusion with Core. This is essentially to solve one issue we have right now. A few people reviewing and approving PRs, it's about finding a way to involve more people in the reviews.

karmatosed commented 5 years ago

Find or create a karma/bounty system in which people can earn non-monetary credit weighted by their activity (comments, reviews, etc) and offer for redemption in return by request for review or request for issue to be resolved.

I would note this could be incredibly difficult to do and not have bias. I really think focus on the problem and scale up over including this.

@youknowriad thanks for clarifying:

I think we should clarify that this is not about being a component maintainer or something, it can create confusion with Core. This is essentially to solve one issue we have right now. A few people reviewing and approving PRs, it's about finding a way to involve more people in the reviews.

The way this issue reads could easily be confused and clarity is great around this. With that in mind, I even more think 'Project Components' don't need to be included.

karmatosed commented 5 years ago

Graph/visualize the disparity publicly and on an individual level contrasting one's opened pull requests to reviewed pull requests.

I would caution against anything that 'calls out' someone's activity. Every person is welcome to contribute and should be. Graphically watching what one person does over another is a slippery slope to exclusion.

gziolo commented 5 years ago

There needs to be a good balance between contributing to code directly and reviewing work submitted by other contributors. The idea is to have a way to give more recognition to all those who perform design, copy, accessibility or code reviews and do testing.

youknowriad commented 5 years ago

I agree with both :) We should recognize those who perform well but we shouldn't blame/block anyone for not having the time or the willingness to help elsewhere.

danielbachhuber commented 5 years ago

Find or create a karma/bounty system in which people can earn non-monetary credit weighted by their activity (comments, reviews, etc) and offer for redemption in return by request for review or request for issue to be resolved.

I would note this could be incredibly difficult to do and not have bias. I really think focus on the problem and scale up over including this.

I agree. Many people have tried creating reputation systems and very few have been successful. Those systems that are successful require tremendous amount of effort to maintain.

I'd suggest a narrow focus on the problem with explicitly defined steps and measurable outcomes:

Problem: There are too many open, unreviewed pull requests.
Hypothesis: By instituting X, Y, and Z, we think we can solve the problem.
Measurement: We'll review the efficacy of X, Y, and Z by measuring A, B, C.

If we follow this approach, we'd first define A, B, C, and then identify the process of X, Y, Z.

Some options for A, B, C include:

Time it takes to the first review.
Time it takes to merge or close the pull request.
Time of day (UTC) pull requests are submitted.
Total number of comments (potentially an indicator the PR wasn't properly discussed in advance).

Once we have A, B, C in place, it would be interesting to analyze the data before proposing X, Y, and Z:

Which pull requests get merged quickly, and why?
Which pull requests languish for a while, and why?
What's the average time of day that pull requests are submitted? Is there a gap between this and when the core team is active on the project?

Battling a qualitative description like "overwhelming" can end up with us simply tilting at windmills. Once you have quantitative measures, it can be much easier to determine whether your actions have the impact you intend.

youknowriad commented 5 years ago

@danielbachhuber Having a way to measure success is good, If this is something you think you could help with, that's great. We could do a weekly recap during the meetings.

We made some updates to the original issue to make it more actionable. The idea is to collect people interested by each area of Gutenberg. so let's start volunteering.

youknowriad commented 5 years ago

✅ I'm personally still interested in being approver and reviewer for all these areas (for the moment, changing his mind later is ok).

nerrad commented 5 years ago

✅ I'd like to volunteer to be a reviewer for the data and tooling areas.

aduth commented 5 years ago

I'll review/approve just about anything. If anything, less so a focus on blocks.

danielbachhuber commented 5 years ago

Having a way to measure success is good, If this is something you think you could help with, that's great.

Sure. Which metrics do you think best represent success?

gziolo commented 5 years ago

✅ I'm personally still interested in being approver and reviewer for all these areas until we have enough reviewers and approvers :)

mcsf commented 5 years ago

✅ I'm personally still interested in being approver and reviewer for all these areas until we have enough reviewers and approvers :) (grabbed verbatim from gziolo)

nerrad commented 5 years ago

Having a way to measure success is good, If this is something you think you could help with, that's great.

Sure. Which metrics do you think best represent success?

I like the idea of collecting metrics as you suggested earlier @danielbachhuber and agree its a good way to help with measuring whether the goal is being reached.

As far as metrics I wonder if we can use them to identify potential bottlenecks in various identified areas of GB. In which case we'd need to be collecting metrics for both the entire project and also for each identified area.

It'd be good if the metrics help answer the questions (spitballing here):

Which areas of Gutenberg have more issues/pulls opened vs closed in a given period of time? Is that consistent? How is it trending?
Which areas of Gutenberg have more velocity?
Which areas have more of a bus factor (i.e. is there a lopsided number of pulls created/reviewed by fewer amount of people)?

So having metrics that helps answer the above questions would be useful in contributing to keeping the momentum healthy I think?

youknowriad commented 5 years ago

@danielbachhuber

Maybe these metrics?

How many different reviewers do we have (for all merged PRs)? (not the number of reviewers per PR, but a way to find if we onboarded different reviewers)
Number of PRs reviewed per reviewer.
Time between a PR is opened and first review
Time between a PR is opened and merged/closed

chrisvanpatten commented 5 years ago

✅ I'm happy to review anything to do with UI Components and Docs!

Soean commented 5 years ago

✅ I'm happy to review everything with blocks.

jasmussen commented 5 years ago

✅ I'm happy to review anything you'd like my opinion on, usually around high level design.

youknowriad commented 5 years ago

@jasmussen Thanks :) Just noting that these don't replace the existing "Needs Design Review" / "Needs Accessibility Feedback" ... which I think work pretty well. This is more about knowing who to ping in case an area of the code is touched.

ajitbohra commented 5 years ago

Have been trying to do the review for blocks, UI Components, docs

Recently I have started reading PR's around tooling would love to test and try to chip in.

ellatrix commented 5 years ago

I'm happy to review anything for the RichText component, rich-text and format-library packages, changes to paste logic, and more generally writing flow things.

ellatrix commented 5 years ago

Not sure why format-library is part of the blocks area.

danielbachhuber commented 5 years ago

@youknowriad @nerrad Good suggestions. I've created #13492 for further conversation as to not derail this one.

nerrad commented 5 years ago

Not sure why format-library is part of the blocks area.

@iseulde do you think it should be its own area (formatting maybe?) or should it be in a different area?

ellatrix commented 5 years ago

I'm not sure to be honest. There could be an area Rich Text or something, but maybe that's too specific.

oandregal commented 5 years ago

I'd like to help in whatever capacity I can. In my few months in Gutenberg, I've helped in areas that were understaffed and needed fixing instead of focusing on a particular sub-system. Do ping me in any part of the code you see I've contributed to. I guess this pattern resonates with a lot of the other >350 contributors (the contributors pareto law!), so perhaps by setting up automatic review requests for small things (instead of broad areas) people may feel more up to the challenge and reviews will be distributed out of the core gutenberg group. Here are some paths that I remember having contributed to:

packages/components/src/color-picker
packages/components/src/draggable
packages/components/src/drop-zone
packages/components/src/slot-fill
packages/editor/src/components/block-draggable
packages/editor/src/components/post-publish-button
packages/editor/src/components/post-publish-panel
docs/designers-developers/developers/tutorials

ntwb commented 5 years ago

✅ I'm happy to review everything with tooling.

jaymanpandya commented 5 years ago

I will be able to work on anything in UI Components

aduth commented 5 years ago

An initial CODEOWNERS file proposal can be found at #13604

youknowriad commented 5 years ago

I'm going to close this issue for now as we have an initial codeowners/reviewers implementation in place. Let's evaluate in some weeks and consider improvements.

talldan commented 5 years ago

Some feedback from my point of view.

It feels like we have two separate groups that might be interested in a pull request:

Those who are considered owners, and always need to be notified about any proposed changes to a particular part of the codebase.
Those who can review pull requests for specific parts of the codebase

Github's code owners feature seems to be designed for the first group but not the second, as the name implies.

An example recently is that I had a very minimal pull request (a file rename) and there were ~10 reviewers added automatically to the PR, which I found overkill and creates a lot of notification noise.

I feel like the perfect system would be that when a pull request is created:

All codeowners for matched paths are added.
A random selection of reviewers (maybe up to a maximum of 3 or 4) are also added (again, this could be based on matching paths).

I know that GH doesn't support this idea out of the box, we'd probably need a bot to handle something like this ... perhaps an idea for the automation project?https://github.com/WordPress/gutenberg/projects/24

WordPress / gutenberg