[Merged by Bors] - Add "how to adopt pull requests" section #6895

oCaioOliveira · 2022-12-09T00:51:02Z

Objective

Add "how to adopt pull requests" section.
Fixes Add "how to adopt pull requests" section in Contributing.md #5539

Solution

Add "how to adopt pull requests" section in Contributing.md.

Fixes bevyengine#5539 Co-authored-by: Erick <[email protected]>

MrGVSV

I think this section should also include how to go about requesting permission in the original PR as well as conferring with org members to close the original when the new one is ready.

MrGVSV · 2022-12-09T01:16:37Z

CONTRIBUTING.md

@@ -275,6 +275,12 @@ By giving feedback on this work (and related supporting work), you can help us m
 Finally, if nothing brings you more satisfaction than seeing every last issue labeled and all resolved issues closed, feel free to message @cart for a Bevy org role to help us keep things tidy.
 As discussed in [*How we're organized*](#how-were-organized), this role only requires good faith and a basic understanding of our development process.

+### How to adopt pull requests
+
+When you want to contribute pull request with the label *S-Adopt-Me*, to preserve the credit of the original authors, you must reference the PR hash in the description.


You could probably add a link to the S-Adopt-Me label. That way, people reading this section can see which PRs are up for adoption.

james7132 · 2022-12-09T01:39:46Z

CONTRIBUTING.md

@@ -275,6 +275,12 @@ By giving feedback on this work (and related supporting work), you can help us m
 Finally, if nothing brings you more satisfaction than seeing every last issue labeled and all resolved issues closed, feel free to message @cart for a Bevy org role to help us keep things tidy.
 As discussed in [*How we're organized*](#how-were-organized), this role only requires good faith and a basic understanding of our development process.

+### How to adopt pull requests
+
+When you want to contribute pull request with the label *S-Adopt-Me*, to preserve the credit of the original authors, you must reference the PR hash in the description.


Preferably adopters should build off of the existing commits of the original author. This should mention pulling down the commits of the original PR as a base and rebasing into main to preserve the commit history and credit of the original author.

This is not always necessarily practical- the largest blocker to #5373 was a need for a rebase upon mainline, and squash-reduction to a single commit aided in that rebase significantly when creating #6853. If this is a preference, that's fine, but I'd avoid elevating it to a hard requirement.

This is not always necessarily practical- the largest blocker to #5373 was a need for a rebase upon mainline, and squash-reduction to a single commit aided in that rebase significantly when creating #6853. If this is a preference, that's fine, but I'd avoid elevating it to a hard requirement.

Hello @Dessix, did you comment in the correct PR? I didn't understand your comment...

Preferably adopters should build off of the existing commits of the original author.

@oCaioOliveira To rephrase my prior comment: "Reusing the original commits is sometimes hard".

Yep, IMO this should be noted as a preference, not a requirement.

oCaioOliveira · 2022-12-10T15:38:23Z

I think this section should also include how to go about requesting permission in the original PR as well as conferring with org members to close the original when the new one is ready.

Hello @MrGVSV, if the PR already has the S-Adopt-Me label, isn't it understood that the original PR will be used as a base? Why would it be necessary to ask for permission?

oCaioOliveira · 2022-12-10T15:53:49Z

Hello @james7132 and @MrGVSV, your suggestions were implemented, can you check?

CONTRIBUTING.md

alice-i-cecile · 2022-12-11T01:25:04Z

Hello @MrGVSV, if the PR already has the S-Adopt-Me label, isn't it understood that the original PR will be used as a base? Why would it be necessary to ask for permission?

I understood this request to add another section explaining the etiquette / practices around how that label gets added in the first place :)

MrGVSV · 2022-12-11T01:28:44Z

Hello @MrGVSV, if the PR already has the S-Adopt-Me label, isn't it understood that the original PR will be used as a base? Why would it be necessary to ask for permission?

I understood this request to add another section explaining the etiquette / practices around how that label gets added in the first place :)

Yes, sorry for being unclear before! This was what I meant since you may want to adopt an old PR without the label (or even just ask for permission to apply the label if you’re not planning on adopting it yourself).

alice-i-cecile · 2022-12-11T03:32:46Z

@oCaioOliveira feel free to resolve comments as you address them to help reviewers understand outstanding concerns.

Co-authored-by: James Liu <[email protected]>

oCaioOliveira · 2022-12-12T00:55:46Z

Hello @MrGVSV, we added a section explaining the etiquette / practices around how that label gets added.

MrGVSV

Looking a lot better! I think the second paragraph is pretty much good to go. Just a few comments on the surrounding paragraphs.

MrGVSV · 2022-12-13T07:24:57Z

