Difference between revisions of "Code Review"
Foxfirefey (Talk | contribs) m (→Remove the review tag) |
|||
| (10 intermediate revisions by 6 users not shown) | |||
| Line 1: | Line 1: | ||
| − | + | Code Review is an important task that must be done for pull requests submitted to GitHub. By doing this task, you help <dwuser>mark</dwuser> and <dwuser>fu</dwuser> spend less time doing code review and more time coding. | |
| − | + | You can find pull requests for the two main code repositories: | |
| + | |||
| + | * [https://github.com/dreamwidth/dw-free/pulls dw-free] | ||
| + | * [https://github.com/dreamwidth/dw-nonfree/pulls dw-nonfree] | ||
== Code Review Steps == | == Code Review Steps == | ||
| Line 7: | Line 10: | ||
=== Style compliance === | === Style compliance === | ||
| − | Run through the [[Programming Guideline Checklist]]. If the patch violates any of these guidelines, | + | Run through the [[Programming Guideline Checklist]]. If the patch violates any of these guidelines, comment with specific instructions on what is wrong, ideally on the lines that have violations. |
| − | === | + | === Build a test-case === |
| − | Update to the latest committed code, as per the instructions in [[Dev Maintenance]]. Apply the | + | Look at what the patch is supposed to fix and reproduce it on your own installation. Already at this step it is best to think of several ways this patch can be tested so you can see what happens before and after you apply the patch. |
| + | |||
| + | === Apply the branch from the pull request === | ||
| + | |||
| + | Update to the latest committed code, as per the instructions in [[Dev Maintenance]]. Apply the commits from the pull request. The easiest way to do this is checking out the master branch (git checkout master) and then using [http://hub.github.com/ the hub command] (if it is installed) with the link to the pull request: | ||
| + | |||
| + | hub checkout https://github.com/dreamwidth/dw-free/pull/000 | ||
| + | |||
| + | If the pull request doesn't apply cleanly to the master branch, comment with a note to check their branch against the latest committed code. They may have forgotten to update before submitting, or there may have been changes committed since they submitted! | ||
=== Test the functionality === | === Test the functionality === | ||
| − | Note what the | + | Note what the pull request is meant to fix. (The originating issue number should be in the title of the branch and each commit.) Verify that the issue is fixed or that the new feature is working properly. Try to see if you can make things break. |
| + | |||
| + | Suggestions for things to test for, as applicable: test with paid, free, personal and community accounts, logged in and logged out, use different site schemes and layouts, try foreign characters in user input or empty input, entries and user names that don't exist, and anything else you can think of. | ||
| − | === | + | === Make appropriate commentary === |
| − | If the patch follows style guidelines, applies cleanly, and functions, you may now note that you have reviewed the patch for all of these | + | * If the patch follows style guidelines, applies cleanly, and functions, you may now note that you have reviewed the patch for all of these in a comment. |
| + | * If you find a patch still needs work, you can explain in a clear and friendly manner what should be done in a comment. Ideally, match the commentary as line comments on the sections that need wor. | ||
[[Category: Development]] | [[Category: Development]] | ||
Latest revision as of 17:05, 1 July 2019
Code Review is an important task that must be done for pull requests submitted to GitHub. By doing this task, you help ![[info]](http://wiki.dreamwidth.net/wiki/extensions/LJ/images//dw/user.png){kind=small}
mark and fu spend less time doing code review and more time coding.
You can find pull requests for the two main code repositories:
Contents
Code Review Steps
Style compliance
Run through the Programming Guideline Checklist. If the patch violates any of these guidelines, comment with specific instructions on what is wrong, ideally on the lines that have violations.
Build a test-case
Look at what the patch is supposed to fix and reproduce it on your own installation. Already at this step it is best to think of several ways this patch can be tested so you can see what happens before and after you apply the patch.
Apply the branch from the pull request
Update to the latest committed code, as per the instructions in Dev Maintenance. Apply the commits from the pull request. The easiest way to do this is checking out the master branch (git checkout master) and then using the hub command (if it is installed) with the link to the pull request:
hub checkout https://github.com/dreamwidth/dw-free/pull/000
If the pull request doesn't apply cleanly to the master branch, comment with a note to check their branch against the latest committed code. They may have forgotten to update before submitting, or there may have been changes committed since they submitted!
Test the functionality
Note what the pull request is meant to fix. (The originating issue number should be in the title of the branch and each commit.) Verify that the issue is fixed or that the new feature is working properly. Try to see if you can make things break.
Suggestions for things to test for, as applicable: test with paid, free, personal and community accounts, logged in and logged out, use different site schemes and layouts, try foreign characters in user input or empty input, entries and user names that don't exist, and anything else you can think of.
Make appropriate commentary
- If the patch follows style guidelines, applies cleanly, and functions, you may now note that you have reviewed the patch for all of these in a comment.
- If you find a patch still needs work, you can explain in a clear and friendly manner what should be done in a comment. Ideally, match the commentary as line comments on the sections that need wor.
