From c0b8a745e87fdc5db994392d5c1396768cdd28a1 Mon Sep 17 00:00:00 2001 From: Boxy Date: Sun, 15 Jan 2023 18:36:49 +0000 Subject: [PATCH 1/4] a --- CONTRIBUTING.md | 63 ++++++++++++++++++++++++++++++++----------------- 1 file changed, 42 insertions(+), 21 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index cb36131fd1cdc..8de0206cfc414 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -76,37 +76,58 @@ 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: +- How controversial are the design decisions +- How complex is the implementation -PRs are trivial if there is no reasonable argument against them. This might include: -* 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 +the PR should either go through an RFC (although in some cases small enough decisions can get by without). +PRs that are non trivial to review are given the [`D-Complex`] label. This indicates that the PR +should be reviewed more thoroughly and by people with experience in the area that the PR touches. -PRs are controversial if there is serious design discussion required, or a large impact to contributors or users. Factors that increase controversy include: +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. 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) -Finally, changes are "relatively uncontroversial" if they are neither trivial or controversial. -Most PRs should fall into this category. +Examples of PRs that are both [`S-Controversial`] and [`D-Complex`]: +* [bevy_reflect: Binary formats](https://github.com/bevyengine/bevy/pull/6140) -Here are some useful pull request queries: +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 From 02c60029c1115b7c94a6edb8c917825a4a39886f Mon Sep 17 00:00:00 2001 From: Boxy Date: Sun, 15 Jan 2023 18:48:11 +0000 Subject: [PATCH 2/4] CI --- CONTRIBUTING.md | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 8de0206cfc414..f91b9abfbbcd7 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -77,19 +77,20 @@ Check out our dedicated [Bevy Organization document](/docs/the_bevy_organization ### Classifying PRs Our merge strategy relies on the classification of PRs on two axes: -- How controversial are the design decisions -- How complex is the implementation - +* How controversial are the design decisions +* How complex is the implementation PRs with non trivial design decisions are given the [`S-Controversial`] label. This indicates that -the PR should either go through an RFC (although in some cases small enough decisions can get by without). -PRs that are non trivial to review are given the [`D-Complex`] label. This indicates that the PR +the PR should either go through an RFC (although in some cases small enough decisions can get by without). + +PRs that are non trivial to review are given the [`D-Complex`] label. This indicates that the PR 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. 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. Serious tradeoffs were made. @@ -100,11 +101,13 @@ Some things that are reason to apply the [`S-Controversial`] label to a PR: 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. @@ -112,17 +115,21 @@ Examples of PRs that are not [`S-Controversial`] or [`D-Complex`]: * [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) 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) From c326c391ec9dd0c2f5d4be050c54c6df65407fcf Mon Sep 17 00:00:00 2001 From: Boxy Date: Sun, 15 Jan 2023 18:59:04 +0000 Subject: [PATCH 3/4] a --- CONTRIBUTING.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index f91b9abfbbcd7..d717e6095c6d3 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -77,6 +77,7 @@ Check out our dedicated [Bevy Organization document](/docs/the_bevy_organization ### Classifying PRs Our merge strategy relies on the classification of PRs on two axes: + * How controversial are the design decisions * How complex is the implementation From bbce5590dd1aada28c1b4e98e08b0e6096719c17 Mon Sep 17 00:00:00 2001 From: Boxy Date: Sun, 15 Jan 2023 19:26:44 +0000 Subject: [PATCH 4/4] Apply suggestions from code review Co-authored-by: James Liu --- CONTRIBUTING.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index d717e6095c6d3..d7d94ca9825d6 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -81,10 +81,10 @@ Our merge strategy relies on the classification of PRs on two axes: * How controversial are the design decisions * How complex is the implementation -PRs with non trivial design decisions are given the [`S-Controversial`] label. This indicates that -the PR should either go through an RFC (although in some cases small enough decisions can get by without). +PRs with non-trivial design decisions are given the [`S-Controversial`] label. This indicates that +the PR needs more thorough design review or an [RFC](https://github.com/bevyengine/rfcs), if complex enough. -PRs that are non trivial to review are given the [`D-Complex`] label. This indicates that the PR +PRs that are non-trivial to review are given the [`D-Complex`] label. This indicates that the PR 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.