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

Add advice about submitting potentially hard-to-review PRs #2036

Merged
merged 1 commit into from
Sep 23, 2024

Conversation

jieyouxu
Copy link
Member

@jieyouxu jieyouxu commented Aug 5, 2024

Prior discussion: https://rust-lang.zulipchat.com/#narrow/stream/131828-t-compiler/topic/Expert.20map.20and.20suitable.20reviewer.20maps.3F/near/456305821.

Some context (partial summary of the prior discussion): I've noticed that we have had quite a few cases of the spin-the-bottle or pass-the-hot-potato review assignment reroll game (at least for the compiler reviewers) because some PRs might be very hard to review (due to being massive/complex/cross-cutting/highly domain specific etc.).

So in this PR, we add some advice for contributors about making potentially hard-to-review PRs (those that are large, complex, cross-cutting and/or highly domain-specific) to:

  • Talk with t-compiler before submitting a very difficult to review PR
  • Work with t-compiler to break down a large unreviewable PR into smaller, more reviewable PRs

cc @wesleywiser and @davidtwco compiler team leads for a vibe check and wording feedback, and if we want to make this advice at all.

@jieyouxu
Copy link
Member Author

jieyouxu commented Aug 5, 2024

(Seems like PR CI has a bunch of pre-existing link check errors)

src/contributing.md Outdated Show resolved Hide resolved
> large, complex, cross-cutting and/or highly domain-specific, it becomes very difficult to find a
> suitable reviewer who is comfortable in reviewing all of the changes in such a PR. This is also
> true if the changes are not only compiler-specific but also contains changes which fall under the
> purview of reviewers from other teams, like the standard library team.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion (but not important): "In this case, a bot will automatically ping relevant groups based on the code that the patch is touching."

(or something along this line)

Copy link
Member Author

Choose a reason for hiding this comment

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

Added basic info about triagebot notifying relevant teams and people who registered specific alerts. I was almost going to write a sentence to the effect of "if you see a massive list of pinged people, the PR is probably too large" but that's already after a person spent a ton of time and effort, so I decided against that.

Copy link
Contributor

@apiraino apiraino left a comment

Choose a reason for hiding this comment

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

I really like the wording here! If it's not explicited elsewhere, it's an important addition to our developer documentation - it suggests common good practices when collaborating on / contributing to a FOSS project)

Copy link
Member

@compiler-errors compiler-errors left a comment

Choose a reason for hiding this comment

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

💞

@jieyouxu
Copy link
Member Author

(Contains no changes, just rebased to overcome CI failure)

@jieyouxu jieyouxu merged commit 0f7f7df into rust-lang:master Sep 23, 2024
1 check passed
@jieyouxu jieyouxu deleted the compiler-large-pr-advice branch September 23, 2024 08:46
compiler-errors added a commit to compiler-errors/rust that referenced this pull request Sep 24, 2024
Update books

## rust-lang/book

5 commits in e7d217be2a75ef1753f0988d6ccaba4d7e376259..99cf75a5414fa8adbe3974bd0836661ca901708f
2024-09-23 16:18:39 UTC to 2024-09-11 18:38:03 UTC

