godotengine/godot-contributing-docs
1
0
Fork 0
mirror of https://github.com/godotengine/godot-contributing-docs.git synced 2025-12-31 05:48:13 +03:00
Code Issues Packages Projects Releases Wiki Activity

Move pull requests info to organization, since it's relevant for many areas.

Browse Source
This commit is contained in:
Lukas Tenbrink
2025-07-30 12:28:29 +02:00
parent 867c827c8f
commit cffd8bbe8a
6 changed files with 1 additions and 1 deletions
Download Patch File Download Diff File Expand all files Collapse all files

579
engine/pull_requests/creating_pull_requests.rst
View File

@@ -1,579 +0,0 @@
.. _doc_pr_workflow:
Creating pull requests
======================
.. highlight:: shell
The so-called "PR workflow" used by Godot is common to many projects using
Git, and should be familiar to veteran free software contributors. The idea
is that only a small number (if any) commit directly to the *master* branch.
Instead, contributors *fork* the project (i.e. create a copy of it, which
they can modify as they wish), and then use the GitHub interface to request
a *pull* from one of their fork's branches to one branch of the original
(often named *upstream*) repository.
The resulting *pull request* (PR) can then be reviewed by other contributors,
which might approve it, reject it, or most often request that modifications
be done. Once approved, the PR can then be merged by one of the core
developers, and its commit(s) will become part of the target branch (usually
the *master* branch).
We will go together through an example to show the typical workflow and
associated Git commands. But first, let's have a quick look at the
organization of Godot's Git repository.
Git source repository
---------------------
The `repository on GitHub <https://github.com/godotengine/godot>`_ is a
`Git <https://git-scm.com>`_ code repository together with an embedded
issue tracker and PR system.
.. note:: If you are contributing to the documentation, its repository can
be found `here <https://github.com/godotengine/godot-docs>`_.
The Git version control system is the tool used to keep track of successive
edits to the source code - to contribute efficiently to Godot, learning the
basics of the Git command line is *highly* recommended. There exist some
graphical interfaces for Git, but they usually encourage users to take bad
habits regarding the Git and PR workflow, and we therefore recommend not to
use them. In particular, we advise not to use GitHub's online editor for code
contributions (although it's tolerated for small fixes or documentation changes)
as it enforces one commit per file and per modification,
which quickly leads to PRs with an unreadable Git history (especially after peer review).
.. seealso:: The first sections of Git's "Book" are a good introduction to
the tool's philosophy and the various commands you need to
master in your daily workflow. You can read them online on the
`Git SCM <https://git-scm.com/book/en/v2>`_ website.
You can also try out `GitHub's interactive guide <https://try.github.io/>`__.
The branches on the Git repository are organized as follows:
- The ``master`` branch is where the development of the next major version
occurs. As a development branch, it can be unstable
and is not meant for use in production. This is where PRs should be done
in priority.
- The stable branches are named after their version, e.g. ``3.1`` and ``2.1``.
They are used to backport bugfixes and enhancements from the ``master``
branch to the currently maintained stable release (e.g. 3.1.2 or 2.1.6).
As a rule of thumb, the last stable branch is maintained until the next
minor version (e.g. the ``3.0`` branch was maintained until the release of
Godot 3.1).
If you want to make PRs against a maintained stable branch, please check
first if your changes are also relevant for the ``master`` branch, and if so
make the PR for the ``master`` branch in priority. Release managers can then
cherry-pick the fix to a stable branch if relevant.
- There might occasionally be feature branches, usually meant to be merged into
the ``master`` branch at some time.
Forking and cloning
-------------------
The first step is to *fork* the `godotengine/godot <https://github.com/godotengine/godot>`_
repository on GitHub. To do so, you will need to have a GitHub account and to
be logged in. In the top right corner of the repository's GitHub page, you
should see the "Fork" button as shown below:
.. image:: img/github_fork_button.png
Click it, and after a while you should be redirected to your own fork of the
Godot repo, with your GitHub username as namespace:
.. image:: img/github_fork_url.png
You can then *clone* your fork, i.e. create a local copy of the online
repository (in Git speak, the *origin remote*). If you haven't already,
download Git from `its website <https://git-scm.com>`_ if you're using Windows or
macOS, or install it through your package manager if you're using Linux.
.. note:: If you are on Windows, open Git Bash to type commands. macOS and Linux users
can use their respective terminals.
To clone your fork from GitHub, use the following command:
::
git clone https://github.com/USERNAME/godot
After a little while, you should have a ``godot`` directory in your current
working directory. Move into it using the ``cd`` command:
::
cd godot
We will start by setting up a reference to the original repository that we forked:
::
git remote add upstream https://github.com/godotengine/godot
git fetch upstream
This will create a reference named ``upstream`` pointing to the original
``godotengine/godot`` repository. This will be useful when you want to pull new
commits from its ``master`` branch to update your fork. You have another
remote reference named ``origin``, which points to your fork (``USERNAME/godot``).
You only need to do the above steps once, as long as you keep that local
``godot`` folder (which you can move around if you want, the relevant
metadata is hidden in its ``.git`` subfolder).
.. note:: *Branch it, pull it, code it, stage it, commit, push it, rebase
it... technologic.*
This bad take on Daft Punk's *Technologic* shows the general
conception Git beginners have of its workflow: lots of strange
commands to learn by copy and paste, hoping they will work as
expected. And that's actually not a bad way to learn, as long as
you're curious and don't hesitate to question your search engine
when lost, so we will give you the basic commands to know when
working in Git.
In the following, we will assume as an example that you want to implement a feature in
Godot's Project Manager, which is coded in the ``editor/project_manager.cpp``
file.
Branching
---------
By default, the ``git clone`` should have put you on the ``master`` branch of
your fork (``origin``). To start your own feature development, we will create
a feature branch:
::
# Create the branch based on the current branch (master)
git branch better-project-manager
# Change the current branch to the new one
git checkout better-project-manager
This command is equivalent:
::
# Change the current branch to a new named one, based on the current branch
git checkout -b better-project-manager
If you want to go back to the ``master`` branch, you'd use:
::
git checkout master
You can see which branch you are currently on with the ``git branch``
command:
::
git branch
2.1
* better-project-manager
master
Be sure to always go back to the ``master`` branch before creating a new branch,
as your current branch will be used as the base for the new one. Alternatively,
you can specify a custom base branch after the new branch's name:
::
git checkout -b my-new-feature master
Updating your branch
--------------------
This would not be needed the first time (just after you forked the upstream
repository). However, the next time you want to work on something, you will
notice that your fork's ``master`` is several commits behind the upstream
``master`` branch: pull requests from other contributors would have been merged
in the meantime.
To ensure there won't be conflicts between the feature you develop and the
current upstream ``master`` branch, you will have to update your branch by
*pulling* the upstream branch.
::
git pull --rebase upstream master
The ``--rebase`` argument will ensure that any local changes that you committed
will be re-applied *on top* of the pulled branch, which is usually what we want
in our PR workflow. This way, when you open a pull request, your own commits will
be the only difference with the upstream ``master`` branch.
While rebasing, conflicts may arise if your commits modified code that has been
changed in the upstream branch in the meantime. If that happens, Git will stop at
the conflicting commit and will ask you to resolve the conflicts. You can do so
with any text editor, then stage the changes (more on that later), and proceed with
``git rebase --continue``. Repeat the operation if later commits have conflicts too,
until the rebase operation completes.
If you're unsure about what is going on during a rebase and you panic (no worry,
we all do the first few times), you can abort the rebase with ``git rebase --abort``.
You will then be back to the original state of your branch before calling
``git pull --rebase``.
.. note:: If you omit the ``--rebase`` argument, you will instead create a merge
commit which tells Git what to make of the two distinct branches. If any
conflicts arise, they would be resolved all at once via this merge commit.
While this is a valid workflow and the default behavior of ``git pull``,
merge commits within PRs are frowned upon in our PR workflow. We only use
them when merging PRs into the upstream branch.
The philosophy is that a PR should represent the final stage of the changes
made to the codebase, and we are not interested in mistakes and fixes that
would have been done in intermediate stages before merging.
Git gives us great tools to "rewrite the history" and make it as if we got
things right the first time, and we're happy to use it to ensure that
changes are easy to review and understand long after they have been merged.
If you have already created a merge commit without using ``rebase``, or
have made any other changes that have resulted in undesired history, the best option
is to use an *interactive rebase* on the upstream branch. See the :ref:`dedicated
section <doc_pr_workflow_rebase>` for instructions.
.. tip:: If at any time you want to *reset* a local branch to a given commit or branch,
you can do so with ``git reset --hard <commit ID>`` or
``git reset --hard <remote>/<branch>`` (e.g. ``git reset --hard upstream/master``).
Be warned that this will remove any changes that you might have committed in
this branch. If you ever lose commits by mistake, use the ``git reflog`` command
to find the commit ID of the previous state that you would like to restore, and
use it as argument of ``git reset --hard`` to go back to that state.
Making changes
--------------
You would then do your changes to our example's
``editor/project_manager.cpp`` file with your usual development environment
(text editor, IDE, etc.).
By default, those changes are *unstaged*. The staging area is a layer between
your working directory (where you make your modifications) and the local Git
repository (the commits and all the metadata in the ``.git`` folder). To
bring changes from the working directory to the Git repository, you need to
*stage* them with the ``git add`` command, and then to commit them with the
``git commit`` command.
There are various commands you should know to review your current work,
before staging it, while it is staged, and after it has been committed.
- ``git diff`` will show you the current unstaged changes, i.e. the
differences between your working directory and the staging area.
- ``git checkout -- <files>`` will undo the unstaged changes to the given
files.
- ``git add <files>`` will *stage* the changes on the listed files.
- ``git diff --staged`` will show the current staged changes, i.e. the
differences between the staging area and the last commit.
- ``git reset HEAD <files>`` will *unstage* changes to the listed files.
- ``git status`` will show you what are the currently staged and unstaged
modifications.
- ``git commit`` will commit the staged files. It will open a text editor
(you can define the one you want to use with the ``GIT_EDITOR`` environment
variable or the ``core.editor`` setting in your Git configuration) to let you
write a commit log. You can use ``git commit -m "Cool commit log"`` to
write the log directly.
- ``git commit --amend`` lets you amend the last commit with your currently
staged changes (added with ``git add``). This is the best option if you
want to fix a mistake in the last commit (bug, typo, style issue, etc.).
- ``git log`` will show you the last commits of your current branch. If you
did local commits, they should be shown at the top.
- ``git show`` will show you the changes of the last commit. You can also
specify a commit hash to see the changes for that commit.
That's a lot to memorize! Don't worry, just check this cheat sheet when you
need to make changes, and learn by doing.
Here's how the shell history could look like on our example:
::
# It's nice to know where you're starting from
git log
# Do changes to the Project Manager with the nano text editor
nano editor/project_manager.cpp
# Find an unrelated bug in Control and fix it
nano scene/gui/control.cpp
# Review changes
git status
git diff
# We'll do two commits for our unrelated changes,
# starting by the Control changes necessary for the PM enhancements
git add scene/gui/control.cpp
git commit -m "Fix handling of margins in Control"
# Check we did good
git log
git show
git status
# Make our second commit
git add editor/project_manager.cpp
git commit -m "Add a pretty banner to the Project Manager"
git log
With this, we should have two new commits in our ``better-project-manager``
branch which were not in the ``master`` branch. They are still only local
though, the remote fork does not know about them, nor does the upstream repo.
Pushing changes to a remote
---------------------------
That's where ``git push`` will come into play. In Git, a commit is always
done in the local repository (unlike Subversion where a commit will modify
the remote repository directly). You need to *push* the new commits to a
remote branch to share them with the world. The syntax for this is:
::
git push <remote> <local branch>[:<remote branch>]
The part about the remote branch can be omitted if you want it to have the
same name as the local branch, which is our case in this example, so we will
do:
::
git push origin better-project-manager
Git will ask you for your username and password. For your password, enter your
GitHub Personal Access Token (PAT). If you do not have a GitHub Personal Access
Token, or do not have one with the correct permissions for your newly forked
repository, you will need to create one. Follow this link to create your Personal
Access Token: `Creating a personal access token
<https://docs.github.com/en/authentication/keeping-your-account-and-data-secure/creating-a-personal-access-token>`_.
After you have successfully verified your account using your PAT, the changes
will be sent to your remote repository. If you check the fork's page on GitHub,
you should see a new branch with your added commits.
Issuing a pull request
----------------------
When you load your fork's branch on GitHub, you should see a line saying
*"This branch is 2 commits ahead of godotengine:master."* (and potentially some
commits behind, if your ``master`` branch was out of sync with the upstream
``master`` branch).
.. image:: img/github_fork_make_pr.png
On that line, there is a "Pull request" link. Clicking it will open a form
that will let you issue a pull request on the ``godotengine/godot`` upstream
repository. It should show you your two commits, and state "Able to merge".
If not (e.g. it has way more commits, or says there are merge conflicts),
don't create the PR yet, something went wrong. Go to our
`Godot Contributors Chat <https://chat.godotengine.org/>`_ and ask for support :)
Use an explicit title for the PR and put the necessary details in the comment
area. You can drag and drop screenshots, GIFs or zipped projects if relevant,
to showcase what your work implements. Click "Create a pull request", and
tadaa!
Modifying a pull request
------------------------
While it is reviewed by other contributors, you will often need to make
changes to your yet-unmerged PR, either because contributors requested them,
or because you found issues yourself while testing.
The good news is that you can modify a pull request simply by acting on the
branch you made the pull request from. You can e.g. make a new commit on that
branch, push it to your fork, and the PR will be updated automatically:
::
# Check out your branch again if you had changed in the meantime
git checkout better-project-manager
# Fix a mistake
nano editor/project_manager.cpp
git add editor/project_manager.cpp
git commit -m "Fix a typo in the banner's title"
git push origin better-project-manager
However, be aware that in our PR workflow, we favor commits that bring the
codebase from one functional state to another functional state, without having
intermediate commits fixing up bugs in your own code or style issues. Most of
the time, we will prefer a single commit in a given PR (unless there's a good
reason to keep the changes separate). Instead of authoring a new commit,
consider using ``git commit --amend`` to amend the previous commit with your
fixes. The above example would then become:
::
# Check out your branch again if you had changed in the meantime
git checkout better-project-manager
# Fix a mistake
nano editor/project_manager.cpp
git add editor/project_manager.cpp
# --amend will change the previous commit, so you will have the opportunity
# to edit its commit message if relevant.
git commit --amend
# As we modified the last commit, it no longer matches the one from your
# remote branch, so we need to force push to overwrite that branch.
git push --force origin better-project-manager
.. Kept for compatibility with the previous title, linked in many PRs.
.. _mastering-the-pr-workflow-the-rebase:
.. _doc_pr_workflow_rebase:
The interactive rebase
----------------------
If you didn't follow the above steps closely to *amend* changes into a commit
instead of creating fixup commits, or if you authored your changes without being
aware of our workflow and Git usage tips, reviewers might request you to
*rebase* your branch to *squash* some or all of the commits into one.
Indeed, if some commits have been made following reviews to fix bugs, typos, etc.
in the original commit, they are not relevant to a future changelog reader who
would want to know what happened in the Godot codebase, or when and how a given
file was last modified.
To squash those extraneous commits into the main one, we will have to *rewrite
history*. Right, we have that power. You may read that it's a bad practice, and
it's true when it comes to branches of the upstream repo. But in your fork, you
can do whatever you want, and everything is allowed to get neat PRs :)
We will use the *interactive rebase* ``git rebase -i`` to do this. This
command takes a commit ID or a branch name as argument, and will let you modify
all commits between that commit/branch and the last one in your working branch,
the so-called ``HEAD``.
While you can give any commit ID to ``git rebase -i`` and review everything in
between, the most common and convenient workflow involves rebasing on the
upstream ``master`` branch, which you can do with:
::
git rebase -i upstream/master
.. note:: Referencing branches in Git is a bit tricky due to the distinction
between remote and local branches. Here, ``upstream/master`` (with a
`/`) is a local branch which has been pulled from the ``upstream``
remote's ``master`` branch.
Interactive rebases can only be done on local branches, so the `/`
is important here. As the upstream remote changes frequently, your
local ``upstream/master`` branch may become outdated, so you can
update it with ``git fetch upstream master``. Contrarily to
``git pull --rebase upstream master`` which would update your
currently checked out branch, ``fetch`` will only update the
``upstream/master`` reference (which is distinct from your local
``master`` branch... yes it's confusing, but you'll become familiar
with this little by little).
This will open a text editor (``vi`` by default, see
`Git docs <https://git-scm.com/book/en/v2/Customizing-Git-Git-Configuration#_core_editor>`_
to configure your favorite one) with something which may look like this:
.. code-block:: text
pick 1b4aad7 Add a pretty banner to the Project Manager
pick e07077e Fix a typo in the banner's title
The editor will also show instructions regarding how you can act on those
commits. In particular, it should tell you that "pick" means to use that
commit (do nothing), and that "squash" and "fixup" can be used to *meld* the
commit in its parent commit. The difference between "squash" and "fixup" is
that "fixup" will discard the commit log from the squashed commit. In our
example, we are not interested in keeping the log of the "Fix a typo" commit,
so we use:
.. code-block:: text
pick 1b4aad7 Add a pretty banner to the Project Manager
fixup e07077e Fix a typo in the banner's title
Upon saving and quitting the editor, the rebase will occur. The second commit
will be melded into the first one, and ``git log`` and ``git show`` should
now confirm that you have only one commit with the changes from both previous
commits.
But! You rewrote the history, and now your local and remote branches have
diverged. Indeed, commit 1b4aad7 in the above example will have changed, and
therefore got a new commit hash. If you try to push to your remote branch, it
will raise an error:
::
git push origin better-project-manager
To https://github.com/akien-mga/godot
! [rejected] better-project-manager -> better-project-manager (non-fast-forward)
error: failed to push some refs to 'https://akien-mga@github.com/akien-mga/godot'
hint: Updates were rejected because the tip of your current branch is behind
hint: its remote counterpart.
This is reasonable behavior, Git will not let you push changes that would
override remote content. But that's actually what we want to do here, so we
will have to *force* it:
::
git push --force origin better-project-manager
And tadaa! Git will happily *replace* your remote branch with what you had
locally (so make sure that's what you wanted, using ``git log``). This will
also update the PR accordingly.
Rebasing onto another branch
----------------------------
If you have accidentally opened your PR on the wrong branch, or need to target another branch
for some reason, you might need to filter out a lot of commits that differ between the old branch
(for example ``4.2``) and the new branch (for example ``master``). This can make rebasing difficult
and tedious. Fortunately ``git`` has a command just for this situation, ``git rebase --onto``.
If your PR was created from the ``4.2`` branch and you want to update it to instead start at ``master``
the following steps *should* fix this in one step:
.. code-block:: text
git rebase -i --onto master 4.2
This will take all the commits on your branch *after* the ``4.2`` branch, and then splice them on top of ``master``,
ignoring any commits from the ``4.2`` branch not on the ``master`` branch. You may still need to do some fixing, but
this command should save you a lot of tedious work removing commits.
Just like above for the interactive rebase you need to force push your branch to handle the different changes:
::
git push --force origin better-project-manager
Deleting a Git branch
---------------------
After your pull request gets merged, there's one last thing you should do: delete your
Git branch for the PR. There won't be issues if you don't delete your branch, but it's
good practice to do so. You'll need to do this twice, once for the local branch and another
for the remote branch on GitHub.
To delete our better Project Manager branch locally, use this command:
::
git branch -d better-project-manager
Alternatively, if the branch hadn't been merged yet and we wanted to delete it anyway, instead
of ``-d`` you would use ``-D``.
Next, to delete the remote branch on GitHub use this command:
::
git push origin -d better-project-manager
You can also delete the remote branch from the GitHub PR itself, a button should appear once
it has been merged or closed.

13
engine/pull_requests/index.rst
View File

@@ -1,13 +0,0 @@
Pull requests
=============
This section explains how to work with pull requests.
.. toctree::
:maxdepth: 1
:name: sec-pull-requests
creating_pull_requests
review_process
review_guidelines
testing

369
engine/pull_requests/review_guidelines.rst
View File

@@ -1,369 +0,0 @@
.. _doc_pr_review_guidelines:
Review guidelines
=================
Code review and testing
-----------------------
The following is a list of things that contributors and engine maintainers can
do to conduct a substantive code review of a pull request.
.. note::
If you want to conduct a code review, but can't do everything on this list,
say that in your review comment. For example, it is still very helpful to
provide comments on code, even if you can't build the pull request locally to
test the pull request (or vice versa). Feel free to review the code, just
remember to make a note at the end of your review that you have reviewed the
code only and have not tested the changes locally.
1. Confirm that the problem exists
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
PRs need to solve problems and problems need to be documented. Make sure that
the pull request links and closes (or at least addresses) a bug or a proposal.
If it doesn't, consider asking the contributor to update the opening message of
the PR to explain the problem that the PR aims to solve in more detail.
.. note::
It should be clear _why_ a pull request is needed before it is merged. This
assists reviewers in determining whether a PR does what it says it does and it
helps contributors in the future understand why the code is the way it is.
2. Test the PR and look for regressions
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
While strict code review and CI help to ensure that all pull requests work as
intended, mistakes happen and sometimes contributors push code that creates a
problem in addition to solving a problem. Maintainers will avoid merging code
that contains a regression even if it solves the problem as intended.
When reviewing a pull request, ensure that the PR does what it says it does
(i.e. fixes the linked bug or implements the new feature) and nothing outside of
the PR target area is broken by the change. You can do this by running the
editor and trying out some common functions of the editor (adding objects to a
scene, running GDScript, opening and closing menus etc.). Also, while reviewing
the code, look for suspicious changes in other parts of the engine. Sometimes
during rebasing changes slip through that contributors are not aware of.
3. Do a code review
~~~~~~~~~~~~~~~~~~~
Code reviews are usually done by people who are already experienced in a given
area. They may be able to provide ideas to make code faster, more organized, or
more idiomatic. But, even if you are not very experienced, you may want to
conduct a code review to provide feedback within the scope of what you are
comfortable reviewing. Doing so is valuable for the area maintainer (as a second
set of eyes on a problem is always helpful) and it is also helpful for you as it
will help you get more familiar with that area of code and will expose you to
how other people solve problems. In fact, reviewing the code of experienced
engine maintainers is a great way to get to know the codebase.
Here are some things to think about and look out for as you review the code:
* **Code only touches the areas announced in the PR (and the commit
message).**
It can be tempting to fix random things in the code, as you see them. However,
this can quickly make a pull request difficult to review and can make it hard
to dig through in the commit history. Small touch-ups next to the related area
are alright, but often bugs that you can find along the way are better fixed
in their own PRs.
* **Code properly uses Godot's own APIs and patterns.**
Consistency is very important, and a solution that already exists in the
codebase is preferable to an ad-hoc solution.
* **Are core areas affected by the change?**
Sometimes a PR that is supposed to solve a local problem can have a
far-reaching effect way outside of its scope. Usually it is best to keep code
changes local to where the problem arises. If you think that the solution
requires changes outside the scope of the problem, it is usually best to seek
the opinion of a team leader who may have another idea for how to solve the
problem.
4. Iterate with the contributor and improve the PR
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Maintainers should provide feedback and suggestions for improvement if they spot
things in the code that they would like changed. Preferably, suggestions should
come in order of importance: first, address overall code design and the approach
to solving the problem, then make sure the code is complying with the engine's
best practices, and lastly, do the :ref:`code style review <doc_code_style_review>`.
.. note::
**Communicate barriers to merging early in the review process.**
If the PR has clear blockers or will likely not get merged for whatever other
reason, that fact should be communicated as early and clearly as possible. We
want to avoid stringing people along because it feels bad to say "sorry, no".
As you review pull requests, keep the Godot `Code of Conduct
<https://godotengine.org/code-of-conduct>`_ in mind. Especially the following:
* Politeness is expected at all times. Be kind and courteous.
* Always assume positive intent from others.
* Feedback is always welcome, but keep your criticism constructive.
Here are some things to avoid as you iterate on a pull request with a
contributor:
* **Needless double reviews.**
In other words, review the full PR at once and avoid coming back endless times
to point out issues that you could have noted in the first review. Of course,
this can't always be avoided, but we should try to catch everything at once.
* **Being overly nitpicky.**
Code quality can be flexible depending on the area of the engine you are
working in. In general, our standard for code quality is much higher in core
areas and in performance-sensitive areas than it is in editor code for
example.
* **Expanding the scope of a pull request.**
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
as well while at it" or "could we add to this as well?" isn't always fair to
the contributor. Use your judgement when deciding whether additional fixes are
within scope, but try to keep the scope as close to the original pull request
as possible.
And ultimately, don't feel pressured to deal with the PR all alone. Feel free to
ask for a helping hand on the `Godot Contributors Chat
<https://chat.godotengine.org>`_, in the appropriate channel or in #general.
Other teams may already be tagged for review, so you can also wait or ask for
their assistance.
5. Approve the pull request
~~~~~~~~~~~~~~~~~~~~~~~~~~~
After reviewing the code, if you think that the code is ready to be merged into
the engine, then go ahead and "approve" it. Make sure to also comment and
specify the nature of your review (i.e. say whether you ran the code locally,
whether you reviewed for style as well as correctness, etc.). Even if you are
not an engine maintainer, approving a pull request signals to others that the
code is good and likely solves the problem the PR says it does. Approving a pull
request as a non-engine maintainer does not guarantee that the code will be
merged, other people will still review it, so don't be shy.
.. _doc_code_style_review:
Code style review
-----------------
Generally speaking, we aim to conduct a code review before a style/clarity
review as contributors typically want to know if their general approach is
acceptable before putting in the effort to make nitpicky changes to style. In
other words, maintainers shouldn't ask contributors to change the style of code
that may need to be rewritten in subsequent reviews. Similarly, maintainers
should avoid asking for contributors to rebase PRs if the PR has not been
reviewed.
That being said, not everyone feels confident enough to provide a review on code
correctness, in that case, providing comments on code style and clarity ahead of
a more substantive code review is totally appropriate and more than welcome.
In practice the code style review can be done as part of the substantive code
review. The important thing is that both the substantive code and the code style
need to be reviewed and considered before a pull request is merged.
When reviewing code style pay particular attention to ensuring that the pull
request follows the :ref:`doc_code_style_guidelines`. While ``clang-format`` and
various CI checks can catch a lot of inconsistencies, they are far from perfect
and are unable to detect some issues. For example, you should check that:
* The style of header includes is respected.
* Identifiers use ``snake_case`` and follow our naming conventions.
* Method parameters start with ``p_*`` or ``r_*`` (if they are used to return
a value).
* Braces are used appropriately, even for one-liner conditionals.
* Code is properly spaced (exactly one empty line between methods, no
unnecessary empty lines inside of method bodies).
.. note::
This list is not complete and doesn't aim to be complete. Refer to
the linked style guide document for a complete set of rules. Keep
in mind that ``clang-format`` may not catch things you hope it would,
so pay attention and try to build a sense of what exactly it can and
cannot detect.
Merging pull requests
---------------------
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,
the networking team leader could merge a networking pull request that doesn't
substantially change non-networking sections of code.
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
have a better sense of what other recent/upcoming changes this pull request may
conflict with (or any other reason that it may make sense to delay the pull
request). Feel free to leave a comment saying that the PR should be ready to
merge.
The following are the steps to take before merging a pull request. The degree to
which you adhere to these steps can be flexible for simple/straightforward pull
requests, but they should be carefully taken for complex or risky pull requests.
As a contributor you can help move a pull request forward by doing some of these
steps yourself.
1. Get feedback from the right people/teams
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
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
weigh in. In other cases, only one substantive approval is needed before the
code can be merged.
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
teams that may be impacted have been reasonably represented by the reviewers.
For example, if a pull request adds to the documentation, it's often useful to
let the area maintainers check it for factual correctness and let documentation
maintainers check it for formatting, style, and grammar.
A good rule of thumb is that at least one subject matter expert should have
approved the pull request for correctness, and at least one other maintainer
should have approved the pull request for code style. Either of those people
could be the person merging the pull request.
Make sure that the reviews and approvals were left by people competent in that
specific engine area. It is possible that even a long-standing member of the
Godot organization left a review without having the relevant expertise.
.. note::
An easy way to find PRs that may be ready for merging is filtering by
approved PRs and sorting by recently updated. For example, in the main Godot
repository, you can use `this link
<https://github.com/godotengine/godot/pulls?q=is%3Apr+is%3Aopen+review%3Aapproved+sort%3Aupdated-desc>`_.
2. Get feedback from the community
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
If a pull request is having trouble attracting reviewers, you may need to reach
out more broadly to ask for help reviewing. Consider asking:
* the person who reported the bug if the pull request fixes the bug for them,
* contributors who have recently edited that file if they could take a look, or
* a more experienced maintainer from another area if they could provide feedback.
3. Git checklist
~~~~~~~~~~~~~~~~
* **Make sure that the PR comes in one commit.**
When each commit is self-contained and could be used to build a clean and
working version of the engine, it may be okay to merge a pull request with
multiple commits, but in general, we require that all pull requests only have
one commit. This helps us keep the Git history clean.
* **Fixes made during the review process must be squashed into
the main commit.**
For multi-commit PRs check that those fixes are amended in the relevant
commits, and are not just applied on top of everything.
* **Make sure that the PR has no merge conflicts.**
Contributors may need to rebase their changes on top of the relevant branch
(e.g. ``master`` or ``3.x``) and manually fix merge conflicts. Even if there
are no merge conflicts, contributors may need to rebase especially old PRs as
the GitHub conflict checker may not catch all conflicts, or the CI may have
changed since it was originally run.
* **Check for proper commit attribution.**
If a contributor uses an author signature that is not listed in their GitHub
account, GitHub won't link the merged pull request to their account. This
keeps them from getting proper credit in the GitHub history and makes them
appear like a new contributor on the GitHub UI even after several
contributions.
Ultimately, it's up to the user if they want to fix it, but they can do so by
authoring the Git commit with the same email they use for their GitHub
account, or by adding the email they used for the Git commit to their GitHub
profile.
* **Check for proper commit messages.**
While we don't have a very strict ruleset for commit messages, we still
require them to be short yet descriptive and use proper English. As a
maintainer you've probably written them enough times to know how to make one,
but for a general template think about *"Fix <issue> in <part of codebase>"*.
For a more detailed recommendation see the `contributing.md
<https://github.com/godotengine/godot/blob/master/CONTRIBUTING.md#format-your-commit-messages-with-readability-in-mind>`_
page in the main Godot repository.
4. GitHub checklist
~~~~~~~~~~~~~~~~~~~
* **Validate the target branch of the PR.**
Most Godot development happens around in the ``master`` branch. Therefore most
pull requests must be made against it. From there pull requests can then be
backported to other branches. Be wary of people making PRs on the version they
are using (e.g, ``3.3``) and guide them to make a change against a
higher-order branch (e.g. ``3.x``). If the change is not applicable for the
``master`` branch, the initial PR can be made against the current maintenance
branch, such as ``3.x``. It's okay for people to make multiple PRs for each
target branch, especially if the changes cannot be easily backported.
Cherry-picking is also an option, if possible. Use the appropriate labels if
the PR can be cherrypicked (e.g. ``cherrypick:3.x``).
.. note::
It is possible to change the target branch of the PR, that has already been
submitted, but be aware of the consequences. As it cannot be synchronized
with the push, the target branch change will inevitable tag the entire list
of maintainers for review. It may also render the CI incapable of running
properly. A push should help with that, but if nothing else, recommend
opening a new, fresh PR.
* **Make sure that the appropriate milestone is assigned.**
This will make it more obvious which version would include the submitted
changes, should the pull request be merged now. Note, that the milestone is
not a binding contract and does not guarantee that this version is definitely
going to include the PR. If the pull request is not merged before the version
is released, the milestone will be moved (and the PR itself may require a
target branch change).
Similarly, when merging a PR with a higher milestone than the current version,
or a "wildcard" milestone (e.g. "4.x"), ensure to update the milestone to the
current version.
* **Make sure that the opening message of the PR contains the
magic words "Closes #..." or "Fixes #...".**
These link the PR and the referenced issue together and allow GitHub to
auto-close the latter when you merge the changes. Note, that this only works
for the PRs that target the ``master`` branch. For others you need to pay
attention and close the related issues manually. Do it with *"Fixed by #..."*
or *"Resolved by #..."* comment to clearly indicate the act for future
contributors.
* **For the issues that get closed by the PR add the closest
relevant milestone.**
In other words, if the PR is targeting the ``master`` branch, but is then also
cherrypicked for ``3.x``, the next ``3.x`` release would be the appropriate
milestone for the closed issue.
5. Merge the pull request
~~~~~~~~~~~~~~~~~~~~~~~~~
If it is appropriate for you to be merging a pull request (i.e. you are on the
production team or you are the team leader for that area), you are confident
that the pull request has been sufficiently reviewed, and once you carry out
these steps you can go ahead and merge the pull request.

54
engine/pull_requests/review_process.rst
View File

@@ -1,54 +0,0 @@
Review process
==============
.. note::
This page is intended to provide insight into the pull request (PR) review
process that we aspire to. As such, it is primarily targeted at engine
maintainers who are responsible for reviewing and approving pull requests.
That being said, much of the content is useful for prospective contributors
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
following:
1. A contributor opens a PR that fixes a specific problem (optimally closing
a GitHub `issue <https://github.com/godotengine/godot>`_ or implementing
a `proposal <https://github.com/godotengine/godot-proposals>`_).
2. Other contributors provide feedback on the PR (including reviewing and/or
approving the PR, as appropriate).
3. An engine maintainer reviews the code and provides feedback, requests
changes, or approves the pull request, as appropriate.
4. Another maintainer reviews the code with a focus on code style/clarity and
approves it once satisfied.
5. A team leader or a member of the `production team
<https://godotengine.org/teams#production>`_ merges the pull request if
satisfied that it has been sufficiently reviewed.
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
request workflow document <doc_pr_workflow>`.
.. note::
In practice these steps may blend together. Oftentimes maintainers will
provide comments on code style and code quality at the same time and will
approve a pull request for both.
Typically the first interaction on a pull request will be an engine maintainer
assigning tags to the pull request and flagging it for review by someone
familiar with that area of code.
Engine maintainers are folks who are "members" of the Godot project repository
on GitHub and/or are listed on the `Teams page <https://godotengine.org/teams>`_
on the Godot website. Maintainers are responsible for a given area of the
engine. Typically this means they are the people who are given more trust to
approve and recommend pull requests for merging.
Even if you are not a maintainer, you can still help by reviewing code,
providing feedback on PRs and testing PRs locally on your machine to confirm
that they work as intended. Many of the currently active maintainers started out
doing this before they became maintainers.

137
engine/pull_requests/testing.rst
View File

@@ -1,137 +0,0 @@
.. _doc_testing_pull_requests:
Testing pull requests
=====================
Many people are developing new features or fixing bugs on GitHub.
To help with engine development, you may be asked to test those pull requests
with a Godot build that includes code from the pull request in question.
Thanks to GitHub Actions, all `pull requests <https://github.com/godotengine/godot/pulls>`__
have continuous builds available. These builds let you try out pull requests
without having to compile anything from source.
Downloading a compiled build
----------------------------
You can download pull request builds from GitHub Actions. Since only signed in
users may download builds directly from GitHub Actions, the procedure varies
depending on whether you have a GitHub account or not.
.. note::
Due to a GitHub Actions limitation, builds are only available for 90 days
after the pull request was last updated. If you still wish to try a
pull request locally, you can
:ref:`compile the pull request branch from source <doc_testing_pull_requests_compile>`
instead.
If you have a GitHub account
~~~~~~~~~~~~~~~~~~~~~~~~~~~~
- Open the pull request page. Click the **Checks** tab near the top of the page:
.. image:: img/testing_pull_requests_access_checks.webp
- Click the **Artifacts** dropdown on the right of the page:
.. image:: img/testing_pull_requests_checks_artifacts.webp
- In the dropdown, click the artifact's name to download it. Remember to scroll
if you cannot see the name of the platform you're looking for:
.. image:: img/testing_pull_requests_checks_artifacts_list.webp
- Extract the ZIP archive then run the executable.
Note that Windows and macOS binaries are **not** code signed.
This means you may have to bypass a security warning before you can run the executable.
On Windows, if you frequently test pull request builds, it may be better to disable
Windows SmartScreen permanently in the Windows security settings.
On macOS, see :ref:`doc_running_on_macos` for instructions on bypassing Gatekeeper.
If you don't have a GitHub account
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
If you don't have a GitHub account and can't sign up for one,
you can use the third-party `nightly.link <https://nightly.link>`__ service
to generate a universal download link.
- Open the pull request page. Click the *fork*'s branch name near the top of the page:
.. image:: img/testing_pull_requests_access_fork.png
- Now that you are on the fork's branch page, click the ``.github`` folder at the top of the file list.
Then, click on the ``workflows`` folder (which is inside the ``.github`` folder).
Click the workflow file for the platform you wish to download artifacts for.
*After* clicking on the file (which opens the file view), copy the page URL from your browser's address bar.
- Open the `nightly.link <https://nightly.link>`__ website and paste the URL you just copied
into the text field located below the heading **Paste a GitHub link, get a nightly.link!**.
After pasting the URL, click **Get links** on the right.
If the format of the URL you pasted is correct, you should be presented
with a page like this:
.. image:: img/testing_pull_requests_nightly_link.png
- Click the URL of the artifact you wish to download.
- Extract the ZIP archive then run the executable.
Note that Windows and macOS binaries are not code signed.
This means you may have to bypass a security warning before you can run the executable.
If you frequently test pull request builds, it may be better to disable
Windows SmartScreen or `disable macOS Gatekeeper <https://disable-gatekeeper.github.io/>`__ permanently.
.. _doc_testing_pull_requests_compile:
Compiling a pull request branch from source
-------------------------------------------
This approach may be needed for pull requests that were last updated more than
90 days ago, or to test on platforms and configurations that are not supported
by Godot's GitHub Actions setup.
Downloading a zipped pull request branch
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
- Open the pull request page. Click the *fork*'s branch name near the top of the page:
.. image:: img/testing_pull_requests_access_fork.png
- Now that you are on the fork's branch page, click the green **Code** button on the right of the page
then choose **Download ZIP** in the dropdown:
.. image:: img/testing_pull_requests_fork_zip.png
- Extract the ZIP archive and follow the :ref:`compiling <toc-devel-compiling>` instructions
for your operating system.
Checking out a pull request branch with git
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Alternatively, you can checkout the pull request directly with git:
- Open the pull request page. Note the pull request *number* (``PR_NUMBER``), and the *branch name*
(``BRANCH_NAME``), but without the user name.
.. image:: img/testing_pull_requests_command_line_checkout.webp
- Construct the command using this pattern:
::
git fetch upstream pull/PR_NUMBER/head:BRANCH_NAME
So for the pull request above, the actual command will be:
::
# Fetch PR branch locally
git fetch upstream pull/48734/head:editor_file_dialog_filter_sort
- Once the pull request finishes downloading, checkout its branch:
::
git checkout editor_file_dialog_filter_sort
- And follow the :ref:`compiling <toc-devel-compiling>` instructions for your operating system.