Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Review policy changes #790

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

Conversation

apiraino
Copy link
Contributor

@apiraino apiraino commented Dec 10, 2024

Follow-up to #553 for a second round of comments/fixes/improvements.

I applied a few minor cosmetic changes and some wording (feel free to improve how it flows)

@rustbot
Copy link
Collaborator

rustbot commented Dec 10, 2024

r? @workingjubilee

rustbot has assigned @workingjubilee.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 10, 2024
@apiraino apiraino marked this pull request as draft December 10, 2024 15:51
Copy link
Member

@jieyouxu jieyouxu left a comment

Choose a reason for hiding this comment

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

Thanks! I left some minor wording feedback.

Comment on lines 157 to +158
- If the change is intended for another team, roll a reviewer from the relevant
team.
team (with `r? @rust-lang/<team-name>`)
Copy link
Member

Choose a reason for hiding this comment

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

Suggestion: probably not this form, because if you are a project member who has ping permissions it will ping the entire team. This should probably still use the triagebot adhoc_groups names I think.

src/compiler/reviews.md Outdated Show resolved Hide resolved
Comment on lines +303 to +305
complicated. It is expected from contributors to split their work as much as
(reasonably) possible. Smaller, ideally self-contained bites make a review
feasable. In general, before submitting large impact changes, it is expected the
Copy link
Member

Choose a reason for hiding this comment

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

Suggestion: slightly adjust the wording here, maybe something like "a patch series of more digestible PRs which are individually more logically self-contained".

My thinking here is that micro PRs for the sake of small PRs isn't always good either. Could also recommend PR author discuss how to split the large change with the team as well. Sometimes it might take different reviewers who are specialized/experts in different areas to review a subset of individual PRs in the patch series.

Co-authored-by: 许杰友 Jieyou Xu (Joe) <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants