classification
Title: Add 'How to Review a Patch' section to devguide
Type: enhancement Stage: resolved
Components: Devguide Versions:
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: SilentGhost, Winterflower, ezio.melotti, ned.deily, python-dev, terry.reedy, willingc
Priority: normal Keywords: patch

Created on 2015-12-24 21:20 by Winterflower, last changed 2016-06-05 19:12 by ned.deily. This issue is now closed.

Files
File name Uploaded Description Edit
patchreview.patch Winterflower, 2015-12-24 21:20
patchreview18May.patch Winterflower, 2016-05-18 20:34
Messages (6)
msg256970 - (view) Author: Camilla Montonen (Winterflower) Date: 2015-12-24 21:20
This list is based on helpful tips and discussions received on the core-mentorship list and aims to help new beginners review patches in the bug tracker. The submitted patch is still in progress (the layout is a bit wonky and some details are still missing).
msg256981 - (view) Author: SilentGhost (SilentGhost) * (Python triager) Date: 2015-12-25 10:19
Content of the article is in very reasonable shape, I have only couple of notes:
1. I don't think "production" is a good description of the python's repository workflow, so I'd suggest changing it to "repository".
2. Lack of links: instead of saying "check the devguide" a link to the relevant section would be useful. Though it applies even when no text is going to be saved.

Technically, there are couple more issues, such as extra empty lines at the start and end of file, page title should be followed by an empty line. Please trim all the trailing spaces. The list items should all have the same indentation. Importantly, please restrict width of each line to under 80 characters: while a couple characters above this limit typically will do no harm, the lines should generally be of the same width. More links!
msg257121 - (view) Author: Ezio Melotti (ezio.melotti) * (Python committer) Date: 2015-12-28 17:51
+1 to the above message.

There are also a couple of things that should be corrected:
* bugtracker -> bug tracker
* If the patch makes codechanges to C codebase -> If the patch affects any C file

This could also be condensed a bit and merged with https://docs.python.org/devguide/patch.html#reviewing instead of creating a new page.
msg265836 - (view) Author: Camilla Montonen (Winterflower) Date: 2016-05-18 20:34
Thank you very much for your comments. I have cleaned up the previous patch and merged it with the Lifecycle of a Patch article.
msg267461 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2016-06-05 19:07
New changeset 8b3f4473432e by Ned Deily in branch 'default':
Issue #25941: Add "How To Review A Patch" section to the devguide.
https://hg.python.org/devguide/rev/8b3f4473432e
msg267463 - (view) Author: Ned Deily (ned.deily) * (Python committer) Date: 2016-06-05 19:12
The revised patch looks good to me (other than some trailing whitespace).  Camilla, thanks for your contribution.  If you haven't already, please sign a contributor form to cover this and future contributions as noted elsewhere in the Developer's Guide:

https://docs.python.org/devguide/coredev.html#sign-a-contributor-agreement
History
Date User Action Args
2016-06-05 19:12:59ned.deilysetstatus: open -> closed

nosy: + ned.deily
messages: + msg267463

resolution: fixed
stage: patch review -> resolved
2016-06-05 19:07:59python-devsetnosy: + python-dev
messages: + msg267461
2016-05-18 20:34:25Winterflowersetfiles: + patchreview18May.patch

messages: + msg265836
2015-12-28 17:51:21ezio.melottisettype: enhancement
messages: + msg257121
stage: patch review
2015-12-25 10:19:07SilentGhostsetnosy: + SilentGhost
messages: + msg256981
2015-12-24 22:06:57terry.reedysetnosy: + terry.reedy
2015-12-24 21:20:57Winterflowercreate