Skip to content
This repository has been archived by the owner on Mar 14, 2023. It is now read-only.

rust-lang/rust: Compiler review assignments based on the expert map #266

Closed
wants to merge 2 commits into from

Conversation

petrochenkov
Copy link
Contributor

@petrochenkov petrochenkov commented Jun 12, 2020

Zulip discussion: https://rust-lang.zulipchat.com/#narrow/stream/131828-t-compiler/topic/Compiler.20review.20assignments.20based.20on.20the.20expert.20map

This PR fills the review assignment map using the rustc expert map, PR histories of the mentioned people as well as my personal observations.

Reviewers are now assigned using specific rustc crates rather than single rustc* wildcard.
Much larger number of people from the compiler and compiler-contributors groups can now be assigned, instead of the same 7-9 people for the whole compiler.

Pros:

  • Reducing review load from those 7-9 people who were assigned previously.
  • More people are involved into the review work and get reviewing experience.
  • Better match between the reviewer and reviewed code, higher quality reviews without reassignments.

Cons:

  • Harder to keep the map up-to-date.
  • Less chance for a reviewer to learn something new by being assigned to an unfamiliar part of the compiler.

Future work:

  • Need an exclude list to temporarily exclude busy people from reviewers without breaking the expert map.
  • Merging all this with the expert map in the compiler team repo and storing this data somewhere in the team repo rather than in highfive (needs involvement from the infra team).

r? @nikomatsakis

I'll ping the other people mentioned in the PR once some decision about doing or not doing this is reached.
I'm pretty sure some people will want to tweak their assignments.

Copy link
Member

@davidtwco davidtwco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some thoughts: In general, I like this idea. However, I'm definitely interested in being assigned to areas of the compiler that I'm not familiar with to learn and help spread the workload (I can always re-assign when I'm not able to meaningfully review) - and this approach leaves some areas with only one or two reviewers, which could leave PRs unreviewed if those folks are busy/unavailable.

@nikomatsakis
Copy link
Contributor

I'm of a few minds. For one thing, I'm worried about the lists getting out of date -- I also think it's great for folks to get some chance to review a broader set of things.

On the other hand, I think that having fewer reviews from random bits of the code, and more focused reviews on areas of interest to a specific person, will likely help.

This seems pretty related to the "areas of the compiler" proposal from rust-lang/compiler-team#288 as well -- I had imagined there having directories assigned to areas, and areas populated with people, probably at three levels: "lead", "member", and "notify", where leads/member is probably the folks who would do most of the reviewing. (Sometimes, though, areas are kind of "cross-cutting" across directories.) But "notify" people might be folks who'd like a ping.

@petrochenkov
Copy link
Contributor Author

One alternative here that would allow to achieve pros 1 and 2 (but not 3), and wouldn't have the listed drawbacks, is to assign reviews to all people from the compiler and compiler-contributors groups actively involved in rustc development now.
That's 10 + 10-15 more people.

Anyway, I collected the data which will stay if this PR is closed, and don't have time to pursue this actively, so I'm going to close it.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants