Reviewing Mapbase pull requests
This guide will explain how to review a pull request on the Mapbase repository.
A pull request (or "PR") is a request to add code to the repository. Under most circumstances, pull requests on the Mapbase repository are required to follow the contributing guidelines.
After a pull request is opened, it must pass the repository's automated checks. In Mapbase's case, GitHub Actions automatically builds the pull request's code on both Windows and Linux to ensure that it compiles properly. If the compilation fails, then the check fails. If it compiles properly, then the binaries will be made available for download by anyone with a GitHub account.
If all checks have succeeded, then the pull request must be manually reviewed and approved by other users. An approving review from at least 1 collaborator with write access is required in order for a pull request to be merged.
Once a pull request is merged, it becomes "Closed" and is filed away.
If a pull request fails the appropriate checks and reviews, and the author doesn't/couldn't make changes, then the pull request can be closed by either the pull request's author or collaborators on the repository.
When a new pull request is opened, it will appear under the "Pull requests" tab on this repository.
Once there, you can reach the pull request by clicking on its title.
Once you are on the pull request page, there are three things you will need to check before starting a review.
- Read the description to understand what the PR does.
- Verify that the pull request adheres to the PR Checklist.
- Scroll down and verify that all automated checks have passed.
NOTE: Some PRs might not have a checklist, or even a description altogether. This can only mean that it was removed by the author. This is typically a mistake and does not usually reflect well on the quality of the pull request, but it is not necessarily an automatic rejection as long as it still follows contribution guidelines and targets the correct branch. Just use your best judgment on a PR which does this.
Once you understand what the PR is supposed to do, you can find out what it changed by going to the "Files changed" tab. This will show you the differences between the pull request's code and the code of the branch it is being merged into. New content is highlighted in green and removed content is highlighted in red.
This page may look intimidating, but in most cases, you will only be using a few of the buttons here.
Pull requests can either change a few lines or add entire files. In a review, you have the freedom to leave a comment on any change. However, if you are a collaborator with write access, your main objective is to ensure that the pull request lives up to Mapbase's guidelines and standards of quality. Ideally, you should also ensure that the changes function properly in-game, which can most easily be done with GitHub's automated binaries. If you are reviewing a pull request and are unable to test changes in-game, you should note this in the main comment.
This guide will cover both reviewing the code itself and testing the changes in-game.
If you find a change you would like to comment on, hover over the line and click on the plus icon at the beginning of the line.
If there are multiple lines you'd like to comment on, click on the plus and drag downwards.
Both of these actions will open a prompt to write a comment. You can write anything in this comment, but you will generally only need to comment on a line of code to request a change.
If you would like to suggest a very specific change to the relevant lines, you can optionally click on the "Add a suggestion" button to append your comment with a direct modification of the code.
Again, this is not required to make a comment.
Once you have written your comment, you should click on the "Start a review" button.
If this is the only comment you plan on making and you are not starting an actual approving review or request for changes, you can use the "Add single comment" button, but generally, you will want to start a full review.
After starting a review, you will be able to add other comments on other lines as part of one single review.
Note that a thorough review of the code sometimes isn't feasible for larger amounts of changes, especially when it involves code which the reviewer might not be familiar with (e.g. completely new files created by the author). This can be a problem if the pull request specifically needs your review in order to be merged. In these situations, you should still skim through the changes, but your goal would more-so be to ensure that the game still works and the pull request plays well with the existing codebase and guidelines.
This step is technically separate from the review process. You can choose to do it at any point during a code review, but generally, you should do this after you've looked at the code and added comments before you've submitted the review itself. The review comments will be saved if you navigate away from the webpage.
The easiest way to test changes in-game is to download the binaries produced by GitHub's automated checks. To do this, go to the "Checks" tab on the pull request.
Here, you can see all of the logs from when GitHub Actions built the relevant code. You can download the binaries produced by this process by clicking on the "Artifacts" drop-down menu on the right side of the screen.
Several download options will be available for separate builds on Windows and Linux using Debug and Release configurations. Download the binaries that correspond to your system. You will also usually want to use the Release configuration.
After the download is complete, you should now have a .zip file containing the relevant binaries. You can drag and drop these into your Mapbase mod's bin folder to replace the existing binaries.
After doing that, you should now be able to launch the game with the changes included in the PR. Exactly what you should test at this point depends on what changes are included in the PR. If the PR changes the pistol's upwards view kick, you can test the HL2 pistol and see if the view kick is different. If the PR adds new I/O/KV, you can test them using the ent_fire command.
Once you have tested the changes, you can note whether the test went smoothly in your review.
(Note that these automated binaries disappear from the site after 90 days)
Once you are done making comments and/or you have tested the changes in-game, you can finish your review by clicking "Review changes" on the right side of the screen.
You will be given a prompt to finish your review with an optional summary comment and a marker indicating the result of your review. Use "Approve" if you believe the pull request is ready to be merged, and use "Request changes" if you are requesting changes. Then, just click "Submit review".
You can do this regardless of whether you've made any comments on the lines themselves.
A section explaining how to merge pull requests as a collaborator will be added to this guide at a later time. For the time being, leave merges to Blixibon.