CONTRIBUTING.md

@@ -275,6 +275,14 @@ By giving feedback on this work (and related supporting work), you can help us m
 Finally, if nothing brings you more satisfaction than seeing every last issue labeled and all resolved issues closed, feel free to message @cart for a Bevy org role to help us keep things tidy.
 As discussed in [*How we're organized*](#how-were-organized), this role only requires good faith and a basic understanding of our development process.

+### How to adopt pull requests
+
+Occasionally authors of pull requests get busy or become unresponsive. This is a natural part of any open source project. To avoid blocking these efforts, these pull requests may be *adopted*, where another contributor creates a new pull request with the same content. If there is an old Pull request without the label that is without updates, comment to the organization whether it is appropriate to add the *[S-Adopt-Me](https://github.com/bevyengine/bevy/labels/S-Adopt-Me)* label, to indicate that it can be *adopted*.


I think there's two things a person can do:

As you mentioned, they can ask an org member if a given PR could use the label (this generally happens on Discord from my observations but can also happen on the PR like you described)

If they want to pick it up themselves, they should likely leave a comment on the PR asking the author if they plan on returning to the PR and if they can adopt it otherwise. If the author gives permission to adopt or simply doesn't respond after some time (maybe a few days?), they may then continue to adopt the PR.

You pretty much have 1 written down here, but 2 is another thing users can do to adopt a PR (and this may sometimes even skip the labeling since at that point the PR has been adopted).

Sidenote: I know this file doesn't necessarily follow this style, but I’d recommend splitting these longer paragraphs over multiple lines (with no empty lines between). This allows markdown to still treat it as a single paragraph, but makes it easier for reviewers to leave comments on particular sentences or phrases— both for this PR and all future ones that modify these lines.

The 2 was resolved as you asked, can you check please?

We followed your suggestion of splitting these longer paragraphs over multiple lines, but the CI / markdownlint (pull_request) accused as errors. So we needed to maintain a paragraph.

Looks like the reason for the error was because of some spacing issues. It's telling you to remove trailing spaces on lines:

- Here is a sentence. + Here is a sentence. And here is another.

It also is suggested to add blank lines between lists and code blocks:

Here is some code: + ``` fn foo() {} ```

CONTRIBUTING.md

MrGVSV · 2022-12-13T07:32:10Z

CONTRIBUTING.md

+
+With this label added, it's best practice to fork the original author's branch. This ensures that they still get credit for working on it and that the commit history is retained. When the new pull request is ready, it should reference the original PR in the description. Then notify org members to close the original.
+
+* Example: `Adopted #number-original-pull-request`


Nit: Not super required, but sometimes people at-mention the original author in their PR description to further give back some credit to that author. Is this something we should add here? Or is it mainly a preference thing that users can decide for themselves (and so we can exclude it from this document)?

We put this part with examples just because in the description of issue (#5539) it asked for "correct way to "adopt a PR" that also preserves credit of the original authors". Do you think this part may be deleted?

Co-authored-by: Gino Valente <[email protected]>

Co-authored-by: Erick <[email protected]>

CONTRIBUTING.md

MrGVSV · 2022-12-19T00:01:31Z

CONTRIBUTING.md

@@ -275,6 +275,16 @@ By giving feedback on this work (and related supporting work), you can help us m
 Finally, if nothing brings you more satisfaction than seeing every last issue labeled and all resolved issues closed, feel free to message @cart for a Bevy org role to help us keep things tidy.
 As discussed in [*How we're organized*](#how-were-organized), this role only requires good faith and a basic understanding of our development process.

+### How to adopt pull requests
+
+Occasionally authors of pull requests get busy or become unresponsive. This is a natural part of any open source project. To avoid blocking these efforts, these pull requests may be *adopted*, where another contributor creates a new pull request with the same content. If there is an old Pull request without the label that is without updates, comment to the organization whether it is appropriate to add the *[S-Adopt-Me](https://github.com/bevyengine/bevy/labels/S-Adopt-Me)* label, to indicate that it can be *adopted*. Otherwise, should leave a comment on the PR asking the author if they plan on returning, if the author gives permission or simply doesn't respond after few days, then it can be adopted. This may sometimes even skip the labeling since at that point the PR has been adopted.


Note: I replied to your comment.

Not a blocker, but I still think it would be beneficial to add a new line after each sentence/phrase. It will make future review much simpler!

Thanks for the explanation, we added this suggestion, really is more simpler for review.

mockersf · 2022-12-19T11:18:22Z

CONTRIBUTING.md

@@ -275,6 +275,16 @@ By giving feedback on this work (and related supporting work), you can help us m
 Finally, if nothing brings you more satisfaction than seeing every last issue labeled and all resolved issues closed, feel free to message @cart for a Bevy org role to help us keep things tidy.
 As discussed in [*How we're organized*](#how-were-organized), this role only requires good faith and a basic understanding of our development process.

+### How to adopt pull requests
+
+Occasionally authors of pull requests get busy or become unresponsive. This is a natural part of any open source project. To avoid blocking these efforts, these pull requests may be *adopted*, where another contributor creates a new pull request with the same content. If there is an old Pull request without the label that is without updates, comment to the organization whether it is appropriate to add the *[S-Adopt-Me](https://github.com/bevyengine/bevy/labels/S-Adopt-Me)* label, to indicate that it can be *adopted*. Otherwise, should leave a comment on the PR asking the author if they plan on returning, if the author gives permission or simply doesn't respond after few days, then it can be adopted. This may sometimes even skip the labeling since at that point the PR has been adopted.


Occasionally authors of pull requests get busy or become unresponsive.

This could make it seems like it's all on the authors shoulders... Sometimes it's also on the project side that we fail to reply in a timely manner

Changes have been done @mockersf.

Co-authored-by: Gino Valente <[email protected]>

Co-authored-by: Erick <[email protected]>

MrGVSV

Looks good! Thank you!

mockersf · 2022-12-21T11:11:00Z

CONTRIBUTING.md

@@ -275,6 +275,26 @@ By giving feedback on this work (and related supporting work), you can help us m
 Finally, if nothing brings you more satisfaction than seeing every last issue labeled and all resolved issues closed, feel free to message @cart for a Bevy org role to help us keep things tidy.
 As discussed in [*How we're organized*](#how-were-organized), this role only requires good faith and a basic understanding of our development process.

+### How to adopt pull requests
+
+Occasionally authors of pull requests get busy or become unresponsive.


this is still putting all the fault on PR authors

Sorry, we were sure we had committed the changes, now it's there.

CONTRIBUTING.md

Co-authored-by: Gino Valente <[email protected]>

alice-i-cecile · 2022-12-22T19:40:23Z

CONTRIBUTING.md

@@ -277,7 +277,7 @@ As discussed in [*How we're organized*](#how-were-organized), this role only req

 ### How to adopt pull requests

-Occasionally authors of pull requests get busy or become unresponsive.
+Occasionally authors of pull requests get busy or become unresponsive, in other hand, on the project side, sometimes members fail to reply in a timely manner.


Suggested change

Occasionally authors of pull requests get busy or become unresponsive, in other hand, on the project side, sometimes members fail to reply in a timely manner.

Occasionally authors of pull requests get busy or become unresponsive, or project members fail to reply in a timely manner.

Just cleaning up the wording a bit then this LGTM!

Done! Thanks for the suggestion.

alice-i-cecile · 2022-12-22T19:49:39Z

@oCaioOliveira Thanks for your patience and careful work here; it's great to have this written up :)

bors r+

# Objective - Add "how to adopt pull requests" section. - Fixes #5539 ## Solution - Add "how to adopt pull requests" section in [Contributing.md](https://github.com/bevyengine/bevy/blob/main/CONTRIBUTING.md). Co-authored-by: Erick <[email protected]>

bors · 2022-12-22T20:09:19Z

Pull request successfully merged into main.

Build succeeded:

# Objective - Add "how to adopt pull requests" section. - Fixes bevyengine#5539 ## Solution - Add "how to adopt pull requests" section in [Contributing.md](https://github.com/bevyengine/bevy/blob/main/CONTRIBUTING.md). Co-authored-by: Erick <[email protected]>

Add "how to adopt pull requests" section

48aab46

Fixes bevyengine#5539 Co-authored-by: Erick <[email protected]>

MrGVSV reviewed Dec 9, 2022

View reviewed changes

james7132 requested changes Dec 9, 2022

View reviewed changes

james7132 added C-Docs An addition or correction to our documentation A-Meta About the project itself labels Dec 9, 2022

Add aditional description to section How to adopt pull requests

a49850a

james7132 requested changes Dec 11, 2022

View reviewed changes

CONTRIBUTING.md Show resolved Hide resolved

CONTRIBUTING.md Outdated Show resolved Hide resolved

oCaioOliveira and others added 3 commits December 11, 2022 21:33

Add suggestion

d6d0b9d

Co-authored-by: James Liu <[email protected]>

Best rewrite of How to adopt pull requests

21f55b7

Co-authored-by: James Liu <[email protected]>

Add aditional description to How to adopt pull requests

002c62e

MrGVSV reviewed Dec 13, 2022

View reviewed changes

oCaioOliveira and others added 4 commits December 15, 2022 00:13

More context to example

4dd3cb8

Co-authored-by: Gino Valente <[email protected]>

Add aditional description to section How to adopt pull requests

1a07dd7

Co-authored-by: Erick <[email protected]>

Fix ERRORS FOUND in MARKDOWN

106a8c3

Fixes ERRORS FOUND in MARKDOWN

cc9b59e

james7132 approved these changes Dec 15, 2022

View reviewed changes

alice-i-cecile self-requested a review December 16, 2022 01:38

MrGVSV reviewed Dec 19, 2022

View reviewed changes

mockersf reviewed Dec 19, 2022

View reviewed changes

oCaioOliveira and others added 2 commits December 20, 2022 11:07

Fixes typos

b4c9de6

Co-authored-by: Gino Valente <[email protected]>

Aditional description How to adopt pull requests

8b156ad

Co-authored-by: Erick <[email protected]>

MrGVSV approved these changes Dec 20, 2022

View reviewed changes

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 Dec 21, 2022

Weibye approved these changes Dec 21, 2022

View reviewed changes

mockersf requested changes Dec 21, 2022

View reviewed changes

alice-i-cecile removed 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 Dec 22, 2022

Aditional description to reason label S-Adopt-Me

a0ca180

MrGVSV reviewed Dec 22, 2022

View reviewed changes

CONTRIBUTING.md Outdated Show resolved Hide resolved

Refactored phrase on section How To Adopt PR

96e0ca0

Co-authored-by: Gino Valente <[email protected]>

alice-i-cecile reviewed Dec 22, 2022

View reviewed changes

Add suggestions of fixe wording

b76a123

alice-i-cecile approved these changes Dec 22, 2022

View reviewed changes

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 Dec 22, 2022

bors bot changed the title ~~Add "how to adopt pull requests" section~~ [Merged by Bors] - Add "how to adopt pull requests" section Dec 22, 2022

bors bot closed this Dec 22, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Merged by Bors] - Add "how to adopt pull requests" section #6895

[Merged by Bors] - Add "how to adopt pull requests" section #6895

oCaioOliveira commented Dec 9, 2022

MrGVSV left a comment

MrGVSV Dec 9, 2022

james7132 Dec 9, 2022

Dessix Dec 10, 2022

oCaioOliveira Dec 10, 2022

Dessix Dec 11, 2022

alice-i-cecile Dec 11, 2022

oCaioOliveira commented Dec 10, 2022

oCaioOliveira commented Dec 10, 2022 •

edited

Loading

alice-i-cecile commented Dec 11, 2022

MrGVSV commented Dec 11, 2022

alice-i-cecile commented Dec 11, 2022

oCaioOliveira commented Dec 12, 2022

MrGVSV left a comment

MrGVSV Dec 13, 2022 •

edited

Loading

oCaioOliveira Dec 15, 2022

oCaioOliveira Dec 15, 2022

MrGVSV Dec 18, 2022

MrGVSV Dec 13, 2022

oCaioOliveira Dec 15, 2022

MrGVSV Dec 19, 2022

oCaioOliveira Dec 20, 2022

mockersf Dec 19, 2022

oCaioOliveira Dec 20, 2022

MrGVSV left a comment

mockersf Dec 21, 2022

oCaioOliveira Dec 22, 2022

alice-i-cecile Dec 22, 2022

alice-i-cecile Dec 22, 2022

oCaioOliveira Dec 22, 2022

alice-i-cecile commented Dec 22, 2022

bors bot commented Dec 22, 2022


		With this label added, it's best practice to fork the original author's branch. This ensures that they still get credit for working on it and that the commit history is retained. When the new pull request is ready, it should reference the original PR in the description. Then notify org members to close the original.

		* Example: `Adopted #number-original-pull-request`

[Merged by Bors] - Add "how to adopt pull requests" section #6895

[Merged by Bors] - Add "how to adopt pull requests" section #6895

Conversation

oCaioOliveira commented Dec 9, 2022

Objective

Solution

MrGVSV left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oCaioOliveira commented Dec 10, 2022

oCaioOliveira commented Dec 10, 2022 • edited Loading

alice-i-cecile commented Dec 11, 2022

MrGVSV commented Dec 11, 2022

alice-i-cecile commented Dec 11, 2022

oCaioOliveira commented Dec 12, 2022

MrGVSV left a comment

Choose a reason for hiding this comment

MrGVSV Dec 13, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MrGVSV left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alice-i-cecile commented Dec 22, 2022

bors bot commented Dec 22, 2022

oCaioOliveira commented Dec 10, 2022 •

edited

Loading

MrGVSV Dec 13, 2022 •

edited

Loading