Skip to content

Commit 401b1d8

Browse files
IvorforceIvorforce
committed
Separate merge guidelines from review guidelines.
1 parent e81b33c commit 401b1d8

File tree

3 files changed

+180
-173
lines changed

3 files changed

+180
-173
lines changed

‎organization/pull_requests/index.rst‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,3 +15,4 @@ Learn how we work with pull requests in this section.
1515
review_process
1616
review_guidelines
1717
testing
18+
merge_guidelines

‎organization/pull_requests/merge_guidelines.rst‎

Lines changed: 179 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,179 @@
1+
.. _doc_pr_review_guidelines:
2+
3+
Merge guidelines
4+
================
5+
6+
.. note::
7+
8+
This section is targeted at maintainers. Regular contributors to Godot
9+
cannot merge pull requests.
10+
11+
In general, pull requests should only be merged by members of the production
12+
team or team leaders for pull requests in their area of the engine. For example,
13+
the networking team leader could merge a networking pull request that doesn't
14+
substantially change non-networking sections of code.
15+
16+
In practice it is best to wait for a member of the production team to merge the
17+
pull request as they keep a close eye on the entire codebase and will likely
18+
have a better sense of what other recent/upcoming changes this pull request may
19+
conflict with (or any other reason that it may make sense to delay the pull
20+
request). Feel free to leave a comment saying that the PR should be ready to
21+
merge.
22+
23+
The following are the steps to take before merging a pull request. The degree to
24+
which you adhere to these steps can be flexible for simple/straightforward pull
25+
requests, but they should be carefully taken for complex or risky pull requests.
26+
27+
As a contributor you can help move a pull request forward by doing some of these
28+
steps yourself.
29+
30+
1. Get feedback from the right people/teams
31+
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
32+
33+
Production team members should ensure that the right people look at a pull
34+
request before it is merged. In some cases this may require multiple people to
35+
weigh in. In other cases, only one substantive approval is needed before the
36+
code can be merged.
37+
38+
In general, try not to merge things based on one review alone, especially if it
39+
is your own. Get a second opinion from another maintainer, and make sure all the
40+
teams that may be impacted have been reasonably represented by the reviewers.
41+
For example, if a pull request adds to the documentation, it's often useful to
42+
let the area maintainers check it for factual correctness and let documentation
43+
maintainers check it for formatting, style, and grammar.
44+
45+
A good rule of thumb is that at least one subject matter expert should have
46+
approved the pull request for correctness, and at least one other maintainer
47+
should have approved the pull request for code style. Either of those people
48+
could be the person merging the pull request.
49+
50+
Make sure that the reviews and approvals were left by people competent in that
51+
specific engine area. It is possible that even a long-standing member of the
52+
Godot organization left a review without having the relevant expertise.
53+
54+
.. note::
55+
56+
An easy way to find PRs that may be ready for merging is filtering by
57+
approved PRs and sorting by recently updated. For example, in the main Godot
58+
repository, you can use `this link
59+
<https://github.com/godotengine/godot/pulls?q=is%3Apr+is%3Aopen+review%3Aapproved+sort%3Aupdated-desc>`_.
60+
61+
2. Get feedback from the community
62+
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
63+
64+
If a pull request is having trouble attracting reviewers, you may need to reach
65+
out more broadly to ask for help reviewing. Consider asking:
66+
67+
* the person who reported the bug if the pull request fixes the bug for them,
68+
* contributors who have recently edited that file if they could take a look, or
69+
* a more experienced maintainer from another area if they could provide feedback.
70+
71+
3. Git checklist
72+
~~~~~~~~~~~~~~~~
73+
74+
* **Make sure that the PR comes in one commit.**
75+
76+
When each commit is self-contained and could be used to build a clean and
77+
working version of the engine, it may be okay to merge a pull request with
78+
multiple commits, but in general, we require that all pull requests only have
79+
one commit. This helps us keep the Git history clean.
80+
81+
* **Fixes made during the review process must be squashed into
82+
the main commit.**
83+
84+
For multi-commit PRs check that those fixes are amended in the relevant
85+
commits, and are not just applied on top of everything.
86+
87+
* **Make sure that the PR has no merge conflicts.**
88+
89+
Contributors may need to rebase their changes on top of the relevant branch
90+
(e.g. ``master`` or ``3.x``) and manually fix merge conflicts. Even if there
91+
are no merge conflicts, contributors may need to rebase especially old PRs as
92+
the GitHub conflict checker may not catch all conflicts, or the CI may have
93+
changed since it was originally run.
94+
95+
* **Check for proper commit attribution.**
96+
97+
If a contributor uses an author signature that is not listed in their GitHub
98+
account, GitHub won't link the merged pull request to their account. This
99+
keeps them from getting proper credit in the GitHub history and makes them
100+
appear like a new contributor on the GitHub UI even after several
101+
contributions.
102+
103+
Ultimately, it's up to the user if they want to fix it, but they can do so by
104+
authoring the Git commit with the same email they use for their GitHub
105+
account, or by adding the email they used for the Git commit to their GitHub
106+
profile.
107+
108+
* **Check for proper commit messages.**
109+
110+
While we don't have a very strict ruleset for commit messages, we still
111+
require them to be short yet descriptive and use proper English. As a
112+
maintainer you've probably written them enough times to know how to make one,
113+
but for a general template think about *"Fix <issue> in <part of codebase>"*.
114+
For a more detailed recommendation see the `contributing.md
115+
<https://github.com/godotengine/godot/blob/master/CONTRIBUTING.md#format-your-commit-messages-with-readability-in-mind>`_
116+
page in the main Godot repository.
117+
118+
4. GitHub checklist
119+
~~~~~~~~~~~~~~~~~~~
120+
121+
* **Validate the target branch of the PR.**
122+
123+
Most Godot development happens around in the ``master`` branch. Therefore most
124+
pull requests must be made against it. From there pull requests can then be
125+
backported to other branches. Be wary of people making PRs on the version they
126+
are using (e.g, ``3.3``) and guide them to make a change against a
127+
higher-order branch (e.g. ``3.x``). If the change is not applicable for the
128+
``master`` branch, the initial PR can be made against the current maintenance
129+
branch, such as ``3.x``. It's okay for people to make multiple PRs for each
130+
target branch, especially if the changes cannot be easily backported.
131+
Cherry-picking is also an option, if possible. Use the appropriate labels if
132+
the PR can be cherrypicked (e.g. ``cherrypick:3.x``).
133+
134+
.. note::
135+
136+
It is possible to change the target branch of the PR, that has already been
137+
submitted, but be aware of the consequences. As it cannot be synchronized
138+
with the push, the target branch change will inevitable tag the entire list
139+
of maintainers for review. It may also render the CI incapable of running
140+
properly. A push should help with that, but if nothing else, recommend
141+
opening a new, fresh PR.
142+
143+
* **Make sure that the appropriate milestone is assigned.**
144+
145+
This will make it more obvious which version would include the submitted
146+
changes, should the pull request be merged now. Note, that the milestone is
147+
not a binding contract and does not guarantee that this version is definitely
148+
going to include the PR. If the pull request is not merged before the version
149+
is released, the milestone will be moved (and the PR itself may require a
150+
target branch change).
151+
152+
Similarly, when merging a PR with a higher milestone than the current version,
153+
or a "wildcard" milestone (e.g. "4.x"), ensure to update the milestone to the
154+
current version.
155+
156+
* **Make sure that the opening message of the PR contains the
157+
magic words "Closes #..." or "Fixes #...".**
158+
159+
These link the PR and the referenced issue together and allow GitHub to
160+
auto-close the latter when you merge the changes. Note, that this only works
161+
for the PRs that target the ``master`` branch. For others you need to pay
162+
attention and close the related issues manually. Do it with *"Fixed by #..."*
163+
or *"Resolved by #..."* comment to clearly indicate the act for future
164+
contributors.
165+
166+
* **For the issues that get closed by the PR add the closest
167+
relevant milestone.**
168+
169+
In other words, if the PR is targeting the ``master`` branch, but is then also
170+
cherrypicked for ``3.x``, the next ``3.x`` release would be the appropriate
171+
milestone for the closed issue.
172+
173+
5. Merge the pull request
174+
~~~~~~~~~~~~~~~~~~~~~~~~~
175+
176+
If it is appropriate for you to be merging a pull request (i.e. you are on the
177+
production team or you are the team leader for that area), you are confident
178+
that the pull request has been sufficiently reviewed, and once you carry out
179+
these steps you can go ahead and merge the pull request.

