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

Add guidance on closing PRs #13160

Merged
merged 5 commits into from
May 2, 2024
Merged

Add guidance on closing PRs #13160

merged 5 commits into from
May 2, 2024

Conversation

alice-i-cecile
Copy link
Member

Objective

  • Bevy has a large number of open PRs in the backlog.
  • When there are problems with these PRs, contributors, maintainers and SMEs are all often left wondering what to do with them and are reluctant to close them fully.
  • Instead, PRs that are a bad idea end up sititng with Controversial status forever, while severely rotten PRs have Adopt-Me on them when it would be more appropriate to simply remake them.

Solution

  • Add clear guidance on how and when to close PRs.

@alice-i-cecile alice-i-cecile added A-Meta About the project itself X-Controversial There is active debate or serious implications around merging this PR labels May 1, 2024
jgayfer
jgayfer approved these changes May 1, 2024
View reviewed changes
Copy link
Contributor

@jgayfer jgayfer left a comment

Choose a reason for hiding this comment

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

+1 on more readily closing PRs where appropriate

@NthTensor
Copy link
Contributor

Looks great! Two questions:

  • Should adopt-me PRs be left closed or open?
  • I've closed three or four issues (and I think a PR) unilaterally by posting a comment saying something like "this looks like it should be closed, I will close after 7 days if no one objects" and then doing that when no one says anything. Some guidance to org-members as to if that's ok or not would be appreciated.

@alice-i-cecile alice-i-cecile added the C-Docs An addition or correction to our documentation label May 1, 2024
@alice-i-cecile alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label May 1, 2024
NiseVoid
NiseVoid approved these changes May 1, 2024
View reviewed changes
CONTRIBUTING.md Outdated
3. The work is particularly low quality, and the author is resistant to coaching.
4. The work adds features or abstraction of limited value, especially in a way that could easily be recreated outside of the engine.
5. The work has been sitting in review for so long and accumulated so many conflicts that it would be simpler to redo it from scratch.
6. The PR is pointlessly large, and should be broken into multiple smaller PRs for easier review.
Copy link
Contributor

Choose a reason for hiding this comment

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

An edge case would be large PRs that are left open while the small PRs are made, but I guess we should probably also just close them. Closed PRs can still be linked to from the other smaller PRs

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I think that's generally the way.

BD103
BD103 approved these changes May 2, 2024
View reviewed changes
CONTRIBUTING.md Outdated Show resolved Hide resolved
@alice-i-cecile @BD103
Nit on slashes
72730b5 
Co-authored-by: BD103 <59022059+BD103@users.noreply.github.com>
@alice-i-cecile
Copy link
Member Author

Merging following a blessing from @cart here.

@alice-i-cecile alice-i-cecile added this pull request to the merge queue May 2, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks May 2, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue May 2, 2024
Merged via the queue into bevyengine:main with commit 2719562 May 2, 2024
27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@NiseVoid NiseVoid NiseVoid approved these changes

@SolarLiner SolarLiner SolarLiner approved these changes

@jgayfer jgayfer jgayfer approved these changes

@NthTensor NthTensor NthTensor approved these changes

@MalekiRe MalekiRe MalekiRe approved these changes

@BD103 BD103 BD103 approved these changes

@superdump superdump Awaiting requested review from superdump

@cart cart Awaiting requested review from cart

@james7132 james7132 Awaiting requested review from james7132

@mockersf mockersf Awaiting requested review from mockersf

Assignees
No one assigned
Labels
A-Meta About the project itself C-Docs An addition or correction to our documentation S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it X-Controversial There is active debate or serious implications around merging this PR
Projects
Alice's Work Planning
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@alice-i-cecile @NthTensor @NiseVoid @SolarLiner @jgayfer @MalekiRe @BD103