|
@@ -12,7 +12,7 @@ Pull request review process
|
|
wanting to know how to ensure that their PR is merged.
|
|
wanting to know how to ensure that their PR is merged.
|
|
|
|
|
|
From a high level, the ideal life cycle of a pull request looks like the
|
|
From a high level, the ideal life cycle of a pull request looks like the
|
|
-following:
|
|
|
|
|
|
+following:
|
|
|
|
|
|
1. A contributor opens a PR that fixes a specific problem (optimally closing
|
|
1. A contributor opens a PR that fixes a specific problem (optimally closing
|
|
a GitHub `issue <https://github.com/godotengine/godot>`_ or implementing
|
|
a GitHub `issue <https://github.com/godotengine/godot>`_ or implementing
|
|
@@ -33,9 +33,9 @@ following:
|
|
|
|
|
|
This document will explain steps 2, 3, 4, and 5 in more detail. For a more
|
|
This document will explain steps 2, 3, 4, and 5 in more detail. For a more
|
|
detailed explanation of the pull request workflow please see the :ref:`pull
|
|
detailed explanation of the pull request workflow please see the :ref:`pull
|
|
-request workflow document <doc_pr_workflow>`.
|
|
|
|
|
|
+request workflow document <doc_pr_workflow>`.
|
|
|
|
|
|
-.. note::
|
|
|
|
|
|
+.. note::
|
|
In practice these steps may blend together. Oftentimes maintainers will
|
|
In practice these steps may blend together. Oftentimes maintainers will
|
|
provide comments on code style and code quality at the same time and will
|
|
provide comments on code style and code quality at the same time and will
|
|
approve a pull request for both.
|
|
approve a pull request for both.
|
|
@@ -160,7 +160,7 @@ As you review pull requests, keep the Godot `Code of Conduct
|
|
|
|
|
|
* Always assume positive intent from others.
|
|
* Always assume positive intent from others.
|
|
|
|
|
|
-* Feedback is always welcome but keep your criticism constructive.
|
|
|
|
|
|
+* Feedback is always welcome, but keep your criticism constructive.
|
|
|
|
|
|
Here are some things to avoid as you iterate on a pull request with a
|
|
Here are some things to avoid as you iterate on a pull request with a
|
|
contributor:
|
|
contributor:
|
|
@@ -178,7 +178,7 @@ contributor:
|
|
areas and in performance-sensitive areas than it is in editor code for
|
|
areas and in performance-sensitive areas than it is in editor code for
|
|
example.
|
|
example.
|
|
|
|
|
|
-* **Expanding the scope of a pull request.**
|
|
|
|
|
|
+* **Expanding the scope of a pull request.**
|
|
|
|
|
|
Providing context or related/similar issues or proposals that may be fixed
|
|
Providing context or related/similar issues or proposals that may be fixed
|
|
similarly can be helpful, but adding a "may as well fix that thing over there
|
|
similarly can be helpful, but adding a "may as well fix that thing over there
|
|
@@ -253,7 +253,7 @@ Merging pull requests
|
|
In general, pull requests should only be merged by members of the production
|
|
In general, pull requests should only be merged by members of the production
|
|
team or team leaders for pull requests in their area of the engine. For example,
|
|
team or team leaders for pull requests in their area of the engine. For example,
|
|
the networking team leader could merge a networking pull request that doesn't
|
|
the networking team leader could merge a networking pull request that doesn't
|
|
-substantially change non-networking sections of code.
|
|
|
|
|
|
+substantially change non-networking sections of code.
|
|
|
|
|
|
In practice it is best to wait for a member of the production team to merge the
|
|
In practice it is best to wait for a member of the production team to merge the
|
|
pull request as they keep a close eye on the entire codebase and will likely
|
|
pull request as they keep a close eye on the entire codebase and will likely
|
|
@@ -275,7 +275,7 @@ steps yourself.
|
|
Production team members should ensure that the right people look at a pull
|
|
Production team members should ensure that the right people look at a pull
|
|
request before it is merged. In some cases this may require multiple people to
|
|
request before it is merged. In some cases this may require multiple people to
|
|
weigh in. In other cases, only one substantive approval is needed before the
|
|
weigh in. In other cases, only one substantive approval is needed before the
|
|
-code can be merged.
|
|
|
|
|
|
+code can be merged.
|
|
|
|
|
|
In general, try not to merge things based on one review alone, especially if it
|
|
In general, try not to merge things based on one review alone, especially if it
|
|
is your own. Get a second opinion from another maintainer, and make sure all the
|
|
is your own. Get a second opinion from another maintainer, and make sure all the
|