‎organization/pull_requests/review_guidelines.rst‎

Lines changed: 0 additions & 173 deletions
Original file line numberDiff line numberDiff line change
@@ -194,176 +194,3 @@ and are unable to detect some issues. For example, you should check that:
194194
in mind that ``clang-format`` may not catch things you hope it would,
195195
so pay attention and try to build a sense of what exactly it can and
196196
cannot detect.
197-
198-
Merging pull requests
199-
---------------------
200-
201-
In general, pull requests should only be merged by members of the production
202-
team or team leaders for pull requests in their area of the engine. For example,
203-
the networking team leader could merge a networking pull request that doesn't
204-
substantially change non-networking sections of code.
205-
206-
In practice it is best to wait for a member of the production team to merge the
207-
pull request as they keep a close eye on the entire codebase and will likely
208-
have a better sense of what other recent/upcoming changes this pull request may
209-
conflict with (or any other reason that it may make sense to delay the pull
210-
request). Feel free to leave a comment saying that the PR should be ready to
211-
merge.
212-
213-
The following are the steps to take before merging a pull request. The degree to
214-
which you adhere to these steps can be flexible for simple/straightforward pull
215-
requests, but they should be carefully taken for complex or risky pull requests.
216-
217-
As a contributor you can help move a pull request forward by doing some of these
218-
steps yourself.
219-
220-
1. Get feedback from the right people/teams
221-
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
222-
223-
Production team members should ensure that the right people look at a pull
224-
request before it is merged. In some cases this may require multiple people to
225-
weigh in. In other cases, only one substantive approval is needed before the
226-
code can be merged.
227-
228-
In general, try not to merge things based on one review alone, especially if it
229-
is your own. Get a second opinion from another maintainer, and make sure all the
230-
teams that may be impacted have been reasonably represented by the reviewers.
231-
For example, if a pull request adds to the documentation, it's often useful to
232-
let the area maintainers check it for factual correctness and let documentation
233-
maintainers check it for formatting, style, and grammar.
234-
235-
A good rule of thumb is that at least one subject matter expert should have
236-
approved the pull request for correctness, and at least one other maintainer
237-
should have approved the pull request for code style. Either of those people
238-
could be the person merging the pull request.
239-
240-
Make sure that the reviews and approvals were left by people competent in that
241-
specific engine area. It is possible that even a long-standing member of the
242-
Godot organization left a review without having the relevant expertise.
243-
244-
.. note::
245-
246-
An easy way to find PRs that may be ready for merging is filtering by
247-
approved PRs and sorting by recently updated. For example, in the main Godot
248-
repository, you can use `this link
249-
<https://github.com/godotengine/godot/pulls?q=is%3Apr+is%3Aopen+review%3Aapproved+sort%3Aupdated-desc>`_.
250-
251-
2. Get feedback from the community
252-
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
253-
254-
If a pull request is having trouble attracting reviewers, you may need to reach
255-
out more broadly to ask for help reviewing. Consider asking:
256-
257-
* the person who reported the bug if the pull request fixes the bug for them,
258-
* contributors who have recently edited that file if they could take a look, or
259-
* a more experienced maintainer from another area if they could provide feedback.
260-
261-
3. Git checklist
262-
~~~~~~~~~~~~~~~~
263-
264-
* **Make sure that the PR comes in one commit.**
265-
266-
When each commit is self-contained and could be used to build a clean and
267-
working version of the engine, it may be okay to merge a pull request with
268-
multiple commits, but in general, we require that all pull requests only have
269-
one commit. This helps us keep the Git history clean.
270-
271-
* **Fixes made during the review process must be squashed into
272-
the main commit.**
273-
274-
For multi-commit PRs check that those fixes are amended in the relevant
275-
commits, and are not just applied on top of everything.
276-
277-
* **Make sure that the PR has no merge conflicts.**
278-
279-
Contributors may need to rebase their changes on top of the relevant branch
280-
(e.g. ``master`` or ``3.x``) and manually fix merge conflicts. Even if there
281-
are no merge conflicts, contributors may need to rebase especially old PRs as
282-
the GitHub conflict checker may not catch all conflicts, or the CI may have
283-
changed since it was originally run.
284-
285-
* **Check for proper commit attribution.**
286-
287-
If a contributor uses an author signature that is not listed in their GitHub
288-
account, GitHub won't link the merged pull request to their account. This
289-
keeps them from getting proper credit in the GitHub history and makes them
290-
appear like a new contributor on the GitHub UI even after several
291-
contributions.
292-
293-
Ultimately, it's up to the user if they want to fix it, but they can do so by
294-
authoring the Git commit with the same email they use for their GitHub
295-
account, or by adding the email they used for the Git commit to their GitHub
296-
profile.
297-
298-
* **Check for proper commit messages.**
299-
300-
While we don't have a very strict ruleset for commit messages, we still
301-
require them to be short yet descriptive and use proper English. As a
302-
maintainer you've probably written them enough times to know how to make one,
303-
but for a general template think about *"Fix <issue> in <part of codebase>"*.
304-
For a more detailed recommendation see the `contributing.md
305-
<https://github.com/godotengine/godot/blob/master/CONTRIBUTING.md#format-your-commit-messages-with-readability-in-mind>`_
306-
page in the main Godot repository.
307-
308-
4. GitHub checklist
309-
~~~~~~~~~~~~~~~~~~~
310-
311-
* **Validate the target branch of the PR.**
312-
313-
Most Godot development happens around in the ``master`` branch. Therefore most
314-
pull requests must be made against it. From there pull requests can then be
315-
backported to other branches. Be wary of people making PRs on the version they
316-
are using (e.g, ``3.3``) and guide them to make a change against a
317-
higher-order branch (e.g. ``3.x``). If the change is not applicable for the
318-
``master`` branch, the initial PR can be made against the current maintenance
319-
branch, such as ``3.x``. It's okay for people to make multiple PRs for each
320-
target branch, especially if the changes cannot be easily backported.
321-
Cherry-picking is also an option, if possible. Use the appropriate labels if
322-
the PR can be cherrypicked (e.g. ``cherrypick:3.x``).
323-
324-
.. note::
325-
326-
It is possible to change the target branch of the PR, that has already been
327-
submitted, but be aware of the consequences. As it cannot be synchronized
328-
with the push, the target branch change will inevitable tag the entire list
329-
of maintainers for review. It may also render the CI incapable of running
330-
properly. A push should help with that, but if nothing else, recommend
331-
opening a new, fresh PR.
332-
333-
* **Make sure that the appropriate milestone is assigned.**
334-
335-
This will make it more obvious which version would include the submitted
336-
changes, should the pull request be merged now. Note, that the milestone is
337-
not a binding contract and does not guarantee that this version is definitely
338-
going to include the PR. If the pull request is not merged before the version
339-
is released, the milestone will be moved (and the PR itself may require a
340-
target branch change).
341-
342-
Similarly, when merging a PR with a higher milestone than the current version,
343-
or a "wildcard" milestone (e.g. "4.x"), ensure to update the milestone to the
344-
current version.
345-
346-
* **Make sure that the opening message of the PR contains the
347-
magic words "Closes #..." or "Fixes #...".**
348-
349-
These link the PR and the referenced issue together and allow GitHub to
350-
auto-close the latter when you merge the changes. Note, that this only works
351-
for the PRs that target the ``master`` branch. For others you need to pay
352-
attention and close the related issues manually. Do it with *"Fixed by #..."*
353-
or *"Resolved by #..."* comment to clearly indicate the act for future
354-
contributors.
355-
356-
* **For the issues that get closed by the PR add the closest
357-
relevant milestone.**
358-
359-
In other words, if the PR is targeting the ``master`` branch, but is then also
360-
cherrypicked for ``3.x``, the next ``3.x`` release would be the appropriate
361-
milestone for the closed issue.
362-
363-
5. Merge the pull request
364-
~~~~~~~~~~~~~~~~~~~~~~~~~
365-
366-
If it is appropriate for you to be merging a pull request (i.e. you are on the
367-
production team or you are the team leader for that area), you are confident
368-
that the pull request has been sufficiently reviewed, and once you carry out
369-
these steps you can go ahead and merge the pull request.

0 commit comments

Comments
 (0)