Changes between Version 20 and Version 21 of PatchReviewProcess


Ignore:
Timestamp:
2013-08-08T11:58:29Z (11 years ago)
Author:
daira
Comment:

remove obsolete darcs-related info and other minor tweaks

Legend:

Unmodified
Added
Removed
Modified

  • PatchReviewProcess

    v20 v21  
    2727  a. If you can't understand the patch after spending some time on it, then say so in a comment on the ticket!  This might mean that we need to add documentation or comments or to refactor the code.  On the other hand, it might just be that you don't have enough context to understand the code.  That's okay too, so go ahead and speak up.
    2828  b. Check whether every feature or bugfix in the patch has an accompanying test in the patch.
    29   c. If you find errors or omissions in the docs, tests, code or comments then write that down in the ticket, remove the {{{review-needed}}} keyword from the keywords, and assign the ticket to someone other than yourself.  (Assign it to the original author of the patch, or someone who seems likely to fix the patch, or "nobody".)
    30   d. If you understand the patch and find no errors or omissions then write a comment on the ticket saying that you reviewed it, remove the keyword {{{review-needed}}}, add the keyword {{{reviewed}}} and assign it to someone with repository write access (currently 'zooko', 'warner', 'davidsarah', and 'kevan'). We'll commit it to trunk.
     29  c. If you find errors or omissions in the docs, tests, code or comments then write that down in the ticket, remove the {{{review-needed}}} keyword from the keywords, and add the {{{test-needed}}} keyword if that applies. Then assign the ticket to someone other than yourself.  (Assign it to the original author of the patch, or someone who seems likely to fix the patch, or "nobody".)
     30  d. If you understand the patch and find no errors or omissions then write a comment on the ticket saying that you reviewed it, remove the keyword {{{review-needed}}}, add the keyword {{{reviewed}}}, and assign it to someone with repository write access (currently 'zooko', 'warner', 'daira', and 'kevan'). We'll commit it to trunk.
    3131  e. Feel good about yourself.  Thank you for helping with our little project attempting to improve the world!
    3232
    … …  
    41414. Write comments about specific parts of the code. This demonstrates that you actually read it carefully enough to understand and care about parts of it, and it makes the author feel good that someone is talking about what they wrote.
    4242
    43 == Ticket attachment links ==
     43== Using trac and github ==
    4444
    45 NOTE: we've moved from darcs to git, and we've started using github for some things and continuing to use trac for others. So the following is partially obsolete and needs to be rewritten.
     45The patch you're reviewing might be given either as an attachment, or as a github pull request. If it's the latter, then it's encouraged to use line comments on github for detailed comments or questions on the code. However, you should also write a short summary of the review on the trac ticket. (Sometimes this can be as simple as "+1" if there are no issues to discuss.)
    4646
    47 Attachments in a ticket have two links, a name, which points to an html page with prettification, and a raw download link.  You can save the download link (such as by {{{wget}}}) and use {{{darcs apply}}} to configure your repository to test a particular patch. Note that you do not ''have'' to apply a patch to your local repository or test it in order to review it—often just reading the prettified version on the web is sufficient.
    48 
    49 For the #1149 ticket, for example, there are two links for the attachment named "test-for-webopen.darcs.patch":
    50 
    51  * The link with a name points here: https://tahoe-lafs.org/trac/tahoe-lafs/attachment/ticket/1149/test-for-webopen.darcs.patch
    52  * The link with a download icon, here: https://tahoe-lafs.org/trac/tahoe-lafs/raw-attachment/ticket/1149/test-for-webopen.darcs.patch
    53 
    54 The latter can be used to apply a patch, like this example shows:
    55 
    56 {{{
    57 $ wget 'https://tahoe-lafs.org/trac/tahoe-lafs/raw-attachment/ticket/1149/test-for-webopen.darcs.patch'
    58 --2011-07-31 20:23:39--  http://tahoe-lafs.org/trac/tahoe-lafs/raw-attachment/ticket/1149/test-for-webopen.darcs.patch
    59 Connecting to 127.0.0.1:8123... connected.
    60 Proxy request sent, awaiting response... 200 Ok
    61 Length: 18275 (18K) [text/x-diff]
    62 Saving to: “test-for-webopen.darcs.patch”
    63 
    64 100%[==============================>] 18,275      21.8K/s   in 0.8s   
    65 
    66 2011-07-31 20:23:54 (21.8 KB/s) - “test-for-webopen.darcs.patch” saved [18275/18275]
    67 
    68 $ darcs apply test-for-webopen.darcs.patch
    69 Finished applying...
    70 }}}
     47In all cases it's recommended to apply the patch or check out the code and run the full test suite locally (using `python setup.py test` since a rebuild is usually necessary), to check that it passes. All committed code should also be free of pyflakes errors or warnings.