Review - ltem/OpenRA GitHub Wiki

A guide how to review pull requests (PRs) for/from OpenRA.

TODO :construction_worker:

• Prerequisite - What is needed? Github account? Git? Source-Code? Refer to Contributing for setting up the basics, which is also exptected for the following commands
• Why reviews need to be done, what is the goal (modding focus), what is in scope of the PR (not everything can be changed/reviewed (too big) in one PR, needs a follow up PR or something else) !!!CONSTRUCTIVE CRITIC!!!
• Code reviewing:
  • giving actual feedback only online possible
    • comment changes
    • mark lines
    • give examples
  • if you want to review a PR offline, playtesting it, create test scenarios, then you have to download/checkout the PR
• How to do this via commandline (same for all operating systems)
  • git checkout (-> make optional changes, create test cases) -> make all (if something needs to be recompiled, for example if C#-files got changed by the PR or you) -> run game, do your tests

git fetch ???
git checkout ???
make all
./launch-game.sh

• If everything is fine -> approve the changes

• If you have found some issues -> report online

• git checkout -- PATH-TO-YOUR-CHANGED-FILES, to revert your changes, else you cannot go back to bleed e.g. you modified a yaml-file

git checkout -- /mods/ra/rules/vehicle.yaml

- git checkout bleed to get back -> make all for 

• It is much easier by setting up refs in the git config (location of git config /OpenRA/.git/config), Source: https://gist.github.com/piscisaureus/3342247
  • /.git/config looks something like that, below it can be several lines for the branches, which you possible have created

[core]
	repositoryformatversion = 0
	filemode = true
	bare = false
	logallrefupdates = true
[remote "origin"]
	url = http://github.com/$YOU/OpenRA.git
	fetch = +refs/heads/*:refs/remotes/origin/*
	pushurl = [email protected]:$YOU/OpenRA.git
[branch "bleed"]
	remote = origin
	merge = refs/heads/bleed
[remote "upstream"]
	url = http://github.com/OpenRA/OpenRA.git
	fetch = +refs/heads/*:refs/remotes/upstream/*
	fetch = +refs/pull/*/head:refs/remotes/upstream/pr/*

• Advanced Skills - Checkout PRs with Dependencies
• If there is a PR which depends on an other one and you want to test it you have to http://stackoverflow.com/questions/5410742/how-can-two-branches-be-combined-into-a-single-branch-based-on-the-date-of-each#5412356

git branch branchAB branchA
git checkout branchB
git merge branchA
git rebase --onto A1^ B1^ branchAB

• Very advanced Skills - Push to Pull Request
  • Maybe not wanted
  • HOW TO?

FROM GITLAB

 Step 1. Fetch and check out the branch for this merge request

git fetch upstream
git checkout -b pr-branch upstream/pr-branch

Step 2. Review the changes locally

# Step 3. Merge the branch and fix any conflicts that come up

# git checkout master
# git merge --no-ff pr-branch
 
# Step 4. Push the result of the merge to GitLab

# git push origin master

Tip: You can also checkout merge requests locally by following these guidelines. 

https://gitlab.com/help/user/project/merge_requests/index.md

==============FINDME========================

What you need

• A GitHub account (this website!)
• Git
• Windows: We recommend Git Extensions or msysgit. Experience showed that the GitHub for Windows client is not suited for our workflow.
• Linux/Mac: Git (via the command line) should come bundled with your OS (you may want to update it or use a GUI application).
• C# IDE (if you intend on modifying code)
• Windows: Microsoft Visual Studio 2013 or above or SharpDevelop
• Linux/Mac: MonoDevelop is very similar to Visual Studio and is the most common Mono/.NET IDE.

GitHub account setup

TIP: If you are interested purely in compiling the code, and not contributing, feel free to skip to the Cloning step, replacing $YOU with OpenRA, then skip to the Compiling section.

Forking

The first step for new contributors is to fork the OpenRA repository. Forking creates a copy of the repository that belongs to your GitHub account. As an example, this is one of the developer's forks; they are free to make any changes to their repository, having no effect on the OpenRA repository. At a later point they may ask to create a pull request to merge their changes to the OpenRA repository. This does not put any code on your machine, so the next step is to start using git.

To fork, open the OpenRA repo page and press the fork button () on the top right of the screen.

Git setup

Git is a source control tool used by many projects (including OpenRA), while GitHub is a web interface for a Git repository. Git will allow you to manage your copy of the OpenRA source code. This guide uses the Git command-line, for beginners it can be difficult, but these commands are consistent among all Git clients.

In the guide whenever you see $YOU, replace it with your GitHub username.

Configuration

For GitHub to know who you are when using Git, type these two commands:

git config --global user.name $YOU
git config --global user.email [email protected]

TIP: You may wish to use your real name instead of your GitHub username for the user.name field.

Cloning

To get the source code on your machine and change to that directory, type these two commands:

git clone http://github.com/$YOU/OpenRA.git
cd OpenRA

TIP: the code will be in the current directory + /OpenRA/

Remotes

Add a remote for the main OpenRA repository, named upstream. Type this command:

git remote add upstream http://github.com/OpenRA/OpenRA.git

Another remote named origin was added when you performed the clone, which points to your own repository.

TIP: Git can interact with other GitHub repositories by providing either a path to the repository, or the remote name. The upstream remote that was just set up points to the http://github.com/OpenRA/OpenRA.git path. Both could be used interchangeably. You can also add remotes for other developers' repositories as well if desired.

TIP: To see a list of configured remotes run git remote -v.

TIP: origin and upstream are common conventions for remote names, referring to the origin of code (where you cloned from), and the official OpenRA repository. You may add more remotes later with different names.

Fetching

Ensure your copy of Git is up to date with all the various commits / branches / tags that exist in the OpenRA repository on GitHub by entering this command:

git fetch --all

TIP: At this point in time your code is probably up to date regardless. For future fetches, you should only need to use git fetch upstream.

Status check

To make sure everything is good, type this command:

git status

You should see On branch bleed and Your branch is up-to-date with origin/bleed.

TIP: Your bleed branch will be identical to the OpenRA repository's bleed branch at this time.

The OpenRA repository has three branches:

• Master (contains code and tags for releases).
• Next (contains code and tags for playtests).
• Bleed (contains code at the latest state).

Keeping up to date (rebasing)

Your copy of the code will eventually fall behind and no longer be the most recent. If you are curious if your bleed branch has fallen behind, type git status.

If you're up to date, you'll see: Your branch is up-to-date with 'upstream/bleed'.

If you're behind, you'll see: Your branch is behind 'upstream/bleed' by # commits, and can be fast-forwarded.

We recommend that you perform a git rebase by executing these two commands:

git fetch upstream
git rebase upstream/bleed

TIP: You may run into a merge conflict, which means that your committed code has a conflicting change with other code that has changed in upstream. Unfortunately at this time we do not have documentation on how to solve merge conflicts, but the internet does - feel free to ask via IRC for help.

Compiling

Please see the Compiling -> Test your changes wiki page for how to build.

Creating a commit

Branching

Work can be done entirely on your bleed branch, but it's advised to switch to different branches when working on different features or bugfixes. Developers will have many branches (and don't follow the master/next/bleed structure in the OpenRA repository). These branches can be explored by visiting a developer's GitHub OpenRA repository and clicking the "Branches" drop down list.

Here's a way to make sure you create a branch based off of the latest OpenRA repository bleed, and change to it:

git fetch upstream
git checkout -b myfeature upstream/bleed

This will create the branch myfeature, which will be based on upstream/bleed, and check it out.

TIP: This branch won't be visible on GitHub until you push it (mentioned later on).

Committing

Assuming that:

• Your branch is up to date,
• And you've made the code changes you want,
• And (if you intend to submit a pull request) your code follows the OpenRA coding standards,

You can prepare a commit. Type git status to see the staged changes you have. It will mention there are files not staged for a commit. There's two ways to do the commit:

1. If all of the files modified need to be committed, you can type

git commit -a -m "Added brand new feature"

2. If you want to only commit certain files, you can type commands similar to this:

git add file1
git add file2
git commit -m "Added brand new feature"

TIP: git rm file1 removes a file from staging.

Pushing to GitHub

Once your patch has been committed to your local OpenRA repository, it still isn't visible on your GitHub repository. To do this type the command:

git push origin myfeature

You should see a message like [new branch] myfeature -> myfeature. You can see this is successful by navigating to your GitHub page and clicking the Branches drop down list.

You can also push your local bleed branch to your GitHub repository in the same way:

git push origin bleed

Pull Requests

You can save some work (both for us, and for yourself) by making sure that your pull request passes the following checklist before creating your pull request:

• Have you considered how your changes integrate with the rest of the game? You may be asked to rethink your approach if it interacts badly with other systems.
• Have you tested all of your changes?
• Do your changes conform to the OpenRA coding standard?
• Does it pass our lint test suite (run via make test)?

If so, and you're satisfied with your patch, then feel free to create a pull request.

Creating a Pull Request

There are two ways to create a pull request.

1. If you open the OpenRA repository page you should see a button called Compare & pull request that wasn't there before, click this button.

Example pull request by user cjshmyr for their returntobase branch

2. If you open the OpenRA repository page and navigate to the pull requests page and click New Pull Request. If you continue with this method, make sure you click the compare across forks hyperlink:

Example pull request by user cjshmyr for their returntobase branch

Ensure the base refers to OpenRA's bleed branch, while the head refers to your own branch.

Example pull request by user cjshmyr for their returntobase branch

Once you've done either 1 or 2 above, you can fill in details about your pull request. The title should briefly describe what your fix is, and the description should give a detailed information about what your patches do.

Example pull request by user cjshmyr for their returntobase branch

Also worth paying attention to is that the commit history below matches your branch (there aren't any extra commits that shouldn't be there). Sometimes when a pull request is incorrectly set up, you will see several other peoples' commits too.

Once satisfied, click the Create Pull Request button. You will be transported to a page containing your pull request.

Common questions

What is this "We can't automatically merge these branches" message when creating a pull request?

You are still fine to create the pull request. This happens when your branch is not rebased with the latest copy of OpenRA's bleed branch. See this section of the guide for an overview of rebase if required, and continue on with the guide.

Pull request process

Here's a snapshot of the pull request queue, to give an example of what might be in there:

Labels

Your pull request will have tags associated with it. These tags can only be added by members of the OpenRA repository. Labels are color coded.

• Orange/Red require a change by the user who opened the pull requests
• Green labels require action by a reviewer

Reviewing

Your pull request also requires that it will be reviewed by at least 2 separate reviewers, who will:

• Test your code to ensure it works
• Review your code to ensure it follows OpenRA's standards

Updating a Pull Request

You may be asked to update your pull request if:

1. Your code doesn't compile
2. Code style violations were found during review
3. Bugs were found during testing
4. Code has a design problem
5. Your branch is no longer up to date with OpenRA's bleed branch

1. Code does not compile - OpenRA's repository is set up to automatically compile pull requests, using a utility named Travis CI. This ensures that your code actually builds, and it passes automated tests that are put in place.

You can tell the build has failed if you see this on your pull request:

If this occurs, a reviewer may leave a message on your pull request about what the problem was. If you are interested in finding out for yourself, you can attempt to read the Travis CI build log, ask someone, or try building the code for yourself again.

2. Code style violations - Review the coding standard. It may not follow your personal standard, but this is common among any established code base.

3. Bugs found - Feel free to ask questions for help if you're unsure how to fix a bug that is found by a reviewer.

4. Code design problem - Your patch may work but it might not be in the best interest of the engine in the long run. You may be asked to change your code to work a bit differently. Work with the reviewer on the pull request (or even better in IRC) if you have questions.

5. Rebase required - You can be asked to rebase your changes. If a rebase is required, on your pull request page you will see:

If you type these git commands (while on your branch):

git fetch upstream
git rebase upstream/bleed
git push origin +myfeature

TIP: + means to force a push to the myfeature branch, it will overwrite your branch history, be careful!

TIP: A force push is only required if you are overriding an existing branch (usually done when squashing commits).

TIP: myfeature refers to your branch name.

You should see Git pull down the latest changes, but then also add Applying: messages to your commits. This is applying your commit messages after pulling down the latest copy of bleed.

Visit your pull request page again -- if you have rebased successfully, you will see: