-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[WIP] Added documentation for handing pull requests #20
Open
DrakBoul
wants to merge
3
commits into
main
Choose a base branch
from
drake/documentation/add_PR_review_documentation
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,46 @@ | ||
| # Code Reviews | ||
|
|
||
| ## What is a code review? | ||
| When you, or another team member wants add changes to an AlbertaSat repository they will go through a process defined by the (contributing guideline section)[Contributing_README.md]. This typically involves creating a branch, making changes, making a pull request, code review, then the reviewer either asks for changes to be made or approves the pull request. After the pull request is approved the code can be merged to main, or additional changes can be made. In this documentation we will go over guidelines for reviewing and writing pull requests. | ||
|
|
||
| ## How should a pull request look like? | ||
| __This section we will go over to how write a proper pull request that allows code reviewers to quickly and efficiently review code.__\ | ||
| \ | ||
| The following are some general rules of what a pull request should look like:\ | ||
| - Change the default pull request name from the branch name to a descriptive name summarizes the changes you want to make. | ||
| - Add a useful description that includes the following: | ||
| - The issue number linked to the pull request. | ||
| - Any dependent pull requests in a different repository that should be reviewed first. | ||
| - Describe the changes you made and how the changes you made solve the root issue. | ||
| - If neccesary, clearly state any feedback you are looking for with a mention to the person who can help with feedback. | ||
| - If your pull request is still a work in progress, prefix the name of the pull request with "[WIP]". | ||
|
|
||
| ## How should a pull request be reviewed? | ||
| __This section we will go over to how review a pull request.__\ | ||
| \ | ||
| The following are some general rules of how a reviewer should review a pull request:\ | ||
| - If the PR is labelled as "WIP" then it should not be approved. | ||
| - Thoroughly go all changes made and if neccesary leave feedback. | ||
| - When leaving feed back try to be as specific as possible (Comment with the specific code line, and the requested change). | ||
| - Checkout the branch on your machine and test to ensure it does not break anything and implements the feature as intended. | ||
| - When requesting changes, be specifc about what you want changed (comment or propose a modified code block or line). | ||
| - Adhere to definitions of pull request approval, request for changes, and comments found in definition section. | ||
| - After the pull request has been merged (by either reviewer or creator of the pull request) the corresponding branch should be deleted and the pull request should be closed. | ||
|
|
||
| ## Definitions | ||
|
|
||
| __Approve__: A pull request should only be approved after the reviewer has completed the following: | ||
| - reviewer has reviewed all code | ||
| - reviewer has tested code by checking out branch | ||
| - reviewer has confirmed the pull request implements the intended feature | ||
| - the pull request is not marked as in progress | ||
| - the pull request does not break any existing features | ||
| - creator of pull request has confirmed it is ready to be merged\ | ||
| \ | ||
| In summation, the pull request can only be marked as approved once it is ready to be merged with main. If any changes or feedback is given, the changes should be implemented and the creator of the pull request should re-request a review. | ||
|
|
||
| __Request for Changes__: A pull request can be reviewed and the reviewer can request changes, all changes should be resolved (fixed or ruled as not neccesary) before being re-reviewed. The reviewer should do their best to be specific about what changes should be made and if neccesary, explain how to implement them. | ||
|
|
||
|
|
||
| __Comment__: A pull request can be reviewed by leaving a comment which is general feedback for the creator can implement. After the creator of the pull request has resolved the feed back they can re-request a review. Although the feed back is general, the reviewer should try and be as helpful as possible in their feedback to help resolve the issue. | ||
|
|
||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo - 'go OVER all changes'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rrasmuss4200 Oops, I didn't get an email notification for any of your comments so I did not see them until now. I'll give it another read over and fix the typos. Also I'll add a section for drafting PR's. Thanks for the feedback!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rrasmuss4200 I fixed the typo and added a code styling section for rust (Using clippy and rust analyzer).
On a different note, in the contributing document there is still a section about using copyright information which I think may be out of date. I remember Devin mentioning that we no longer need to put the copyright info in each code file and now we just have a single license doc in the repo. Let me know if I should remove that section in this PR.