Anonymous user
Open Source Comes to Campus/Newcomer Tasks/Issue Tracker Cleaning: Difference between revisions
Open Source Comes to Campus/Newcomer Tasks/Issue Tracker Cleaning (view source)
Revision as of 20:45, 29 July 2014
, 9 years agono edit summary
No edit summary |
No edit summary |
||
Line 60:
* List all versions of project in which the bug wasn't present for you.
* List all operating systems on which the bug wasn't present for you.
== Reviewing Patches/Pull Requests ==
If you want to work on reviewing patches, filter the issues you see so that you're only looking for "open", "needs review", or "has patch" issues. Again, some projects use different terminology, so you may want to ask a mentor to help you figure out what to filter for. You're looking for issues where a problem has been reported and a pull request or patch has been submitted.
Some issue tracker systems have a standard way to display patches that fix an issue. For instance, Github will show pull requests that reference an issue in the thread for that issue. Bugzilla will show patches in a list of attachments. Other systems require contributors to make commits and then manually leave a comment linking to the commit. It may take a bit of practice to get good at figuring out when when a patch has been submitted.
Once you've found a patch, you can review it by applying it to the project and seeing if it addresses the issue reported, either by fixing an issue or adding an enhancement. Here is a checklist you can use when making your review:
* Does the patch apply cleanly, or does it cause new errors and/or test failures?
* Does the patch address the problem reported?
* Does the patch implement the solution in a straightforward way, or is it confusing? (It's okay if you're not sure whether the solution is straightforward or not! You can always ask why the submitter designed the patch the way they did.)
* Is the patch accompanied by new tests that address the bug or the new functionality?
* If the patch includes tests, make sure that the tests fail when the change is reverted (otherwise it is not a good test).
* If functionality was added or changed, is the patch accompanied by new or updated documentation?
Here is an example comment you might leave:
<blockquote>
jesstess, thanks for your patch and tests! The docstring for parse_request needs to be updated as part of this change.
Other than that, the patch looks correct from my inspection, and applies cleanly. I verified that with the patch applied, the reproducer no longer raises a ValueError and instead prints 42 as expected. make patchcheck also reports no errors, and the test_httplib tests still pass. I confirmed that the tests for parsing unicode strings that were added in this patch exercise the bug, as they fail with the code changes reverted and pass with the full patch applied.
The implementation looks straight-forward to me, although I'm not sure why you chose to separate your changes out into a new function. Can you explain your reasoning?</blockquote>
| |||