-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 R. David Murray wrote:
On Tue, 27 Apr 2010 11:15:49 +1000, Steven D'Aprano <steve@pearwood.info> wrote:
No, of course not. There are always other reasons, the biggest is too many things to do and not enough time to do it. If I did review patches, would they be accepted on the strength on my untrusted reviews?
It is very very helpful for *anyone* to review patches. Let's see if I can clarify the process a little. (This is, of course, my take on it, others can chime in if they think I got anything wrong.)
Someone submits a bug. Someone submits a patch to fix that bug (or add the enhancement). Is that patch ready for commit? No. Is it ready for *commit review* (ie: someone with commit privileges to look at it with an eye toward committing it)? Probably not.
What makes a patch ready for commit review? The patch should:
1) conform to pep 7/8 2) have unit tests that fail before the patch and succeed after 3) have documentation updates if needed 4) have a py3k port *if and only if* the port is non-trivial (well, if someone wants to add one when it is trivial that's OK, but it probably won't get used) 5) if it is at all likely to have system dependencies, be tested on at least linux and windows
This is an excellent set of guidelines. The only drawback I see here is that the current VCS situation makes doing the review more tedious than it should be, especially for non-committers. Or maybe the Hg mirrors are truly up-to-date and working? Last I looked, they were lagging or unavailable.
Making sure that these items are true does not require any in-depth expertise. In many cases it doesn't even require much time. 'Trusted' or 'untrusted' doesn't really come in to it, though doing these sorts of reviews will build trust. If you can in addition look at the patch content and critique it, so much the better. Again, *any* critique is useful, even if you can't review the whole patch in detail, because it gets it that much closer to being commit ready. And there are enough uncommitted patches in the tracker that it ought to be possible for almost anyone to find something they can usefully do a content critique on.
The goal is to make the commit review step as simple and fast for the committer as possible. The more eyes on the patch before hand, the faster the commit review will be. And those people who do a good job making patches commit ready will be on the fast track to getting commit privileges.
-- R. David Murray www.bitdance.com
PS: note that I'm using 'commit review' above with a different sense than that value is currently defined to have in the workflow. I'm thinking about advocating that the definition in the workflow be changed, and indeed we (the informal triage crew) have already occasionally used that setting with the meaning I give it above.
Tres. - -- =================================================================== Tres Seaver +1 540-429-0999 tseaver@palladion.com Palladion Software "Excellence by Design" http://palladion.com -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.9 (GNU/Linux) Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org iEYEARECAAYFAkvXLtkACgkQ+gerLs4ltQ79DACbB35/XFGyiYjd79OtTx+kgoNl mcsAnA4TNlM1ARjyrDrQIwv4KG48w/7h =1hGI -----END PGP SIGNATURE-----