- translations: remove broken link (rust-lang/book#4036)
- Update build instructions: include mdbook plugins (rust-lang/book#4032)
- Add `cargo init` usage suggestion to 1.3 (rust-lang/book#4025)
- Use immutable borrow of `TcpStream` when creating `BufReader` (rust-lang/book#4024)
- Upgrade to Rust 1.81 (rust-lang/book#4031)

## rust-lang/edition-guide

1 commits in b3ca7ade0f87d7e3fb538776defc5b2cc4188172..c7ebae25cb4801a31b6f05353f6d85bfa6feedd1
2024-09-22 08:47:02 UTC to 2024-09-22 08:47:02 UTC

- Update static_mut_refs now that it is a lint (rust-lang/edition-guide#322)

## rust-lang/reference

10 commits in 687faf9958c52116d003b41dfd29cc1cf44f5311..24fb2687cdbc54fa18ae4acf5d879cfceca77b2c
2024-09-22 09:07:12 UTC to 2024-09-10 19:24:17 UTC

- do not talk about the 'address of a constant' (rust-lang/reference#1624)
- Add spec identifier syntax to interior-mutability.md (rust-lang/reference#1585)
- Add spec identifier syntax to input-format.md (rust-lang/reference#1584)
- Document limitations on block doc comments (rust-lang/reference#1602)
- Places based on misaligned pointers: also mention 'static's as a possible base for place projections (rust-lang/reference#1606)
- type-layout: mention that call ABI compatibility is a separate concern (rust-lang/reference#1608)
- stabilize `const_extern_fn` (rust-lang/reference#1596)
- const_eval: update for const_mut_refs and const_refs_to_cell stabilization (rust-lang/reference#1590)
- fix: unclosed tag `sup` (rust-lang/reference#1612)
- Fix improper documentation on casting non_exhaustive enums (rust-lang/reference#1607)

## rust-lang/rustc-dev-guide

13 commits in 0ed9229f5b6f7824b333beabd7e3d5ba4b9bd971..555f3de2fa0d61c4294b74d245f1cbad6fcbf589
2024-09-23 12:51:33 UTC to 2024-09-10 07:32:10 UTC

- chore: add missing `.` (rust-lang/rustc-dev-guide#2074)
- Add remark on required free disk space (rust-lang/rustc-dev-guide#2073)
- fix broken links (rust-lang/rustc-dev-guide#2063)
- Add advice about submitting potentially hard-to-review PRs (rust-lang/rustc-dev-guide#2036)
- Edit a sentence for clarity (rust-lang/rustc-dev-guide#2071)
- Emphasize how to run a single tool test (rust-lang/rustc-dev-guide#2070)
- Remove chalk is owned by WG-traits (rust-lang/rustc-dev-guide#2068)
- Fix conditions lowering text for enums with no fields (rust-lang/rustc-dev-guide#2066)
- update proof tree chapter (rust-lang/rustc-dev-guide#2054)
- Add docs for JS tests (rust-lang/rustc-dev-guide#2048)
- Reflect `x.py test`'s `--rustc-args` option being renamed to `--compiletest-rustc-args` (rust-lang/rustc-dev-guide#2062)
- we standardized on this more reliable command (rust-lang/rustc-dev-guide#2061)
- Fix Typo and Remove Outdated Line About C Variadics (rust-lang/rustc-dev-guide#2060)
compiler-errors added a commit to compiler-errors/rust that referenced this pull request Sep 24, 2024
Update books

## rust-lang/book

5 commits in e7d217be2a75ef1753f0988d6ccaba4d7e376259..99cf75a5414fa8adbe3974bd0836661ca901708f
2024-09-23 16:18:39 UTC to 2024-09-11 18:38:03 UTC

- translations: remove broken link (rust-lang/book#4036)
- Update build instructions: include mdbook plugins (rust-lang/book#4032)
- Add `cargo init` usage suggestion to 1.3 (rust-lang/book#4025)
- Use immutable borrow of `TcpStream` when creating `BufReader` (rust-lang/book#4024)
- Upgrade to Rust 1.81 (rust-lang/book#4031)

## rust-lang/edition-guide

1 commits in b3ca7ade0f87d7e3fb538776defc5b2cc4188172..c7ebae25cb4801a31b6f05353f6d85bfa6feedd1
2024-09-22 08:47:02 UTC to 2024-09-22 08:47:02 UTC

- Update static_mut_refs now that it is a lint (rust-lang/edition-guide#322)

## rust-lang/reference

10 commits in 687faf9958c52116d003b41dfd29cc1cf44f5311..24fb2687cdbc54fa18ae4acf5d879cfceca77b2c
2024-09-22 09:07:12 UTC to 2024-09-10 19:24:17 UTC

- do not talk about the 'address of a constant' (rust-lang/reference#1624)
- Add spec identifier syntax to interior-mutability.md (rust-lang/reference#1585)
- Add spec identifier syntax to input-format.md (rust-lang/reference#1584)
- Document limitations on block doc comments (rust-lang/reference#1602)
- Places based on misaligned pointers: also mention 'static's as a possible base for place projections (rust-lang/reference#1606)
- type-layout: mention that call ABI compatibility is a separate concern (rust-lang/reference#1608)
- stabilize `const_extern_fn` (rust-lang/reference#1596)
- const_eval: update for const_mut_refs and const_refs_to_cell stabilization (rust-lang/reference#1590)
- fix: unclosed tag `sup` (rust-lang/reference#1612)
- Fix improper documentation on casting non_exhaustive enums (rust-lang/reference#1607)

## rust-lang/rustc-dev-guide

13 commits in 0ed9229f5b6f7824b333beabd7e3d5ba4b9bd971..555f3de2fa0d61c4294b74d245f1cbad6fcbf589
2024-09-23 12:51:33 UTC to 2024-09-10 07:32:10 UTC

- chore: add missing `.` (rust-lang/rustc-dev-guide#2074)
- Add remark on required free disk space (rust-lang/rustc-dev-guide#2073)
- fix broken links (rust-lang/rustc-dev-guide#2063)
- Add advice about submitting potentially hard-to-review PRs (rust-lang/rustc-dev-guide#2036)
- Edit a sentence for clarity (rust-lang/rustc-dev-guide#2071)
- Emphasize how to run a single tool test (rust-lang/rustc-dev-guide#2070)
- Remove chalk is owned by WG-traits (rust-lang/rustc-dev-guide#2068)
- Fix conditions lowering text for enums with no fields (rust-lang/rustc-dev-guide#2066)
- update proof tree chapter (rust-lang/rustc-dev-guide#2054)
- Add docs for JS tests (rust-lang/rustc-dev-guide#2048)
- Reflect `x.py test`'s `--rustc-args` option being renamed to `--compiletest-rustc-args` (rust-lang/rustc-dev-guide#2062)
- we standardized on this more reliable command (rust-lang/rustc-dev-guide#2061)
- Fix Typo and Remove Outdated Line About C Variadics (rust-lang/rustc-dev-guide#2060)
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Sep 24, 2024
Rollup merge of rust-lang#130759 - rustbot:docs-update, r=ehuss

Update books

## rust-lang/book

5 commits in e7d217be2a75ef1753f0988d6ccaba4d7e376259..99cf75a5414fa8adbe3974bd0836661ca901708f
2024-09-23 16:18:39 UTC to 2024-09-11 18:38:03 UTC

- translations: remove broken link (rust-lang/book#4036)
- Update build instructions: include mdbook plugins (rust-lang/book#4032)
- Add `cargo init` usage suggestion to 1.3 (rust-lang/book#4025)
- Use immutable borrow of `TcpStream` when creating `BufReader` (rust-lang/book#4024)
- Upgrade to Rust 1.81 (rust-lang/book#4031)

## rust-lang/edition-guide

1 commits in b3ca7ade0f87d7e3fb538776defc5b2cc4188172..c7ebae25cb4801a31b6f05353f6d85bfa6feedd1
2024-09-22 08:47:02 UTC to 2024-09-22 08:47:02 UTC

- Update static_mut_refs now that it is a lint (rust-lang/edition-guide#322)

## rust-lang/reference

10 commits in 687faf9958c52116d003b41dfd29cc1cf44f5311..24fb2687cdbc54fa18ae4acf5d879cfceca77b2c
2024-09-22 09:07:12 UTC to 2024-09-10 19:24:17 UTC

- do not talk about the 'address of a constant' (rust-lang/reference#1624)
- Add spec identifier syntax to interior-mutability.md (rust-lang/reference#1585)
- Add spec identifier syntax to input-format.md (rust-lang/reference#1584)
- Document limitations on block doc comments (rust-lang/reference#1602)
- Places based on misaligned pointers: also mention 'static's as a possible base for place projections (rust-lang/reference#1606)
- type-layout: mention that call ABI compatibility is a separate concern (rust-lang/reference#1608)
- stabilize `const_extern_fn` (rust-lang/reference#1596)
- const_eval: update for const_mut_refs and const_refs_to_cell stabilization (rust-lang/reference#1590)
- fix: unclosed tag `sup` (rust-lang/reference#1612)
- Fix improper documentation on casting non_exhaustive enums (rust-lang/reference#1607)

## rust-lang/rustc-dev-guide

13 commits in 0ed9229f5b6f7824b333beabd7e3d5ba4b9bd971..555f3de2fa0d61c4294b74d245f1cbad6fcbf589
2024-09-23 12:51:33 UTC to 2024-09-10 07:32:10 UTC

- chore: add missing `.` (rust-lang/rustc-dev-guide#2074)
- Add remark on required free disk space (rust-lang/rustc-dev-guide#2073)
- fix broken links (rust-lang/rustc-dev-guide#2063)
- Add advice about submitting potentially hard-to-review PRs (rust-lang/rustc-dev-guide#2036)
- Edit a sentence for clarity (rust-lang/rustc-dev-guide#2071)
- Emphasize how to run a single tool test (rust-lang/rustc-dev-guide#2070)
- Remove chalk is owned by WG-traits (rust-lang/rustc-dev-guide#2068)
- Fix conditions lowering text for enums with no fields (rust-lang/rustc-dev-guide#2066)
- update proof tree chapter (rust-lang/rustc-dev-guide#2054)
- Add docs for JS tests (rust-lang/rustc-dev-guide#2048)
- Reflect `x.py test`'s `--rustc-args` option being renamed to `--compiletest-rustc-args` (rust-lang/rustc-dev-guide#2062)
- we standardized on this more reliable command (rust-lang/rustc-dev-guide#2061)
- Fix Typo and Remove Outdated Line About C Variadics (rust-lang/rustc-dev-guide#2060)
github-actions bot pushed a commit to rust-lang/miri that referenced this pull request Sep 25, 2024
Update books

## rust-lang/book

5 commits in e7d217be2a75ef1753f0988d6ccaba4d7e376259..99cf75a5414fa8adbe3974bd0836661ca901708f
2024-09-23 16:18:39 UTC to 2024-09-11 18:38:03 UTC

- translations: remove broken link (rust-lang/book#4036)
- Update build instructions: include mdbook plugins (rust-lang/book#4032)
- Add `cargo init` usage suggestion to 1.3 (rust-lang/book#4025)
- Use immutable borrow of `TcpStream` when creating `BufReader` (rust-lang/book#4024)
- Upgrade to Rust 1.81 (rust-lang/book#4031)

## rust-lang/edition-guide

1 commits in b3ca7ade0f87d7e3fb538776defc5b2cc4188172..c7ebae25cb4801a31b6f05353f6d85bfa6feedd1
2024-09-22 08:47:02 UTC to 2024-09-22 08:47:02 UTC

- Update static_mut_refs now that it is a lint (rust-lang/edition-guide#322)

## rust-lang/reference

10 commits in 687faf9958c52116d003b41dfd29cc1cf44f5311..24fb2687cdbc54fa18ae4acf5d879cfceca77b2c
2024-09-22 09:07:12 UTC to 2024-09-10 19:24:17 UTC

- do not talk about the 'address of a constant' (rust-lang/reference#1624)
- Add spec identifier syntax to interior-mutability.md (rust-lang/reference#1585)
- Add spec identifier syntax to input-format.md (rust-lang/reference#1584)
- Document limitations on block doc comments (rust-lang/reference#1602)
- Places based on misaligned pointers: also mention 'static's as a possible base for place projections (rust-lang/reference#1606)
- type-layout: mention that call ABI compatibility is a separate concern (rust-lang/reference#1608)
- stabilize `const_extern_fn` (rust-lang/reference#1596)
- const_eval: update for const_mut_refs and const_refs_to_cell stabilization (rust-lang/reference#1590)
- fix: unclosed tag `sup` (rust-lang/reference#1612)
- Fix improper documentation on casting non_exhaustive enums (rust-lang/reference#1607)

## rust-lang/rustc-dev-guide

13 commits in 0ed9229f5b6f7824b333beabd7e3d5ba4b9bd971..555f3de2fa0d61c4294b74d245f1cbad6fcbf589
2024-09-23 12:51:33 UTC to 2024-09-10 07:32:10 UTC

- chore: add missing `.` (rust-lang/rustc-dev-guide#2074)
- Add remark on required free disk space (rust-lang/rustc-dev-guide#2073)
- fix broken links (rust-lang/rustc-dev-guide#2063)
- Add advice about submitting potentially hard-to-review PRs (rust-lang/rustc-dev-guide#2036)
- Edit a sentence for clarity (rust-lang/rustc-dev-guide#2071)
- Emphasize how to run a single tool test (rust-lang/rustc-dev-guide#2070)
- Remove chalk is owned by WG-traits (rust-lang/rustc-dev-guide#2068)
- Fix conditions lowering text for enums with no fields (rust-lang/rustc-dev-guide#2066)
- update proof tree chapter (rust-lang/rustc-dev-guide#2054)
- Add docs for JS tests (rust-lang/rustc-dev-guide#2048)
- Reflect `x.py test`'s `--rustc-args` option being renamed to `--compiletest-rustc-args` (rust-lang/rustc-dev-guide#2062)
- we standardized on this more reliable command (rust-lang/rustc-dev-guide#2061)
- Fix Typo and Remove Outdated Line About C Variadics (rust-lang/rustc-dev-guide#2060)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants