mirror of
https://github.com/mapbase-source/source-sdk-2013.git
synced 2024-12-26 06:45:31 +03:00
Created Reviewing Mapbase pull requests (markdown)
Blixibon
parent
d40cabd63d
commit
a4f1adf6f1
133
Reviewing-Mapbase-pull-requests.md
Normal file
133
Reviewing-Mapbase-pull-requests.md
Normal file
@ -0,0 +1,133 @@
|
||||
This guide will explain how to review a pull request on the Mapbase repository.
|
||||
|
||||
*Note that this guide is currently a work-in-progress.*
|
||||
|
||||
---
|
||||
|
||||
## Quick summary of how pull requests work in Mapbase
|
||||
|
||||
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](https://github.com/mapbase-source/source-sdk-2013/blob/master/.github/CONTRIBUTING.md).
|
||||
|
||||
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 2 collaborators 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.
|
||||
|
||||
## Navigating to a pull request
|
||||
|
||||
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.
|
||||
|
||||
|
||||
|
||||
## Reading a pull request page
|
||||
|
||||
Once you are on the pull request page, there are three things you will need to check before starting a review.
|
||||
|
||||
1. Read the description to understand what the PR does.
|
||||
2. Verify that the pull request adheres to the PR Checklist.
|
||||
3. 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.
|
||||
|
||||
## Starting a review
|
||||
|
||||
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.
|
||||
|
||||
|
||||
|
||||
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.
|
||||
|
||||
---
|
||||
|
||||
### Reviewing the code
|
||||
|
||||
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 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.
|
||||
|
||||
---
|
||||
|
||||
### Testing changes in-game
|
||||
|
||||
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)
|
||||
|
||||
---
|
||||
|
||||
### Submitting a review
|
||||
|
||||
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.
|
||||
|
||||
---
|
||||
|
||||
## Merging a pull request (collaborators only)
|
||||
|
||||
Although anyone with a GitHub account can review, approve, and/or request changes to a pull request, collaborators with write permissions are the only ones with authority to merge them, and a PR must have an approving review from at least 2 collaborators in order to be merged.
|
||||
|
||||
If you are a collaborator with write permissions, then once a PR has been sufficiently reviewed and it is safe to merge it into the target branch, you should see an option to merge a pull request near the bottom of the page.
|
||||
|
||||
(picture of merge prompt)
|
||||
|
||||
This is the most (and possibly the only) dangerous part of the review process. Before merging, you can select a merge strategy to use. If you are familiar with Git, then you will most likely be familiar with these strategies. The other strategies mainly serve to keep the commit history clean, but they should only be used if the source branch will not be used further. If it will (or you're not sure), then you should use "Create a merge commit".
|
||||
|
||||
If you know that the source branch will not be used in the future, then you can either use "Squash and merge" to transform all of the commits into one commit on the target branch or "Rebase and merge" to add the commits to the target branch without creating a merge commit. This makes the commit history look cleaner than using a merge commit, but there are consequences of squashing or rebasing a branch which will merge the target branch back, so they should be used sparingly.
|
||||
|
||||
We may add a new checklist option to check the author's intent with the source branch in the future.
|
||||
Loading…
Reference in New Issue
Block a user