Update review policy

[michaelwoerister]

Discussing this was proposed in: rust-lang/compiler-team#444. The actual discussion took place on zulip.

@rust-lang/compiler-contributors: Please leave comments about anything that you'd still like to have discussed.

[michaelwoerister]

I think it'd be good to add some section on "not assigned reviews" and best practices there -- both in terms of how to indicate you aren't the final reviewer to the author, but also whether e.g. it's acceptable to r- someone's PR asking them to cleanup commit history / fix the description / etc.

@Mark-Simulacrum, where would you like this to be added? Do you want to draft the concrete text?

[michaelwoerister]

I voted :writing: on this because I feel like if you're not feeling comfotable with reviewing a PR, it is unlikely you'll be able to accurately gauge the scope of the PR and/or its contentiousness.

@nagisa, do you want to elaborate on that?

[michaelwoerister]

@pnkfelix, you signaled that you wanted to discuss the wording of this paragraph.

[LeSeulArtichaut]

(rust-lang/highfive#350 has been merged)

[davidtwco]

Little late to the discussion here, updated review policy looks good to me, but wanted to suggest another edge case that we should document in our review policy: a pull request which makes changes to support the use of rustc internals for some external project - these aren't common but I've seen it happen a few times - do we accept these changes if they're small and otherwise improvements, do we reject these unless it's a mature and released project, etc?

[oli-obk]

I think we should allow changes making things public, cleaning up things or making them more general, as long as the owners of the compiler region agree (so just assign to them). As a concrete example: if someone is using the mir interpreter, and they want to make something public, it is likely not a problem, but there are some functions that are module- or crate-private on purpose, as they uphold invariants within the mir interpreter. So basically, just assign such PRs to the relevant people (usually they get pinged anyway due to having told highfive that they want to get pinged on changes to these parts)

[ehuss]

@michaelwoerister I know it's been a while, are you still interested in working on this? I'm not clear if this was waiting for more updates or reviews, or if it is ready to go as-is.

[michaelwoerister]

Oh my, that was never merged, was it? Sorry for not following up!

I'll look into getting this over the finish line.

[michaelwoerister]

@wesleywiser & @davidtwco, I think the outstanding questions have been addressed (see the final two commits). I don't think there are any major changes compared to when the document was discussed originally, so I think we could merge the PR in the current state.

I did not add the point @davidtwco made in #553 (comment). I'd be happy to if I can get some help with what the actual text should look like.