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.

Sign up for GitHub

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

Jump to bottom

[Merged by Bors] - Update "Classifying PRs" section to talk about D-Complex #7216

Closed
wants to merge 4 commits into from
Closed

[Merged by Bors] - Update "Classifying PRs" section to talk about D-Complex #7216

Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c0b8a74
a
BoxyUwU Jan 15, 2023
02c6002
CI
BoxyUwU Jan 15, 2023
c326c39
a
BoxyUwU Jan 15, 2023
bbce559
Apply suggestions from code review
BoxyUwU Jan 15, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
71 changes: 50 additions & 21 deletions CONTRIBUTING.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,37 +76,66 @@ Check out our dedicated [Bevy Organization document](/docs/the_bevy_organization

### Classifying PRs

Our merge strategy relies on the classification of PRs into three categories: **trivial**, **uncontroversial** and **controversial**.
When making PRs, try to split out more controversial changes from less controversial ones, in order to make your work easier to review and merge.
PRs that are deemed controversial will receive the [`S-Controversial`](https://github.com/bevyengine/bevy/pulls?q=is%3Aopen+is%3Apr+label%3AS-Controversial) label, and will have to go through the more thorough review process.
Our merge strategy relies on the classification of PRs on two axes:

PRs are trivial if there is no reasonable argument against them. This might include:
* How controversial are the design decisions
* How complex is the implementation

* Fixing dead links.
* Removing dead code or dependencies.
* Typo and grammar fixes.
PRs with non trivial design decisions are given the [`S-Controversial`] label. This indicates that
BoxyUwU marked this conversation as resolved.
Show resolved Hide resolved
the PR should either go through an RFC (although in some cases small enough decisions can get by without).
BoxyUwU marked this conversation as resolved.
Show resolved Hide resolved

PRs are controversial if there is serious design discussion required, or a large impact to contributors or users. Factors that increase controversy include:
PRs that are non trivial to review are given the [`D-Complex`] label. This indicates that the PR
BoxyUwU marked this conversation as resolved.
Show resolved Hide resolved
should be reviewed more thoroughly and by people with experience in the area that the PR touches.

When making PRs, try to split out more controversial changes from less controversial ones, in order to make your work easier to review and merge.
It is also a good idea to try and split out simple changes from more complex changes if it is not helpful for then to be reviewed together.

1. Changes to project-wide workflow or style.
Some things that are reason to apply the [`S-Controversial`] label to a PR:

1. Changes to a project-wide workflow or style.
2. New architecture for a large feature.
3. PRs where a serious tradeoff must be made.
3. Serious tradeoffs were made.
4. Heavy user impact.
Copy link
Member

Choose a reason for hiding this comment

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

Is adding a lot of docs like in #6649 considered "Heavy user impact"? Initially I thought it would be just about changing the public interface too much, so maybe it's not clear.

Copy link
Member

Choose a reason for hiding this comment

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

For reference, that PR was mislabeled (on the basis of its current contents), which has since been fixed.

Very high impact docs changes could be controversial, but I would expect that to be an extreme exception.

5. New ways for users to make mistakes (footguns).
6. Introductions of `unsafe` code.
7. Large-scale code reorganization.
8. High levels of technical complexity.
9. Adding a dependency.
10. Touching licensing information (due to the level of precision required).
11. Adding root-level files (due to the high level of visibility).
6. Adding a dependency
7. Touching licensing information (due to level of precision required).
8. Adding root-level files (due to the high level of visibility)

Some things that are reason to apply the [`D-Complex`] label to a PR:

1. Introduction or modification of soundness relevent code (for example `unsafe` code)
2. High levels of technical complexity.
3. Large-scale code reorganization

Examples of PRs that are not [`S-Controversial`] or [`D-Complex`]:

* Fixing dead links.
* Removing dead code or unused dependencies.
* Typo and grammar fixes.
* [Add `Mut::reborrow`](https://github.com/bevyengine/bevy/pull/7114)
* [Add `Res::clone`](https://github.com/bevyengine/bevy/pull/4109)

Examples of PRs that are [`S-Controversial`] but not [`D-Complex`]:

* [Implement and require `#[derive(Component)]` on all component structs](https://github.com/bevyengine/bevy/pull/2254)
* [Use default serde impls for Entity](https://github.com/bevyengine/bevy/pull/6194)

Examples of PRs that are not [`S-Controversial`] but are [`D-Complex`]:

* [Ensure `Ptr`/`PtrMut`/`OwningPtr` are aligned in debug builds](https://github.com/bevyengine/bevy/pull/7117)
* [Replace `BlobVec`'s `swap_scratch` with a `swap_nonoverlapping`](https://github.com/bevyengine/bevy/pull/4853)

Examples of PRs that are both [`S-Controversial`] and [`D-Complex`]:

* [bevy_reflect: Binary formats](https://github.com/bevyengine/bevy/pull/6140)
Copy link
Member

Choose a reason for hiding this comment

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

I really like this expansion with concrete example PRs. Helps ground the entire section.


Finally, changes are "relatively uncontroversial" if they are neither trivial or controversial.
Most PRs should fall into this category.
Some useful pull request queries:

Here are some useful pull request queries:
* [PRs which need reviews and are not `D-Complex`](https://github.com/bevyengine/bevy/pulls?q=is%3Apr+-label%3AD-Complex+-label%3AS-Ready-For-Final-Review+-label%3AS-Blocked++)
* [`D-Complex` PRs which need reviews](https://github.com/bevyengine/bevy/pulls?q=is%3Apr+label%3AD-Complex+-label%3AS-Ready-For-Final-Review+-label%3AS-Blocked)

* [Uncontroversial pull requests which have been reviewed and are ready for maintainers to merge](https://github.com/bevyengine/bevy/pulls?q=is%3Aopen+is%3Apr+label%3AS-Ready-For-Final-Review+-label%3AS-Controversial+-label%3AS-Blocked+-label%3AS-Adopt-Me+)
* [Controversial pull requests which have been reviewed and are ready for final input from a Project Lead or SME](https://github.com/bevyengine/bevy/pulls?q=is%3Aopen+is%3Apr+label%3AS-Ready-For-Final-Review+label%3AS-Controversial+)
[`S-Controversial`]: https://github.com/bevyengine/bevy/pulls?q=is%3Aopen+is%3Apr+label%3AS-Controversial
[`D-Complex`]: https://github.com/bevyengine/bevy/pulls?q=is%3Aopen+is%3Apr+label%3AD-Complex

### Preparing Releases

Expand Down